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\$

locked by Jamal Mar 12 '14 at 1:44

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. See the help center for guidance on writing a good question.

Read more about locked posts here.

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\$
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\$
  • \$\begingroup\$ [nitpick] It will be called only if ScatterViewItems implemented in obvious way \$\endgroup\$ – Snowbear Mar 4 '11 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\$ – Steven Jeuris Mar 4 '11 at 12:34
  • 2
    \$\begingroup\$ public new bool Equals(object other) in SourceFile ;-) \$\endgroup\$ – Snowbear Mar 4 '11 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 '11 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\$ – Steven Jeuris Mar 6 '11 at 16:29

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