Competition “Laugh or cry?”

July 19, 2007 | development

Preamble

One day I came to my workplace little bit earlier than I usually do and I decided to review the sources of my current project. As the result of that small research a new note has been written about the interesting piece of code. After reading it, my friend found a similar "creations" in his current project. So the idea of the Worst Code Competition was born.

The Rules

The Competition rules are pretty simple: all you need is just to find a bad code in your current project (you can use the version control system, if you have already refactored it), send me an e-mail with programming language, code and, if possible, how it could be refactored. You can post a code examples directly in the comments (don’t forget to wrap code into block <code lang="language">some code</code>). All examples which have a code smell will be published in the section Competition Participants(if you want to publish your example as anonymous, please note about this in a letter).

For example,

Language: Java

Code:

1
2
3
4
5
6
7
8
9

if (value) {
for (int i = 0; i < list.size(); i++) {
((Column)list.get(i)).visible = false;
}
} else {
for (int i = 0; i < list.size(); i++) {
((Column)list.get(i)).visible = true;
}
}

Refactoring:

1
2
3

for (int i = 0; i < list.size(); i++) {
((Column)list.get(i)).visible = !value;
}

Another example you can find in my previous post.

Code samples will be accepted during three weeks (July 19 — August 9). If you can not (or do not want to) publish your example — please, leave your comments about the creations of others, your opinion will be taken into account when the finalists will be determinated. At the end of the third week five finalists will be selected from all participating examples and everyone will be able to vote for the most worse code during a week (August 10 — August 16).

So, we have three weeks for collecting code, one week for voting and what at the end? The most horrific, unimaginable macabre and frightening piece of code will get…

The Prize

What is a competition without prizes? Well, the winner will get the original McConnell’s book "Code Complete", or it’s Russian translation "Совершенный код". When the winner will determined(August 17), I will contact with him by email to clarify the delivery address and will order a book from Amazon or books.ru directly to his home.

Why this need?

Every day we are finding examples of the code with smell. For example, a classic copy-paste pattern, the talented conditional operators (which sometimes the author could not understand), simple fun errors of refactoring, which store Person at the table products. Everyone in programming practice faced and will face them in the future. So, stop sufferring, show people what the hell is a smell code and win the prize!

Competition Participants

Author: COTOHA
Language: C#

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16

if(Read)
{
for (int index = 0; index < messageInfo.Count; index++)
{
messageInfo[index].Status = false;
messageInfoDataSource.Update(messageInfo[index]);
}
}
else
{
for (int index = 0; index < messageInfo.Count; index++)
{
messageInfo[index].Status = true;
messageInfoDataSource.Update(messageInfo[index]);
}
}

Refactoring:

1
2
3
4
5

for (int index = 0; index < messageInfo.Count; index++)
{
messageInfo[index].Status = !Read;
messageInfoDataSource.Update(messageInfo[index]);
}

Comments (70)

LeKz says:

2007-07-26 at 10.10 am

Language: PHP

А как вам это?

// GET NEWS DATA ///////////////////////////////////////////////////////
$newsArray = unpk(file_get_contents("system/cache/news.dat"));
if (!is_array($newsArray)) $newsArray = array();

foreach ($newsArray as $key => $newsEntity) {

if ($newsEntity["date"] == $_GET["id"]) {

$news = $newsEntity;

$newsKey = $key;
break;
}
}

if (isset($_GET["id_n"])&&$_GET["id_n"]==$news["id_n"]&&empty($newsArray[$key]["userfile"]))

{

$tpl -> AssignArray(array(

"title" => $newsArray[$key]["title"],
"body" => $newsArray[$key]["body"],
"image" => "",
"del" => "",
"info" => "Текущая картинка:"

));

if ($handle = fopen("system/cache/news.dat", "w")) {
fwrite($handle, pk($newsArray));
fclose($handle);
}

}
else if(isset($_GET["id_n"])&&$_GET["id_n"]==$news["id_n"]&&!empty($newsArray[$key]["userfile"]))
{

$tpl -> AssignArray(array(

"title" => $newsArray[$key]["title"],
"body" => $newsArray[$key]["body"],
"userfile" => $newsArray[$key]["userfile"],
"id_n" => $newsArray[$key]["id_n"],
"rm_pic" => $newsArray[$key]["rm_pic"],
"image" => "<IMG SRC = '{userfile}' />",
"del" => "Удалить",
"info" => "Текущая картинка:"

));
}

// HANDLE REMOVE REQUEST ///////////////////////////////////////////

