web4_0 Sep 20 2010 at 09:33

Тестовое задание

2 min

1.6K

Lumber room

Comments 24

LeeMiller Sep 20 2010 at 09:45

Обратитесь с этим вопрос в Q&A

dohlik Sep 20 2010 at 09:45

У ТРУшного (целевого) программиста при взгляде на такой код должны встать дыбом волосы и по телу мурашки пройтись. Так их и вычисляют. Всем, кто пытается код рефакторить — отказ. :)

web4_0 Sep 20 2010 at 09:49

Что-то вроде «При виде нашего тестового задания программист упал в обморок. Пришлось откачивать Бандой Четырёх и Бучем»? :)

kirsan_vlz Sep 20 2010 at 09:49

Можно распечатать и перед сном вместо ужастиков читать =)
Автор, разберись, что делает функция, и перепиши с нуля. Другого варианта рефакторинга тут нет.

IvanT Sep 20 2010 at 09:47

Отличное тестовое задание. Нормальный спец просто переписал бы всё нафиг с нуля, а недокодер попытался бы исправить. В любом случае по результату было бы хорошо видно как мыслит человек и сколько у него опыта.

web4_0 Sep 20 2010 at 09:52

Кстати, как минимум, человек должен будет сказать, что под 5.3 код не запустится, и поэтому пусть дают ему нормальное тестовое задание, которое можно запустить под всеми версяими PHP

halyavin Sep 20 2010 at 11:10

Это если известно, что код должен делать. В противном случае нормальный спец отрефакторил бы код в процессе его понимания. После чего переписывание может и не понадобится (не знаком с php, потому не знаю какова ситуация в этом случае). Поэтому я бы советовал автору создать svn-репозиторий и коммитить туда после каждого рефакторинга. В случае переписывания функции с нуля, следует в одном коммите добавить комментарий с описанием того, что делает функция, а в следующем уже полностью заменить ее тело. Результат послать нанимателям. Это даст им на много больше информации, чем несколько лаконичных строчек кода, которые окажутся в итоговой программе.

LoneCat Sep 20 2010 at 11:17

Ну конкретно этот код так просто не отрефакторишь, так как он вместо/помимо параметров использует переменные из глобальной области видимости, причем три на вход, и одну на выход, соответственно если это переписывать через руки — придется изменить не только код функции, но и те места где она вызывается.

halyavin Sep 20 2010 at 11:53

Я бы написал об этом в комментариях к коммиту. По моему опыту, изменение способа вызова функции в сотне мест вполне решаемая за час-два задача.

LoneCat Sep 20 2010 at 12:19

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

$PERIOD_FROM = '01.01.1970';
$PERIOD_TO = '19.01.2038';

// где-нибудь там
function somefunction() {
	// эта функция и понятия не имеет о том что существуют глобальные переменные $PERIOD_FROM, $PERIOD_TO и $TEXT
	choose_period();
}

// десять тысяч строк спустя
echo $TEXT;

Потому чтобы получить доступ к этим значениям — придется объявление глобальных переменных вынести на уровень выше, а-ля:

$PERIOD_FROM = '01.01.1970';
$PERIOD_TO = '19.01.2038';

function somefunction() {
	global $PERIOD_FROM, $PERIOD_TO, $TEXT; 
	$TEXT = choose_period($PERIOD_FROM, $PERIOD_TO);
}

echo $TEXT;

Получается шило на мыло, от глобальных переменных в итоге не избавились (для этого придется перековырять весь код где они используются), да и к тому-же есть шанс столкнуться с тем что названные таким образом переменные уже используются в somefunction(), например $TEXT — вполне себе безымянная переменная.

Wott Sep 20 2010 at 18:07

В данном кокретном случае надо r/choose_period/choose_period_new/ а потом r/somefunction/choose_period/
И отдельно рефакторить функции и глобальные переменные.

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

LoneCat Sep 20 2010 at 18:08

Ну так а я не про это разве писал? :)

Wott Sep 20 2010 at 19:24

Я хотел сказать что можно сделать

function choose_period() {
	global $PERIOD_FROM, $PERIOD_TO, $TEXT; 
	$TEXT = choose_period_new($PERIOD_FROM, $PERIOD_TO);
}

а уже потом отдельно решать проблему глобальных переменных. После чего удалить этот костыль.

web4_0 Sep 20 2010 at 18:13

Проблема в том, что часто такая логика пронизывает весь проект. Может, пересмотреть и переписать её возможность есть, но переписать значительный кусок проекта, где встречается такая логика — нет ни времени, ни возможности — бизнес не даст.

Вот и тянется сей «балласт» многие годы. Иногда, правда, можно заменять старый код на новый по частям, но далеко не всегда можно обеспечить такую «обратную совместимость».

Wott Sep 20 2010 at 19:20

В таком случае не надо делать рефакторинг just for fun. Для него нужно иметь четкую цель — упростить развитие или расширение, решить глобальную проблему типа производительности. И эта цель, обоснованная требованиями бизнеса и найдет и время и возможности. А нет — значить и не надо. Красота кода, сама по себе денег не приносит. Работает, кушать не просит — и нехай пусть живет. Переписывать можно и кусочками в качестве решения проблем, но обойдется это дороже, хоть и размазано по времени.

Это все конечно не про открытые проекты для себя :)

LoneCat Sep 20 2010 at 10:09

Ну вот каждому соискателю раздашь по одной функции — глядишь рефакторинг и готов :) Хуже-то точно не напишут :)

web4_0 Sep 20 2010 at 11:21

Бывает пишут и хуже (если не брать в расчёт вышеприведённый код).
Пишут как просто плохо, так и ООП ради ООП.

Идея раздать по одной функции, конечно, хороша, но т.к. у каждого соискателя свой стиль, свои правила оформления, да если ещё и кода плохого мегабайт 5-10… — потом будет ад в поддержке.