Нужно ли бороться с хинтами и ворнингами

← →
Sergey Masloff (2004-08-10 20:32) [0]

Или 1423 строчки кода которые потрясли мир (ну если и не мир то меня - точно).
Пришлось тут сегодня покопаться в чужом коде. Делали для нас оутсорсеры кое-какую работу. Давно - почти 3 года назад. Все честь по чести контора серьезная контракт, ТЗ, приемка - сдача. Ну со сроками подзатянули чуток но все вроде заработало худо-бедно. Ну а тут пришлось кое-что подкрутить, да даже не подкрутить а посмотреть как одна вещь там работает и можно ли настроить под немножко другую задачу. Ну я взял исходники - думал на часик работы. И что я там увидел - это нечто. Наверное пост будет длинный - предупреждаю сразу.
Для примера возьму один из модулей (от остальных полутора десятков отличается только тем что то что меня интересовало в нем реализовано). Начало меня обрадовало - компилятор с ходу выдал 183 хинта и около 50 варнингов. Тыкнув в первый же ворнинг я увидел чудо-конструкцию №1(привожу сокращенный вариант):
procedure SomeProc(); var bOK : Boolean; begin if bOK then {Супер пупер код } else {Другой супер-пупер код} end;

беглый просмотр модуля выявил еще 3 таких же участка. Такой видимо вариант русской рулетки

Код номер №2 меня уже не удивил
procedure SomeProc(); var bOK : Boolean; begin if SomeCond then bOK := True else if SomeOtherCond then bOk := True else ..... end; {Тут ничего связанного с использованием bOK нет} end;

Таких было 6 штук

еще неплохой пример
procedure SomeProc(); var X1 : TSomeType; X2 : TOtherType; ... XN : TSomeNType; begin {Тут ничего ни одна из локальных переменных не используется} end;
такое встречалось раз 30 то есть практически в каждой процедуре

...ПРОДОЛЖЕНИЕ СЛЕДУЕТ

← →
Юрий Зотов © (2004-08-10 20:38) [1]

Если SomeCond и SomeOtherCond в коде № 2 связаны с вызовом процедур, то код все же может быть оправдан - он не зависит от используемой схемы вычисления булевских выражений. А его аналог:
bOK := SomeCond or SomeOtherCond;
от нее зависит (SomeOtherCond может вычисляться, а может и нет).

← →
Rouse_ © (2004-08-10 20:45) [2]

На дух не переношу вещей даже с хинтами, не говоря уже о ворнингах...
Код должен быть изначально верен и не допускать даже возможности возникновения ошибки (за исключением ошибок компилятора :)

> [1] Юрий Зотов © (10.08.04 20:38)
Если верить {Тут ничего связанного с использованием bOK нет} то налицо зашумленность кода, а так действительно SomeCond or SomeOtherCond

Вот такое мое ИМХО

← →
Sergey Masloff (2004-08-10 20:48) [3]

...ПРОДОЛЖЕНИЕ

Ну дальше я читал просто как комикс, например
procedure SomeProc(); var F : TSomeFormType; begin if SomeCond then F := TSomeFormType.Create(nil); {тут всякий разный код в том числе использующий F} with F do begin SomeMehod(); SomeOtherMethod(); try SomeThirdMethod(); finally Free; end; end; end;
Таких было 3 вхождения

Зато было очень много комментариев. В пяти местах встречалось
inc(i); {увеличиваем i} // Мой коммент - лучше б написали за каким ИКСОМ тут i когда нигде дальше не используется

Но больше всего мне понравился следующий комментарий (ДОСЛОВНО):
END.//КОНЕЦ {************ Конец модуля *************************}

Кстати еще одна находка - переменные цикла вне цикла использовались В КАЖДОЙ процедуре в которой был цикл. Буквально в каждой.

И напоследок - зачем я это написал. Может быть чтобы показать какой я умный? - нет, это и так все знают ;-)))) Чтобы замеряться с кем - нибудь - нет.
Просто этот код писали люди считающие себя профессиональными программистами. Работающие (надеюсь что работавшие) в серьезной конторе. Не вчерашние студенты - примерно мои ровесники, за 30.
Так вот для чего я это пишу - ребята, кто делает первые шаги или чуть больше, запомните-так писать нельзя. Это может проканать раз, может два. Это переходит в привычку - так писать. А мир программистский тесен. Я, не самый общительный в мире человек хорошо знаю коллег из минимум полутора десятков серьезных компаний. Думаю многие знают больше. Мы общаемся, и я вполне могу скажем при приеме на работу кандидата обзвонить своих знакомых - не знают ли такого. И они мне могут позвонить - и звонят. Ну и как я отрекомендую авторов такого кода? Причем люди не стемнялись, с каждом комментарии авторы чуть ли не с копирайтами.