if (isset($_GET["rm_pic"])&&$_GET["rm_pic"]==$newsArray[$key]["rm_pic"]&&!empty($newsArray[$key]["userfile"])) {

unlink($newsArray[$key]["userfile"]);

$newsArray[$key]["userfile"]="";

$tpl -> AssignArray(array(

"title" => $newsArray[$key]["title"],
"body" => $newsArray[$key]["body"],
"userfile" => "",
"id_n" => $newsArray[$key]["id_n"],
"image" => "",
"del" => "",
"info" => ""

));

if ($handle = fopen("system/cache/news.dat", "w")) {
fwrite($handle, pk($newsArray));
fclose($handle);
}

}
else
{

$tpl -> AssignArray(array(

"title" => $newsArray[$key]["title"],
"body" => $newsArray[$key]["body"],
"userfile" => $newsArray[$key]["userfile"],
"id_n" => $newsArray[$key]["id_n"],
"rm_pic" => $newsArray[$key]["rm_pic"],
"image" => "<IMG SRC = '{userfile}' />",
"del" => "Удалить",
"info" => ""

));

if ($handle = fopen("system/cache/news.dat", "w")) {
fwrite($handle, pk($newsArray));
fclose($handle);
}

}

// HANDLE POST DATA /////////////////////////////////////////////////////
if (isset($_POST["Submit"], $_POST["title"], $_POST["body"])) {

$path ="";

if(is_uploaded_file($_FILES["userfile"]["tmp_name"])){

$path = "system/cache/images/news/".time().$_FILES["userfile"]["name"];

move_uploaded_file($_FILES["userfile"]["tmp_name"], $path);

if(!empty($newsArray[$key]["userfile"])){

$path_old = $newsArray[$key]["userfile"];

unlink($path_old);

}

$newsArray[$key] = array(
"title" => $_POST["title"],
"date" => date("U"),
"body" => $_POST["body"],
"userfile" => $path,
"id_n" => $_GET["id_n"],
"id_n" => $_POST["id_n"],
"rm_pic" => $newsArray[$key]["rm_pic"]
);

if ($handle = fopen("system/cache/news.dat", "w")) {
fwrite($handle, pk($newsArray));
fclose($handle);
$tpl -> Zone("save", "success");

_fnc("reload", 3, "?L=admin.news.news");

}
}
else
{

$path_old = $newsArray[$key]["userfile"];

$tpl -> Zone("save1", "error");

$newsArray[$key] = array(
"title" => $_POST["title"],
"date" => date("U"),
"body" => $_POST["body"],
"userfile" => $path_old,
"id_n" => $_POST["id_n"],
"rm_pic" => $newsArray[$key]["rm_pic"]

);

if ($handle = fopen("system/cache/news.dat", "w")) {
fwrite($handle, pk($newsArray));
fclose($handle);
$tpl -> Zone("save", "success");
_fnc("reload", 3, "?L=admin.news.news");
}

else $tpl -> Zone("save", "error");

}
}

else $tpl -> Zone("edit", "enabled");

COTOHA says:

2007-07-26 at 11.30 am

как обычно. начитаются книжек а-ля “выучи пхп за 21 день” и кодят потом в таком стиле. хорошо хоть темплейты юзает… правда неправильно гыгы

не знаю… имхо это не для этого конкурса у чувака просто неправильное воспитание. а вот из цикла по эксепшену выходить – это допереть надо! это мозк!
LeKz says:

2007-07-26 at 4.40 pm

Гыыы… 21 день – точно
За 4 года ИСПР не дал человеку должного воспитания )))

Alexey Kovyrin says:

2007-07-31 at 12.03 am

So, if html code is eligible to participate in this competition, then here are my 2 cents. The code presented below I’ve found on EVERY page of some client’s site. In addition all this stuff is being hosted on apache server (prefork) with KeepAlive Off.

Language: HTML

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319

Dmytro Shteflyuk says:

2007-08-05 at 2.50 am

1
2
3
4
5
6
7

protected bool DisplayCheck(object display)
{

if (display.ToString() == "True")
return false;
else return true;
}

Передается в метод булева переменная, вытащенная из базы (отсюда и object).

COTOHA says:

2007-08-06 at 2.13 pm

логично.

Dmytro Shteflyuk says:

2007-08-06 at 6.16 pm

Трепещите, уважаемые. Перед вами функция для проверки валидности даты. Параметры – id DOM-элементов (input для year и day, select для month, span для error). Честно говоря, такое я еще не видел…

//

Dmytro Shteflyuk says:

2007-08-06 at 9.19 pm

Да, для тех, кто не понял с ходу алгоритм, рассказываю. Лет в пять (а может и раньше) бабушка научила меня быстрому вычислению количества дней в месяце. Объясню что называется “на пальцах”.

