Is this a valid loop?

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?

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.

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.