10
votes
\$\begingroup\$

I have the following code:

private ScatterViewItem FindScatterViewOfSourceFile(SourceFile find)
{
   foreach (ScatterViewItem svi in classScatterViews)
   {
      if ((svi.Tag as SourceFile).Equals(find))
      {
         return svi;
      }
   }
   return null;
}

Now I'm asking myself if this is valid or if I should better use:

private ScatterViewItem FindScatterViewOfSourceFile(SourceFile find)
{
   ScatterViewItem result = null;
   foreach (ScatterViewItem svi in classScatterViews)
   {
      if ((svi.Tag as SourceFile).Equals(find))
      {
         result = svi;
         break;
      }
   }
   return result;
}

Is there any common practive which one to use? And are both loops doing the same?

\$\endgroup\$
0

2 Answers 2

22
votes
\$\begingroup\$

First one is hundred times better then the second one. I would avoid defining a variable if you don't really need to. Also I don't believe in one return methods. Especially I don't believe that they improve readability.

LINQ would definitely improve it:

return classScatterViews.FirstOrDefault(v => v.Tag.Equals(find));

Also I would not use as operator if I do not check result for null.

\$\endgroup\$
0
9
votes
\$\begingroup\$

So you have heard about the good practice that a method should only have one return statement. This is generally true, but in your case I find it not to be necessary because the method is so small. The main reason for this 'rule' is readability, so you know what is going on. Both code samples are equally clear. When you have several places where a return would occur, it's best to use the second approach.

However, I would rewrite it by using LINQ. I believe this should work:

private ScatterViewItem FindScatterViewOfSourceFile(SourceFile find)
{
    return classScatterViews.FirstOrDefault( svi => svi.Tag.Equals(find) );
}

UPDATE:

Also note that the cast to the specific type to call the Equals() is dropped. This is redundant as the correct Equals() will be called anyhow.

\$\endgroup\$
9
  • \$\begingroup\$ [nitpick] It will be called only if ScatterViewItems implemented in obvious way \$\endgroup\$
    – Snowbear
    Mar 4, 2011 at 12:28
  • \$\begingroup\$ @Snowbear: I can't think of the nonobvious way right now, perhaps I'm missing something. :O Enlighten me. ;p \$\endgroup\$ Mar 4, 2011 at 12:34
  • 2
    \$\begingroup\$ public new bool Equals(object other) in SourceFile ;-) \$\endgroup\$
    – Snowbear
    Mar 4, 2011 at 12:37
  • 1
    \$\begingroup\$ "The main reason for this 'rule' is readability, so you know what is going on." That's simply not true. This rule was created for languages like C that don't automatically manage resources. Returning early from a function that may have allocated early makes it too easy to forget to deallocate it. In a GC language like C#, there's no need for this rule. \$\endgroup\$
    – munificent
    Mar 4, 2011 at 23:47
  • 1
    \$\begingroup\$ @munificent: Thanks for the history lesson, I didn't know that. However, I can give you examples where multiple return statements ARE less readable. Also the fact that there are other reasons doesn't exclude the fact that it also is a practice known to improve readability. I'll rephrase, the only reason I knew of. ;p \$\endgroup\$ Mar 6, 2011 at 16:29

Not the answer you're looking for? Browse other questions tagged or ask your own question.