Нужно повернуть левую руку ладонью вниз и сжать в кулак. Правой рукой начинаем считать костящки и впадины между ними: январь, февраль, март, апрель, май, июнь, июль (всего 7 месяцев). Костяшкам соответствует месяц длиной 31 день, впадинкам – 30 (кроме февраля, но об этом позже). Теперь берем правую руку и считаем левой: август, сентябрь, октябрь, ноябрь, декабрь. Алгоритм тот же – костяшки 31, впадинки 30. Реализацию 1:1 наблюдаем в алгоритме – ветки if (month< =7) и if (month>7).

Осталось разобраться с февралем. Тут уже все сложнее: если год делится на 4, но не делится на 100, – год високосный, т.е. 29 дней. Иначе 28. Как видим, в алгоритме упрощенная версия проверки, именно так мне рассказывала бабушка, дабы не нагружать неокрепший детский моск.

Как видим, автор либо не вырос из пятилетнего возраста, либо его моск кто-то съел. За демонстрацию столь сильной подкованности в плане алгоритмов и вообще соображалки требую себе книжку
COTOHA says:

2007-08-08 at 3.17 pm

тююююю… слабая попытка втесаться в группу финалистов. ведь если подумать то алгоритм-то отличный а ты что предлагаешь? заменить полёт мысли тупым знанием функций? фууууу!

в номинации “не знал, но выкрутился” имхо всё равно нет равных мега-циклу с выходом по ексепшену… ну и брейк канечно рулит.

в номинацию “копипаст” (я б её вобще упразднил) не канает – это после монстров копипаста типа генератора табличных данных от рейджа.
erka says:

2007-08-08 at 10.08 pm

Порой я начинаю задумываться, кто из вас двоих больше критики может выдать . А мне лично “понравился” код и история. Кстати, никогда такой не слышал
COTOHA says:

2007-08-09 at 11.53 am

ого. тяжолое детсво я до сих пор так кол-во дней в месяце проверяю а почему понравился в кавычках? если по отношению к коду я ещё кавычки пойму, то к истории…

так. ну а критики конечно выдать могу больше я. можем проверить, если как-то формализовать правила проверки… но учти, что я их раскритикую!

erka says:

2007-08-09 at 1.48 pm

Вот вершина мастерства.. метод напрямую вызывается через web services. Интересно, какая вероятность того, что я это сломаю?

Language: Java

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34

public Collection searchFreePID(String sql, String[] fields, String tableName, SearchFilter searchFilter) {
logger.debug("#searchFreePID:enter");
Collection result = null;
try {
if (tableName != null) {
Pattern p = Pattern.compile("\\s*(SELECT\\s+)(.+)", Pattern.CASE_INSENSITIVE);
Matcher m = p.matcher(sql);
if (m.find()) {
sql = m.group(1) + OBLIGATORY_FIELDS + m.group(2);
StringBuffer buffer = new StringBuffer();
buffer.append(sql);
if (sql.indexOf("where") < 1) {
buffer.append(" WHERE ");
}
buffer.append(" pid <> oid and pid = ");
if (searchFilter.getPid() != null) {
buffer.append(searchFilter.getPid());
} else {
buffer.append("-1");
}
sql = buffer.toString();
logger.debug("#sql = [" + sql + "]");
result = gFactory.selectFreeAll(sql, fields);
}
}
} catch (Exception e) {
logger.error("Exception in #searchAll", e);
}
if (logger.isDebugEnabled()) {
logger.debug("Result = [" + result + "]");
}
logger.debug("#searchFreePID:exit");
return result;
}

COTOHA says:

2007-08-10 at 7.22 pm

такое я тоже люблю:

1
2
3
if (logger.isDebugEnabled()) {
logger.debug("Result = [" + result + "]");
}

повбивав би.
Dmytro Shteflyuk says:

2007-08-14 at 2.38 pm

Собственно и где же голосование? Кажись провтыкали
COTOHA says:

2007-08-14 at 4.34 pm

At the end of the third week five finalists will be selected from all participating examples and everyone will be able to vote for the most worse code during a week (August 10 — August 16).
обман? ерка….

Alexey Kovyrin says:

2007-08-15 at 11.54 pm

While optimizing some customer application I’ve found following SQL query:

