7

我经常看到的最严重的冗余代码结构涉及使用代码序列

if (condition)
    return true;
else
    return false;

而不是简单地写

return (condition);

我在各种语言中都看到过这个初学者错误：从 Pascal 和 C 到 PHP 和 Java。您会在代码审查中标记哪些其他此类结构？

4

19 回答 19

14 
if (foo == true)
{
   do stuff
}

我一直告诉开发人员这样做应该是

if ((foo == true) == true)
{
   do stuff
}

但他还没有得到提示。

于 2008-09-26T12:51:01.453 回答
10 
if (condition == true)
{
  ...
}

代替

if (condition)
{
  ...
}

编辑：

甚至更糟糕的是，转过条件测试：

if (condition == false)
{
  ...
}

很容易理解为

if (condition) then ...
于 2008-09-26T12:49:27.383 回答
8

使用注释而不是源代码管理：
- 注释掉或重命名函数而不是删除它们，并相信源代码管理可以在需要时为您取回它们。
- 添加诸如“RWF 更改”之类的评论，而不是仅仅进行更改并让源代码管理部门承担责任。

于 2008-09-26T13:04:19.207 回答
5

我在某个地方发现了这个东西，我发现它是布尔冗余的顶峰：

return (test == 1)? ((test == 0) ? 0 : 1) : ((test == 0) ? 0 : 1);

:-)

于 2008-09-26T13:08:41.130 回答
4

在 C 以外的语言中与赋值分开声明： 

int foo;  
foo = GetFoo();
于 2008-09-26T13:00:53.177 回答
4

冗余代码本身并不是错误。但如果你真的想拯救每一个角色

return (condition);

也是多余的。你可以写：

return condition;
于 2008-09-26T13:13:06.377 回答
3

最后无用返回：

   // stuff
   return;
}
于 2008-09-26T12:47:43.737 回答
3 
void myfunction() {
  if(condition) {
    // Do some stuff
    if(othercond) {
      // Do more stuff
    }
  }
}

代替

void myfunction() {
  if(!condition)
    return;

  // Do some stuff

  if(!othercond)
    return;

  // Do more stuff
}
于 2008-09-26T12:52:36.910 回答
3

我曾经有一个人反复这样做：

bool a;
bool b;
...
if (a == true)
    b = true;
else
    b = false;
于 2008-09-26T12:56:36.307 回答
2

我看到的最常见的冗余代码构造是从不从程序中的任何地方调用的代码。

另一种是在没有意义的情况下使用的设计模式。例如，到处都写“new BobFactory().createBob()”，而不是只写“new Bob()”。

删除未使用和不必要的代码可以大大提高系统质量和团队维护它的能力。对于从未考虑过从系统中删除不必要代码的团队来说，这些好处常常令人吃惊。我曾经与一个团队坐在一起进行代码审查，删除了他们项目中一半以上的代码，而不改变他们系统的功能。我以为他们会被冒犯，但在那之后他们经常向我询问设计建议和反馈。

于 2009-06-10T15:00:21.347 回答
2

在字符串上使用 .tostring

于 2008-09-26T12:48:24.407 回答
2

将exit语句作为第一个语句放在函数中以禁用该函数的执行，而不是以下选项之一：

  • 彻底删除功能
  • 注释函数体
  • 保留功能但删除所有代码

使用exitas first 语句很难发现，您可以轻松地阅读它。

于 2008-09-26T12:58:56.380 回答
2

对 null 的恐惧（这也可能导致严重的问题）：

if (name != null)
  person.Name = name;

冗余 if （不使用 else）：

if (!IsPostback)
{
   // do something
}
if (IsPostback)
{
   // do something else
}

冗余检查（Split 从不返回 null）：

string[] words = sentence.Split(' ');
if (words != null)

更多关于检查（如果你要循环，第二次检查是多余的）

if (myArray != null && myArray.Length > 0)
  foreach (string s in myArray)

而我最喜欢的 ASP.NET：DataBind在整个代码中散布 s 以使页面呈现。

于 2008-09-26T13:15:02.423 回答
2

复制粘贴冗余：

if (x > 0)
{
   // a lot of code to calculate z
   y = x + z;
}
else
{
   // a lot of code to calculate z
   y = x - z;
}

代替

if (x > 0)
  y = x + CalcZ(x);
else
  y = x - CalcZ(x);

甚至更好（或更模糊）

y = x + (x > 0 ? 1 : -1) * CalcZ(x)
于 2008-09-26T13:37:25.040 回答
2

在堆而不是堆栈上分配元素。

{
    char buff = malloc(1024);
    /* ... */
    free(buff);
}

代替

{
    char buff[1024];
    /* ... */
}

或者

{    
    struct foo *x = (struct foo *)malloc(sizeof(struct foo));
    x->a = ...;
    bar(x);
    free(x);
}

代替

{
    struct foo x;
    x.a = ...;
    bar(&x);
}
于 2008-09-29T07:49:22.073 回答
1

我经常遇到以下情况：

function foo() {
    if ( something ) {
        return;
    } else {
        do_something();
    }
}

但这无助于告诉他们 else 在这里没用。它必须是

function foo() {
    if ( something ) {
        return;
    }
    do_something();
}

或 - 取决于在 do_something() 之前完成的检查的长度：

function foo() {
    if ( !something ) {
        do_something();
    }
}
于 2008-09-26T13:09:02.987 回答
1

从噩梦般的代码审查......

char s[100];

其次是

memset(s,0,100);

其次是

s[strlen(s)] = 0;

有很多讨厌的

if (strcmp(s, "1") == 0)

乱扔代码。

于 2008-09-30T20:37:25.150 回答
0

当您想要设置行为时使用数组。您需要在插入之前检查所有内容以确保其不在数组中，这会使您的代码更长更慢。

于 2008-09-26T13:12:52.180 回答
0

冗余 .ToString() 调用：

const int foo = 5;
Console.WriteLine("Number of Items: " + foo.ToString());

不必要的字符串格式：

const int foo = 5;
Console.WriteLine("Number of Items: {0}", foo);
于 2008-10-02T13:16:14.400 回答