Так что берегите честь смолоду! Вляпаться легко отмыться потом - намного сложнее. Шутка но всерьез.

С уважением

← →
nikkie © (2004-08-10 20:49) [4]

ну уже весело :))
ждем продолжения :)

← →
Sergey Masloff (2004-08-10 20:51) [5]

Rouse_ © (10.08.04 20:45) [2]
>Если верить {...
зуб даю ;-) - не исполльзуется.

← →
DiamondShark © (2004-08-10 21:11) [6]

> Юрий Зотов © (10.08.04 20:38) [1]
> Если SomeCond и SomeOtherCond в коде № 2 связаны с вызовом
> процедур, то код все же может быть оправдан - он не зависит
> от используемой схемы вычисления булевских выражений. А
> его аналог:
> bOK := SomeCond or SomeOtherCond;
> от нее зависит (SomeOtherCond может вычисляться, а может
> и нет).

Вечер трудного дня?

← →
Cobalt © (2004-08-10 21:12) [7]

А как товарищи матёрые программисты относятся к таким вот хинтам:
[Warning] Unit1.pas(584): Unsafe typecast of "Integer" to "TObject"
[Warning] Unit1.pas(616): Unsafe typecast of "TObject" to "Integer"
И код к ним:
s2:=FieldByName("CompanyName").AsString; if pos(AnsiUpperCase(s),AnsiUpperCase(s2))<>0 then ClLBox.Items.AddObject(s2,TObject(FieldByName("ID").AsInteger)); --------- CLFId:=integer(ClLBox.Items.Objects[ClLBox.ItemIndex]);

← →
jack128 © (2004-08-10 21:14) [8]

не знаю как матерые, а я бы отключил в семерке предепреждения об unsafe коде ;-)

← →
имя (2004-08-10 21:20) [9]

Удалено модератором

← →
имя (2004-08-10 21:20) [10]

Удалено модератором

← →
имя (2004-08-10 21:20) [11]

Удалено модератором

← →
имя (2004-08-10 21:20) [12]

Удалено модератором

← →
имя (2004-08-10 21:20) [13]

Удалено модератором

← →
имя (2004-08-10 21:20) [14]

Удалено модератором

← →
имя (2004-08-10 21:20) [15]

Удалено модератором

← →
Юрий Зотов © (2004-08-10 21:23) [16]

> DiamondShark © (10.08.04 21:11) [6]
У Вас?

> jack128 © (10.08.04 21:14) [8]
А кто мешает?

← →
jack128 © (2004-08-10 21:27) [17]

> > jack128 © (10.08.04 21:14) [8]
> А кто мешает?

Мне? То что я на пятёрке сижу ;-) А вот что Кобальту мешает - не знаю..

← →
Cobalt © (2004-08-10 21:34) [18]

2 Юрий Зотов © (10.08.04 21:23) [16]
То есть, вы хотите сказать, что этот хинт указывает на возможные проблемы при переносе на .Net ?
И при отсутствии таких перспектив можно не обращать внимание на него и отключить?

← →
имя (2004-08-10 21:41) [19]

Удалено модератором

← →
имя (2004-08-10 21:41) [20]

Удалено модератором

← →
имя (2004-08-10 21:41) [21]

Удалено модератором

← →
Юрий Зотов © (2004-08-10 21:59) [22]

> Cobalt © (10.08.04 21:34) [18]

Я так и сделал. А заодно отключил и предупреждения о платформенной зависимости. Потому что проект изначально писался, как чисто виндовый, никаких дотнетов и кроссплатформенности в нем даже и в перспективе не появится. Иначе проще будет новый написать.

← →
GuAV © (2004-08-10 22:02) [23]

> не знаю как матерые, а я бы отключил в семерке предепреждения
> об unsafe коде ;-)

Ага среди них правильных ворнингов не видно - этих слишком много :)
У себя их гашу даже не директивами а опциями.

+ platform - эти на кой?

← →
Piter © (2004-08-10 22:09) [24]

Sergey Masloff (10.08.04 20:48) [3]
Кстати еще одна находка - переменные цикла вне цикла использовались В КАЖДОЙ процедуре в которой был цикл. Буквально в каждой

ну я тоже так делаю. Если цикл уже отработал и локальная абстрактная переменная i уже не нужна - почему бы ее не задействовать? не объявлять же новую (можно, но зачем?).

А вообще - ну подумаешь что такое есть. Проект ведь компилировался с оптимизацией, значит все что объявлено, но не использовано - было выброшено компилятором. И получающийся код не такой уж и плохой.

А так ты же сам говорил про сроки, небось спешили, просто забыли убрать объявления переменных (может хотели по другому реализовать, но в спешке не стали).

Ну это я так. Склонен сегодня к адвокатству

Cobalt © (10.08.04 21:34) [18]
То есть, вы хотите сказать, что этот хинт указывает на возможные проблемы при переносе на .Net ?

