Добро пожаловать в форум, Guest  >>   Войти | Регистрация | Поиск | Правила | В избранное | Подписаться
Все форумы / Microsoft SQL Server Новый топик    Ответить
 Интересует Ваше мнение о качестве кода  [new]
RubinDm
Member

Откуда: SPb
Сообщений: 458
Уважаемые форумчане... В коде (ниже) предпринимается попытка ОДНИМ запросом найти ошибки и ограничения на исполнение некоторой бизнес-логики. Выскажетесь, как Вам такой код? Можно ли его однозначно назвать говнокодом, или только с оговорками, или называть его так совсем нельзя? Обсуждать код не очень хочется (нет времени), нужны просто либо Ваши плюсы, либо минусы.
declare @Trouble varchar(max);
select @Trouble =
    case
        when not exists
            ( -- тут ЛОКАЛЬНЫЙ селект, который НИКАК не связан с алиасами [o] и [os] ниже во-from'е
            )
        then 'Недостаточно прав для исполнения метода'
        when not exists
            ( -- тут ЛОКАЛЬНЫЙ селект, который НИКАК не связан с алиасами [o] и [os] ниже во-from'е
            )
        then 'Еще какая-то ошибка'
        when not exists
            ( -- тут ЛОКАЛЬНЫЙ селект, который НИКАК не связан с алиасами [o] и [os] ниже во-from'е
            )
        then 'Еще 100500 ошибок и ограничений, НИКАК не связанных с алиасами [o] и [os] ниже во-from''е'
        when os.Code not in 'READY' -- Тут ПЕРВОЕ упоминание алиаса [os]
        then 'Метод не доступен для документа в данном статусе'
        else null
    end
from crmOrders o
join crmOrderStatuses os on os.No = o.[Status]
6 июл 12, 19:57    [12831257]     Ответить | Цитировать Сообщить модератору
 Re: Интересует Ваше мнение о качестве кода  [new]
Mnior
Member

Откуда: Кишинёв
Сообщений: 6723
Если оно не связано, то выполнять одну и туже проверку для каждой строки бессмысленно - логическая несостыковка.
Ошибки должны проверятся при отсутствии данных (строк) в запросе?
Может стоит начать со статического набора (без FROM).

ELSE NULL - зачем этот мусор.
Куда делась схема у объектов.
6 июл 12, 20:07    [12831284]     Ответить | Цитировать Сообщить модератору
 Re: Интересует Ваше мнение о качестве кода  [new]
RubinDm
Member

Откуда: SPb
Сообщений: 458
Mnior
Ошибки должны проверятся при отсутствии данных (строк) в запросе?
Да, если строк данных нет, то ошибки должны проверяться, причем ровно в том порядке, как указано в коде.
6 июл 12, 20:17    [12831318]     Ответить | Цитировать Сообщить модератору
 Re: Интересует Ваше мнение о качестве кода  [new]
RubinDm
Member

Откуда: SPb
Сообщений: 458
Mnior
Если оно не связано, то выполнять одну и туже проверку для каждой строки бессмысленно - логическая несостыковка
мне тоже так кажется. но не все с этим согласны.
Mnior
ELSE NULL - зачем этот мусор.
У меня нет ответов. Это не мой код.
Mnior
Куда делась схема у объектов.
Т.е.? Вам запрос показать как он есть и привести схемы используемых табличек?Зачем... у меня нет цели разобрать конкретный случай. Цель - понять ценность такого "патерна" написания кода.
6 июл 12, 20:22    [12831336]     Ответить | Цитировать Сообщить модератору
 Re: Интересует Ваше мнение о качестве кода  [new]
invm
Member

Откуда: Москва
Сообщений: 9397
Подзапросы будут выполняться на каждую строку данных.
Итоговое значение @Trouble будет вычислено для последней строки в результирующем наборе.
Если данных нет, то значение @Trouble не изменится.

Mnior имеет в виду неиспользование имен объектов, квалифицированных схемой.
6 июл 12, 20:40    [12831403]     Ответить | Цитировать Сообщить модератору
 Re: Интересует Ваше мнение о качестве кода  [new]
Crimean
Member

Откуда:
Сообщений: 13148
обрабатывается строк много. а результат - 1 строка. соответственно в строке в результате будет откровенный "мусор". пример:

declare @a table ( id int )

insert into @a select 1 union all select 2 union all select 3

declare @s varchar(max)

select @s = case when id = 2 then 'ошибка' else null end
from @a

select @s


возможный обход (очень далеко не самый правильный, замечу!):

declare @a table ( id int )

insert into @a select 1 union all select 2 union all select 3

declare @s varchar(max)

select @s = case when @s is null then case when id = 2 then 'ошибка' else null end else @s end
from @a

select @s


