Which of these code snippets are better practice?

[promposive]

Which is better, and why?


edit: Wow... How do you make the "Attach Code" actually get line breaks???

//Example 1:
public Employee FindByEID(long EID)
{
foreach (Employee employee in this)
{
if (employee.EID == EID)
{
return employee;
}
}

return null;
}


//Example 2:
public Employee FindByEID(long EID)
{
Employee returnValue = null;

foreach (Employee employee in this)
{
if (employee.EID == EID)
{
returnValue = employee;
break;
}
}

return returnValue;
}

 

[Ken g6]

Define "better".

You might consider a more complex data structure than an unordered list for your Employee list (unless it's really short).

Edit: In other words, what are your criteria for "better"? Code size? Speed? Readability?

But you're just asking about the return procedure, aren't you? In general, I'd say it depends on whether there's something you need to do with the employee object after you find it.

 

[Crusty]

That's not really a good example, I would definitely find both 1 and 2 acceptable as they are. Usually I won't do what you did in example 1 if I can't see all return states in the same scroll view in my code editor. As in... if the function is say 200 lines I won't put return statements at lines 50,100,150 and 200. It's too easy for me or others to skip over it not seeing it.

 

[brandonbull]

If you expect to have multiple employees with the same EID, wouldn't you only capture the first or last employee depending on the example you use?

 

[promposive]

Well.. I tried to make the sample as basic as possible, to get the general idea, I think Crusty gets what I am trying to ask.
Keng6: yes I realize there are better ways for searching for a number, I was just trying to make a basic example...
brandonbull: take a closer look @ example 2, it will behave exactly like example 1

Basically the question I am asking is it better to have some variable you initialize at the beginning of a function, then check all the way through the function if it is null or not to continue doing other code, or just to return the variable whenever you feel it is necessary to return it.

 

[brandonbull]

Originally posted by: C0BRA99
Well.. I tried to make the sample as basic as possible, to get the general idea, I think Crusty gets what I am trying to ask.
Keng6: yes I realize there are better ways for searching for a number, I was just trying to make a basic example...
brandonbull: take a closer look @ example 2, it will behave exactly like example 1

Basically the question I am asking is it better to have some variable you initialize at the beginning of a function, then check all the way through the function if it is null or not to continue doing other code, or just to return the variable whenever you feel it is necessary to return it.

break doesn't cause the loop to end?

 

[promposive]

Break does cause the loop to end, just like the return does in example 1, therefor both examples will cause the first Employee found with matching EID to be returned.

 

[Markbnj]

I just intuitively shudder a bit at returning out of a loop. So I would try to adhere to the second model.

 

[promposive]

Originally posted by: Markbnj
I just intuitively shudder a bit at returning out of a loop. So I would try to adhere to the second model.

Why is it that you shudder?

 

[Markbnj]

Originally posted by: C0BRA99

Originally posted by: Markbnj
I just intuitively shudder a bit at returning out of a loop. So I would try to adhere to the second model.

Why is it that you shudder?

Oh, just... I don't know. There's nothing syntactically or operationally wrong with it. Just seems a little inelegant.

 

[promposive]

Ok, here is another example that is a bit longer to demonstrate the question:

Example 1:
private short GetStatus(Random random, Setting setting)
{
if (setting == null)
{
return 0;
}

if (setting.AlarmExpires > DateTime.Now)
{
return setting.AlarmStatusCode;
}

bool hasAlarm = GetRandomBool(Math.Abs(1 - this.AlarmChance), random);
if (hasAlarm)
{
meterSetting.AlarmExpires = DateTime.Now.AddMinutes(random.Next(0, 720));

bool hasAlarm1 = GetRandomBool(Chance.SlightlyHigh, random);
if (hasAlarm1)
{
return (short)Alarm.Codes.Alarm1;
}
bool hasAlarm2 = GetRandomBool(Chance.EvenChance, random);
if (hasAlarm2)
{
return (short)Alarm.Codes.Alarm2;
}

Alarm.Codes[] alarms = (Alarm.Codes[])Enum.GetValues(typeof(Alarm.Codes));
return (short)alarms[random.Next(0, alarms.Length)];
}
else
{
setting.AlarmExpires = DateTime.Now.AddMinutes(random.Next(0, 2880));
return 0;
}
}





example 2:
private short GetStatus(Random random, Setting setting)
{
short returnValue = 0;

if (setting != null)
{
if (setting.AlarmExpires < DateTime.Now)
{
bool hasAlarm = GetRandomBool(Math.Abs(1 - this.AlarmChance), random);
if (hasAlarm)
{
setting.AlarmExpires = DateTime.Now.AddMinutes(random.Next(0, 720));

bool hasAlarm2 = GetRandomBool(Chance.SlightlyHigh, random);
if (hasAlarm2)
{
returnValue = (short)Alarm.Codes.Alarm2;
}
else
{
bool hasAlarm3 = GetRandomBool(Chance.EvenChance, random);
if (hasAlarm3)
{
returnValue = (short)Alarm.Codes.Alarm3;
}
else
{
Alarm.Codes[] alarms = (Alarm.Codes[])Enum.GetValues(typeof(Alarm.Codes));
returnValue = (short)alarms[random.Next(0, alarms.Length)];
}
}
}
else
{
setting.AlarmExpires = DateTime.Now.AddMinutes(random.Next(0, 2880));
}
}
else
{
returnValue = setting.AlarmStatusCode;
}
}

return returnValue;
}

 

[ivan2]

single point of exit doesn't really matter to me, unless I have some garbage to clean up or memory to free before exit. So both 1 or 2 are fine in this example.

 

[promposive]

I have found a couple interesting articles on this topic:
http://www.secretgeek.net/monkey_exit.asp
http://www.codinghorror.com/blog/archives/000486.html

 

[degibson]

In general, I find:
* Flatter is better
* More readable is better (this can go either way)

From a performance standpoint, it depends on the language. If returns are explicit register jumps (e.g. often SPARC compilers do this), break/return is probably better as it won't pollute the branch target buffer. If returns are explicit instructions (e.g. x86), it doesn't matter, as the return address stack will be accessed no matter what. If the compiler is doing some magic and making the returns conditional pc-relative branches, fewer static branches is better. If the language is interpreted, it doesn't matter at all.

Edit: As per my above statement, it depends on the ISA as well.

 

[IHateMyJob2004]

returning only once in each method is an old style. I think there was a reason for it years ago.

Here is the thing, it is a style issue and unless there is a documented standard you are using on your project that discusses this matter specifically, no one should say that one style is better than another.

If someone is saying someone else is doing it wrong, it could easily be considered a personal attack. Don't attack style that is not in a documented style guideline.

Originally posted by: Crusty
That's not really a good example, I would definitely find both 1 and 2 acceptable as they are. Usually I won't do what you did in example 1 if I can't see all return states in the same scroll view in my code editor. As in... if the function is say 200 lines I won't put return statements at lines 50,100,150 and 200. It's too easy for me or others to skip over it not seeing it.

Umm, and seeing all the places that returnValue is assigned is easier? Most IDEs will atleast color the keyword return for you, but that is not true for returnValue.

 

[Scooby Doo]

Isn't there a saying about "getting out as soon as possible"?

 

[imported_Dhaval00]

Compilers (and frameworks in general) are getting smarter, Scooby Doo. There are too many optimizations - things like String.Intern() in Java and .NET that most people are usually unaware of:

string s1 = "ABCDE";
string s2 = "ABC" + "DE";
// Prove that s1 and s2 point to the same element in the intern pool
Console.WriteLine(String.ReferenceEquals(s1, s2)); // => True

Going back to the OP's question, I completely agree with IHateMyJob2004 that this is a style issue. Most newer compilers *will* warn you if you screw up a return statement (for example, .NET will broadcast the warning "Unreachable code detected" before you even compile the executable!).

 

[Scooby Doo]

Well I was more referring to you making a mistaking with a "resultValue" or "result or whatever between the assignment and the actual return. Actually I think it extends beyond just that, I think it's more like "you've got what you've needed you have no reason to be here anymore"

 

[drebo]

Non-indented code makes my head spin.

 

[promposive]

Originally posted by: drebo
Non-indented code makes my head spin.

Yea I know... That's normally what the "Attach Code" button is used for, but if I used it the code would also have been Non-Linebreaked and your head would of exploded not just spun... lol

 

[Kirby]

Originally posted by: Scooby Doo
Isn't there a saying about "getting out as soon as possible"?

I've only heard that with interrupt service routines, since you can't get other interrupts while in one.

 

[bsobel]

Originally posted by: Crusty
That's not really a good example, I would definitely find both 1 and 2 acceptable as they are. Usually I won't do what you did in example 1 if I can't see all return states in the same scroll view in my code editor. As in... if the function is say 200 lines I won't put return statements at lines 50,100,150 and 200. It's too easy for me or others to skip over it not seeing it.

Next we can discuss the evil of 200+ line functions 😉

 

[Crusty]

Originally posted by: bsobel

Originally posted by: Crusty
That's not really a good example, I would definitely find both 1 and 2 acceptable as they are. Usually I won't do what you did in example 1 if I can't see all return states in the same scroll view in my code editor. As in... if the function is say 200 lines I won't put return statements at lines 50,100,150 and 200. It's too easy for me or others to skip over it not seeing it.

Next we can discuss the evil of 200+ line functions 😉

Lol, yep. I cringe when I see one... although I saw I something that really made me wonder recently...


value = Math.Abs(someNumber) * Math.Sign(someNumber)

I hope it's remnants of some other bigger statement that was just overlooked when refactoring it maybe.

 

[clamum]

I almost always go with Option 1.