примеры бесполезного/код нежелательной и легко исправляет

Question

Недавно я наткнулся на некоторый код, поданном в следующем

} catch (FooException e) { 
//do something 
} catch (BarException e) { 
//do something 
} catch (Exception e) { 
    throw e; 
} 

, который легко переписан в

} catch (FooException e) { 
//do something 
} catch (BarException e) { 
//do something 
} 

и

if (!(flag == false)) 

, который легко переписать как

if (flag) 

Каковы некоторые из наиболее распространенных примеров нежелательных кодов и более простой/четкий/менее подробный код замены.

Answer 1

if (x == 1) 
    return true; 
else 
    return false; 

Rewrite:

return (x == 1); 

Answer 2

http://thedailywtf.com/ имеет множество примеров плохого кода, подобного этому.

Answer 3

Ваши примеры делать разные вещи.

} catch (Exception e) { 
    throw e; 
} 

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

Answer 4

Взгляните на все правила, в которых используются инструменты анализа статического кода, такие как PMD и Findbugs.

Answer 5

троичный оператор

dim str 
if(predicate) 
    str = "a" 
else 
    str = "b" 
end 

Rewrite

String str = iff(predicate, "a", "b") 

Answer 6

инициализации со значением, когда в случае нулевой по умолчанию используется вместо этого.

Foo x = null;  
if(something != null) { 
    x = something; 
} else { 
    x = default; 
} 

=>

Foo x = something != null ? something : default; 

В языках, где если является experssion:

Foo x = if(something != null) something else default; 

Answer 7

(bColour)?bColour=false:bColour=true; //could have done (i%2!=0)?bColour=true:bColour=false; 

вместо:

bColour = !bColour; //could have done bColour=(i%2!=0); 

Да, комментарий об исходном беспорядке был фактически в коде, поэтому я тоже исправил его.

Answer 8

Старомодные манипуляции с строкой типа C, похоже, являются bugaboo для многих людей.

У меня есть два здесь, в моем буфере emacs.Пример 1:

strcpy(label, "\0"); 
strcpy(comm1, "\0"); //!!!20060818 Added protective code for missing nulls 
strcpy(comm2, "\0"); 
strcpy(dir, "\0"); 

В основном это вызов подпрограммы со всеми сопутствующими накладными расходами просто сделать:

label[0] = '\0'; 

Вот еще более причудливый вариант позже в коде:

strncpy(inq.szComment, "\0", 1); 

Опять же, это очень сложный и медленный способ присвоить нуль-символу '\ 0' первому элементу inq.szComment.

Пример 2:

// This is always good for string operations, but may not be necessary. 
strcat(RpRecPrefix, "\0"); 
strcat(RpCircPrefix, "\0"); 
strcat(RpEventPrefix, "\0"); 
strcat(RpFdk, "\0"); 
strcat(RpAic, "\0"); 
strcat(RpDcl, "\0"); 
strcat(RpVis, "\0"); 
strcat(RpSnd, "\0"); 
strcat(RpL3Fsi, "\0"); 
strcat(RpStartState, "\0"); 
strcat(RpContRun, "\0"); 

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

OK. Вот мой личный фаворит.

if (*pnFrequency == 1) { 
     sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at 1 Hz", 
       *pliWriteRate); 
    } 
    else if (*pnFrequency == 2) { 
     sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at 2 Hz", 
       *pliWriteRate); 
    } 
    else if (*pnFrequency == 4) { 
     sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at 4 Hz", 
       *pliWriteRate); 
    } 
    else if (*pnFrequency == 10) { 
     sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at 10 Hz", 
       *pliWriteRate); 
    } 
    else if (*pnFrequency == 20) { 
     sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at 20 Hz", 
       *pliWriteRate); 
    } 
    else if (*pnFrequency == 40) { 
     sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at 40 Hz", 
       *pliWriteRate); 
    } 
    else if (*pnFrequency == 60) { 
     sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at 60 Hz", 
       *pliWriteRate); 
    } 
    else { // above frequencies used for analysis 
     sprintf(szLogMsg, "Write to disk speed = %d bytes/sec", *pliWriteRate); 
    } 

Обратите внимание, что значение * pnFrequency мы проверяем, в каждой отрасли идентично значению жестко закодированной в ее Sprintf, и это единственное различие в любой и каждой отрасли. Все это должно было быть:

sprintf(szLogMsg, "Write to disk speed = %d bytes/sec at %d Hz", *pliWriteRate, 
     *pnFrequency); 

Я просто не могу себе представить, как этот файл должен быть длиной 12000 строк ...

Answer 9

List<String> list = new ArrayList<String>(); 
list.add("Foo"); 
list.add("Bar"); 

=>

List<String> list = Arrays.asList("Foo", "Bar"); 

В список дел которого был бы неизменным. Если вы хотите, чтобы достичь того же с изменяемым списка, создать класс Util:

public CollectionUtils { 
    public static List<T> createMutableList(T ... elements) { 
    List<T> list = new ArrayList<T>(); 

    for(T elem : elements) list.add(elem); 

    return list; 
    } 
} 

И тогда вы можете использовать его:

import static CollectionUtils.createMutableList;  
List<String> list = createMutableList("Foo", "Bar"); 

Answer 10

Перед Linq:

for each listItem1 in list1 
    for each listItem2 in list2 
    if listItem1.value1 = listItem2.value2 
    listSimilarItems.add(listItem1) 
    end 
end 

После Linq :

listSimilarItems = (from items in list1 where list1.value1 = list2.value2).tolist 

Answer 11

На веб-странице p rogramming, то я часто вижу использование правил CSS, как следующее:

<div class="red">Error message!</div> 

С red стиль определяется как:

red { color: #f00; } 

Ну, это глупо.А потом, когда они решили, что сообщение об ошибке должно также иметь желтый фон вы видите это:

<div class="red yellowback">Error message 2.0!</div> 

Или еще хуже:

<div class="yellowback"> 
    <h1 class="red">Error message 3.0!</h1> 
</div> 

Правильный способ использовать CSS, чтобы построить свой HTML в сначала семантически правильно, а затем идентифицировать каждую часть либо с именами 'id', либо с 'class', чтобы вы могли настроить таргетинг на каждый раздел и применить соответствующие правила. Например:

<div id="errormessages">Error message!</div> 

Вместо <div> вы могли бы сделать его пункт, так что Стенды даже без стилей (например, при использовании текстового браузера):

<p id="errormessages">Error message!</p> 

И правил CSS :

#errormessages { 
    color: #f00; 
    background-color: #ff0; 
}