可能是BAD编码风格...请发表评论

时间:2020-03-06 14:59:52  来源:igfitidea点击:

我正在检查新名称是否已经存在。

代码1

if(cmbxExistingGroups.Properties.Items.Cast<string>().ToList().Exists(txt => txt==txtNewGroup.Text.Trim())) {
        MessageBox.Show("already exists.", "Add new group");
      }

否则我可能会写:

代码2

foreach(var str in cmbxExistingGroups.Properties.Items)
      {
        if(str==txtNewGroup.Text) {
        MessageBox.Show("already exists.", "Add new group");        
            break;
        }   
      }

我写了这两本书,以为我正在利用代码1中的语言功能。

...是的:他们俩都为我工作...我想知道它的表现:-/

解决方案

我赞赏第一个示例的聪明之处(假设它可以工作),但是第二个示例对于下一个必须维护代码才能弄清楚的人来说要容易得多。

编码的细节并不总是很糟糕。我比第一个更喜欢第二个代码段。试想一下,我们将必须维护(甚至更改其功能)第一个示例……嗯。

我会同意,选择第二个,因为其他人都可以更轻松地进行维护,而当我们在6到12个月内回到原来的状态时,会更容易记住自己在做什么。

我曾经引用过它,但我会再做一次:

Write your code as if the person maintaining it is a homicidal maniac
  who knows where you live.

我想在WTF的每分钟范围内,第一个会超出图表。计算点数,每行多于两个点是一个潜在问题

cmbxExistingGroups.Properties.Items.Contains(text)

不能正常工作吗?

有时只需一点缩进就能带来与众不同的世界:

if (cmbxExistingGroups.Properties.Items
    .Cast<string>().ToList()
    .Exists
     (
         txt => txt==txtNewGroup.Text.Trim()
     )) 
{
    MessageBox.Show("already exists.", "Add new group");
}

由于使用List <String>,因此在按唯一值比较复杂对象时,最好也删除Exists谓词,并使用Contains ... use Exists。

这里有些错误:

1)这两位代码没有做相同的事情,第一个寻找txtNewGroup的修剪版本,第二个寻找txtNewGroup

2)调用ToList()只会降低效率,没有意义

3)将谓词与Exists一起使用会产生过大杀伤力,这就是我们所需要的一切

因此,第一个可以很容易地归结为:

if (cmbxExistingGroups.Properties.Items.Cast<string>.Contains(txtNewGroup.Text))
{
    // Stuff
}

我可能会创建一个变量,为" cmbxExistingGroups.Properties.Items.Cast"赋予一个有意义的简单名称,但随后我会说,它比显式的foreach循环更容易理解。

both of them works for me ..i am wonodering about the performance

我看不到有人读过这个问题:)我想我明白我们在做什么(我不使用这种语言)。第一种尝试生成列表并一次测试它。第二个执行显式迭代,并且如果它早发现重复项,则可以"短路"自身(提前退出)。问题是,由于语言的实现,"一次全部"是否更有效率。

两者中的第二个将表现更好,并且其表现将与使用Contains的其他人的样本相同。

第一个使用额外修剪的原因。加上对列表的转换。因此迭代一次以进行转换,然后再次开始检查是否存在,并且每次都进行修整,但是如果找到将退出迭代。第二个开始迭代一次,没有修剪,如果找到则退出。

简而言之,第二个问题的答案要好得多。

好吧,如果是我,那将是2的一种变体。总是比一线更喜欢可读性。此外,请始终提取一种方法使其更清晰。

呼叫代码成为

if( cmbxExistingGroups.ContainsKey(txtNewGroup.Text) )
{
   MessageBox.Show("Already Exists");
}

如果为组合框定义扩展方法

public static class ComboBoxExtensions
{
    public static bool ContainsKey(this ComboBox comboBox, string key)
    {
        foreach (string existing in comboBox.Items)
        {
            if (string.Equals(key, existing))
            {
                return true;
            }
        }
        return false;
    }
}

首先,它们不相等。第一个示例对txtNewSGroup.Text.Trim()进行了检查,第二个示例进行了修剪。同样,第一个将所有内容都转换为字符串,而第二个则使用从迭代器中输出的任何内容。我认为这是一个对象,否则我们将不需要在第一名的演员表。

因此,公平地说,与LINQ样式中的第二个样本最接近的等效项是:

if (mbxExistingGroups.Properties.Items.Cast<string>().Contains(txtNewGroup.Text)) {
   ...
}

这还不错。但是,由于我们似乎使用的是旧样式IEnumerable而不是新的IEnumerable <T>,所以为什么不给我们另一种扩展方法:

public static Contains<T>(this IEnumerable e, T value) {
   return e.Cast<T>().Contains(value);
}

现在我们有:

if (mbxExistingGroups.Properties.Items.Contains(txtNewGroup.Text)) {
   ...
}

IMO可读性很强。

第一个代码位很好,除了可以调用Enumerable.Any()-而不是调用Enumerable.ToList()List <T> .Exists()`之外,它可以进行惰性计算,因此它从不为List <T>分配内存,它将停止枚举cmbxExistingGroups.Properties.Items并将其强制转换为string。同样,从谓词内部调用修剪意味着它会在它所查看的每个项目中发生。最好将其移出外部范围:

string match = txtNewGroup.Text.Trim();
if(cmbxExistingGroups.Properties.Items.Cast<string>().Any(txt => txt==match)) {
    MessageBox.Show("already exists.", "Add new group");
}

从性能角度来看:

txtNewGroup.Text.Trim()

在循环外执行一次控件交互/字符串操作,而不是执行n次。