(SELECT
country_code, region_name, towncity_name, venue_name, ticketing_number,
shows.show_id, date, commission_id, locked, bouts.user, inserter,
bouts.bout_id, rounds_scheduled, rounds_boxed, decision, bouts.division , result, time,
boxer_a_id,
boxer_a_weight,
boxer_a.dob AS boxer_a_dob,
boxer_a.last_name AS boxer_a_last_name,
boxer_a.first_name AS boxer_a_first_name,
boxer_b_id,
boxer_b_weight,
boxer_b.dob AS boxer_b_dob,
boxer_b.last_name AS boxer_b_last_name,
boxer_b.first_name AS boxer_b_first_name,
judge1,
jdg1_pts_a,
jdg1_pts_b,
judge1.last_name AS judge1_last_name,
judge1.first_name AS judge1_first_name,
judge2,
jdg2_pts_a,
jdg2_pts_b,
judge2.last_name AS judge2_last_name,
judge2.first_name AS judge2_first_name,
judge3,
jdg3_pts_a,
jdg3_pts_b,
judge3.last_name AS judge3_last_name,
judge3.first_name AS judge3_first_name,
referee,
ref_pts_a,
ref_pts_b,
referee.last_name AS referee_last_name,
referee.first_name AS referee_first_name
FROM bouts
JOIN shows ON bouts.show_id = shows.show_id
JOIN venues ON shows.venue_id = venues.venue_id
JOIN towncities ON venues.towncity_id = towncities.towncity_id
JOIN humans AS boxer_a ON boxer_a.human_id = bouts.boxer_a_id
JOIN humans AS boxer_b ON boxer_b.human_id = bouts.boxer_b_id
JOIN humans AS referee ON referee.human_id = bouts.referee
JOIN humans AS judge1 ON judge1.human_id = bouts.judge1
JOIN humans AS judge2 ON judge2.human_id = bouts.judge2
JOIN humans AS judge3 ON judge3.human_id = bouts.judge3
WHERE
(boxer_b_id = '001616') AND result = 'SC') UNION (SELECT
country_code, region_name, towncity_name, venue_name, ticketing_number,
shows.show_id, date, commission_id, locked, bouts.user, inserter,
bouts.bout_id, rounds_scheduled, rounds_boxed, decision, bouts.division , result, time,
boxer_a_id,
boxer_a_weight,
boxer_a.dob AS boxer_a_dob,
boxer_a.last_name AS boxer_a_last_name,
boxer_a.first_name AS boxer_a_first_name,
boxer_b_id,
boxer_b_weight,
boxer_b.dob AS boxer_b_dob,
boxer_b.last_name AS boxer_b_last_name,
boxer_b.first_name AS boxer_b_first_name,
judge1,
jdg1_pts_a,
jdg1_pts_b,
judge1.last_name AS judge1_last_name,
judge1.first_name AS judge1_first_name,
judge2,
jdg2_pts_a,
jdg2_pts_b,
judge2.last_name AS judge2_last_name,
judge2.first_name AS judge2_first_name,
judge3,
jdg3_pts_a,
jdg3_pts_b,
judge3.last_name AS judge3_last_name,
judge3.first_name AS judge3_first_name,
referee,
ref_pts_a,
ref_pts_b,
referee.last_name AS referee_last_name,
referee.first_name AS referee_first_name
FROM bouts
JOIN shows ON bouts.show_id = shows.show_id
JOIN venues ON shows.venue_id = venues.venue_id
JOIN towncities ON venues.towncity_id = towncities.towncity_id
JOIN humans AS boxer_a ON boxer_a.human_id = bouts.boxer_a_id
JOIN humans AS boxer_b ON boxer_b.human_id = bouts.boxer_b_id
JOIN humans AS referee ON referee.human_id = bouts.referee
JOIN humans AS judge1 ON judge1.human_id = bouts.judge1
JOIN humans AS judge2 ON judge2.human_id = bouts.judge2
JOIN humans AS judge3 ON judge3.human_id = bouts.judge3
WHERE
(boxer_a_id = '001616') AND result = 'SC') ORDER BY date DESC;

erka says:

2007-08-16 at 12.11 am

Oooo… I have never seen a sql query like posted one.
COTOHA says:

2007-08-16 at 9.17 am

осталось только показать как этот SQL оптимизируется… потому как если нет, то что тут такого? просто бокс – сложный спорт и требует сложных SQL’ей.
COTOHA says:

2007-08-18 at 11.14 pm

флудя в блоге ты, эрка, не остановишь волну народного гнева. обманутый народ, он, знаешь ли, беспощаден…
exitTeam says:

2007-10-16 at 11.30 pm

Hi,
As for me the best one is Dmytro Shteflyuk’s example with date validation.
Btw, small addition to the leap year validation, it should be checked if it is divisible to 4, and not divisible to 100, but, if it is divisible to 400 then it is a leap year too

also some small todays example:

1
ini_set('session.timeout', 28800);

hope you will like it

Erka's blog