это вообще может породить проблемы. Сам подумай - число приводишь к TObject - ясно, что это небезопасно. Компилятор же не может понять, что ты используешь ListBox для хранения строки и просто приписанного к нему числа...

← →
Piter © (2004-08-10 22:10) [25]

GuAV © (10.08.04 22:02) [23]
Ага среди них правильных ворнингов не видно - этих слишком много :)

А у меня семерка по умолчанию никогда и не выводила warning на Unsafe код

← →
Anatoly Podgoretsky © (2004-08-10 22:14) [26]

Злой ты человек, думал спать буду спокойно, но после такого не получится :-)

← →
Юрий Зотов © (2004-08-10 22:19) [27]

> Piter © (10.08.04 22:09) [24]

> Если цикл уже отработал и локальная абстрактная переменная i
> уже не нужна - почему бы ее не задействовать? не объявлять же
> новую (можно, но зачем?).

Используя счетчик цикла вне цикла, Вы заставляете компилятор разместить его обязательно в памяти. А без этого он мог бы разместить его в регистре.

← →
Юрий Зотов © (2004-08-10 22:25) [28]

Поправка к [27].

Хотя надо смотреть ассемблерный код - не исключено, что компилятор достаточно умен и все равно строит цикл по регистру.

> Кстати еще одна находка - переменные цикла вне цикла использовались
> В КАЖДОЙ процедуре в которой был цикл.
Я что-то никак раздуплить не могу :) Приведи код, а то не понятно что кого и в каких местах пользует :))

> Rouse_ © (10.08.04 22:31) [29]

Насколько я понял, имеется в виду вот что:

for I := ... to ... do
begin
...
end;
I := ...
... // дальше работаем с I

> > Кстати еще одна находка - переменные цикла вне цикла использовались
>
> > В КАЖДОЙ процедуре в которой был цикл.
все таки я пока не понимаю всей "позорности" такой практики.. ЮЗ [27] - это не причина, ИМХО.

← →
Sergey Masloff (2004-08-10 22:41) [32]

Юрий Зотов © (10.08.04 22:34) [30]
Несколько хуже
for I := ... to ... do begin ... end; I := ... <<== ЭТОГО не было. То есть именно работа с тем что там после цикла осталось ... // дальше работаем с I

> А у меня семерка по умолчанию никогда и не выводила warning
> на Unsafe код

Она выводит или новый проэкт создать или старый открывать а в другом случае они октл, когда точно что - не помню.

2 Cobalt © (10.08.04 21:12) [7]

Нормально отношусь. Ибо по существу. Особенно в предверии перехода к дотнету. Если же в коллекции вместо указателя ставим сознательно какой-то целочисельный идентификатор - что впрочем, есть вполне нормальной практикой в мире win32 - сознательно отключаем сотв. варнинг. Это один из тех немногих случаев, когда соотв. варнинги есть смысл отключать.

> Sergey Masloff (10.08.04 22:41) [32]

Да-а-а... Слов нет, одни буквы. И это работало?

← →
Sergey Masloff (2004-08-10 22:52) [36]

Юрий Зотов © (10.08.04 22:47) [35]
>И это работало?
Ну вобщем, работало. Ну эпизодически происходили всякие побочные эффекты. Типа при запуске на одних и тех же данных то отрабатывало то нет, то обращение к памяти по 0 адресу но вобщем приемный акт им подписали да и использовалась программа все это время. С сегодняшнего дня правда не используется.

> [32] Sergey Masloff (10.08.04 22:41)
Хм, даю пример:

for I := 0 to Len - 1 do
if Buff[I] = Условие then Break;
if I < Len then (Выполняем операции с Buff[I])

Довольно нормальный код, или я не так понял и все намного запущеней?

ЗЫ: Я сам часто использую повторно переменные типа I после их повторной инициализации и только потому, что не вижу смысла создавать кучу временных переменных...

> Sergey Masloff (10.08.04 22:52) [36]

> при запуске на одних и тех же данных то отрабатывало то нет,
> то обращение к памяти по 0 адресу

Неудивительно. Только я бы не называл это "работало".

> вобщем приемный акт им подписали

IMHO, поторопились. Стоило сначала посмотреть код.

> Rouse_ © (10.08.04 22:54) [37]

for I := 0 to Len - 1 do
if Buff[I] = Условие then Break;
if I < Len then (Выполняем операции с Buff[I]);

Если компилятор построит цикл по регистру, значение I после выхода из цикла может быть любым.

> Если компилятор построит цикл по регистру, значение I после
> выхода из цикла может быть любым.
В смысле ты имеешь ввиду если цикл пройдет до конца и условие не выполнится ни одного раза (не сработает Break)?

Нужно ли бороться с хинтами и ворнингами - 2 Найти похожие ветки