судя по написанному, "для оптимизации" стоит "статичные" проверки выполнить до запроса - они-то как раз дадут 1 строку или null если нет ошибок, а для обработки строк уже сделать табличку:

declare @a table ( id int )

insert into @a select 1 union all select 2 union all select 3

declare @s table( id int , s varchar(max) )

insert into @s( id, s )
select id, case when id = 2 then 'ошибка' else null end
from @a

select * from @s


и уже дальше разбираться с коллекцией ошибок, к примеру так:

declare @a table ( id int )

insert into @a select 1 union all select 2 union all select 3

declare @s table( id int , s varchar(max) )

insert into @s( id, s )
select id, case when id = 2 then 'ошибка' else null end
from @a

-- select * from @s

declare @id int, @err varchar(max)
select top 1 @id = id, @err = s from @s where s is not null

if @@rowcount <> 0 raiserror( 'ошибка [%s] проверки строки [%d]', 16, 1, @err, @id )


и при последнем подходе никто не мешает и "статику" и "многострочное" - пихать в одну табличку ошибок, а для получения интегрального результата - анализировать ее целиком:

declare @a table ( id int )

insert into @a select 1 union all select 2 union all select 3

declare @s table( id int null , s varchar(max) null )

insert into @s ( id, s ) select null, 'мало прав'

insert into @s( id, s )
select id, case when id = 2 then 'ошибка' else null end
from @a

-- select * from @s

declare @id int, @err varchar(max)
select top 1 @id = id, @err = s from @s where s is not null

if @@rowcount <> 0 raiserror( 'ошибка [%s] проверки строки [%d]', 16, 1, @err, @id )
6 июл 12, 20:46    [12831428]     Ответить | Цитировать Сообщить модератору
 Re: Интересует Ваше мнение о качестве кода  [new]
RubinDm
Member

Откуда: SPb
Сообщений: 458
invm
Mnior имеет в виду неиспользование имен объектов, квалифицированных схемой.
а.. схемы.. их у нас тож не все любят указывать (пальцы отваляца-жеж все схемы набирать), пока необходимость в with schemabinding не прижимает. но на фоне остального это мелочи, я уже и не замечаю.
6 июл 12, 20:47    [12831438]     Ответить | Цитировать Сообщить модератору
 Re: Интересует Ваше мнение о качестве кода  [new]
Crimean
Member

Откуда:
Сообщений: 13148
типа попытка резюмировать
если всегда обрабатывается гарантировано 1 строка - имеет место быть. в противном случае - говнокод
6 июл 12, 20:50    [12831444]     Ответить | Цитировать Сообщить модератору
 Re: Интересует Ваше мнение о качестве кода  [new]
Mnior
Member

Откуда: Кишинёв
Сообщений: 6723
Crimean
судя по написанному, "для оптимизации" стоит "статичные" проверки выполнить до запроса
Зачем? Зачем императивщину вводить, зачем размазывать код?
Да практически любое можно написать в один запрос, притом эффективно и не заморачиваясь.

Crimean
... а для обработки строк уже сделать табличку ...
declare @a table ( id int )
insert into @a select 1 union all select 2 union all select 3

declare @s table( id int null , s varchar(max) null )
insert into @s ( id, s ) select null, 'мало прав'
insert into @s( id, s )
select id, case when id = 2 then 'ошибка' else null end
from @a

-- select * from @s

declare @id int, @err varchar(max)
select top 1 @id = id, @err = s from @s where s is not null
if @@rowcount <> 0 raiserror( 'ошибка [%s] проверки строки [%d]', 16, 1, @err, @id )
От вас Crimean не ожидал такого.
Чем вам обыкновенный CASE не устраивает? Просто не надо было его во FROM запрос вставлять.

Если нам надо в одну переменную всё зафигачить, то надо знать (по бизнес требованиям) возвращать все ошибки или нет.
Если все, то соединённый (UNION ALL) агрегат всего
Если нет, то в последнем ELSE вернуть первый попавшийся (из под-запроса).
Или как там в требованиях (если нет каких-то прав - то одну эту ошибку, если в данных - то все ошибки). Т.е. да - оправданно не смешивать совершенно разные вещи. Права проверяются заранее и заранее обрубается ветка кода, и т.п. Хотя CASE как раз и не даст считывать данные, если ошибка в правах.

Чё тут мусолить и наворачивать. RubinDm, у вас поправить пару строк - делов-то.

Crimean
если всегда обрабатывается гарантировано 1 строка - имеет место быть
Не соглашусь, логика и структура кода не должна зависеть от данных. И никаких вопросов (а сколько строк) при чтении кода не должно возникать. Этот код не плохой - он тупо неправильный.
6 июл 12, 23:58    [12832078]     Ответить | Цитировать Сообщить модератору
Все форумы / Microsoft SQL Server Ответить