Multiple exit points

[Cags]

I have read in many places how having multiple exit points in a program is not a good idea and can cause trouble during testing aswell as disrupt the general readability of code. Having said this I often find myself writting code like this.

// multiple exit points
if(e.Control)
{
	// ctrl s, save
	if(e.KeyValue == 83)
	{
		// save file
		return;
	}

	// ctrl o, open
	if(e.KeyValue == 83)
	{
		// open file
		return;
	}

	// ctrl e, perform action blah...
	if(e.KeyValue == 69)
	{
		// do stuff
		return;
	}
}
else
{
}

Essentially what this does is check the keyvalue, then if it was the key you pressed it is handled and there is no reason to continue checking through each key value. I format my code like this as I find it nicer to read than the equivalent example with a singular exit point (also this method also prevents checking all other key values after the event has been handled)...

// single exit point
if(e.Control)
{
	if(e.KeyValue == 83) // ctrl s, save
	{
		// code to save file
	}
	else if(e.KeyValue == 79) // ctrl o, open
	{
		// code to load file
	}
	else if(e.KeyValue == 69) // ctrl e, perform action blah
	{
		// do stuff...
	}
}
else
{
}

return;

Now I'll freely admit that in this example I've give the code is perfectly understandable, but once you start adding alot of if / else, switch statements or loops in place of the comments I put above things soon start getting extremely complicated to follow. I guess Methods could be used for each key, but in some system that would be creating an awefull lot of methods, some of which wouldn't have more than a line or two of code. I just wondered what anyone else had to say about the matter.

 

[mskeel]

The second set of code is much better. You aren't actually expecting both ctrl+e and ctrl+s at the exact same time are you? That first chunk of code won't catch that anyway. The first set says the keycode could be all of those things (which you get around with a "hidden" return. The second set says it can only be one. I guess what I'm saying is that you are faking the second set of code in the first set, so why not make it more apparent what you actually mean?

...but once you start adding alot of if / else, switch statements or loops in place of the comments...

Why bother doing that at all? Make a method for each of the comments:

// single exit point
if(e.Control)
{
	if(e.KeyValue == 83) // ctrl s, save
	{
		SaveToFile();
	}
	else if(e.KeyValue == 79) // ctrl o, open
	{
		LoadFile();
	}
	else if(e.KeyValue == 69) // ctrl e, perform action blah
	{
		DoStuff();
	}
}
else
{
}
 
return;  //do you really need this?  This is probably the end of the method which will return anyway...

And then you have self documenting, easy to read, transparent code. And that's the best kind!

I guess Methods could be used for each key, but in some system that would be creating an awefull lot of methods,...

True, but by using methods you will have effectively decoupled the logic of how the code is handled with what the code actually does. So, in the future, when, say, you've notice a bug in your save code, you simply go to the SaveToFile() method and modify that instead of sifting through a giant series of if statements and possibly messing something up there which could effect other parts of the system.

 

[snarfblam]

I know it might sound crazy, but...

// More appropriate program control flow structure
if(e.Control)
{
    switch (e.KeyValue) {
        case 83:
            SaveToFile();
            break;
        case 79:
            LoadFile();
            break;
        case 69:
            DoStuff();
            break;
        default:
            Whatever();
            break;
    }
}

Switches suit this type of situation very well. Not only do they give your program more structure but I think that they are more readable. And if readability is importatnt, constants would be a very nice finishing touch.

using System.Windows.Forms;
// ...
const int KeyS = 83;
const int KeyO = 79;
const int KeyE = 69;
// ...
    if(e.Control) {
        switch(e.KeyValue) {
            case KeyS:
                SaveToFile();
                break;
            case KeyO:
                LoadFile();
                break;
            case KeyE:
                DoStuff();
                break;
        }
    }

 

[Cags]

The 2 83's is a simple typing error (which is fairly obviously from the comments), I guess the problem with this example is that neither code set looks complex in these simplified forms. You all seem to have completely missed the point that this was a simplified example of a complex situation. Of course your examples look nice and neat. In a more complex application practically any key on the keyboard can be used and each one does a number of different things depending on a selection of different states that can be set from within that application. Also in many cases a large selection of keys will call the same method. If I get round to it I might post a more accurate (complex) example tomorrow.

Also one thing I'll have to bare in mind is that people find different things easier to read, as was clearly illustrated by the looping post I mad a few weeks ago.

Having said this thankyou for your comments, I do take them onboard

 

[snarfblam]

You all seem to have completely missed the point that this was a simplified example of a complex situation. Of course your examples look nice and neat. In a more complex application practically any key on the keyboard can be used and each one does a number of different things depending on a selection of different states that can be set from within that application. Also in many cases a large selection of keys will call the same method. If I get round to it I might post a more accurate (complex) example tomorrow.

Also one thing I'll have to bare in mind is that people find different things easier to read, as was clearly illustrated by the looping post I mad a few weeks ago.

Having said this thankyou for your comments, I do take them onboard

Excuse my bluntness, but you have quite a way of saying thank you. I simply can't imagine what kind of answer you were expecting. And no one is missing anything. We provided examples of how to simplify something potentially complex. Extracting functions and using abbreviated syntax would be senseless in a relatively simple situation. It is when it becomes complex that you should consider our tips.

My point was that there is a programming construct designed for a situation where only one of many possibilities may occur. The more complex the situation becomes, the more "else if"s and equality operators and returns you can replace with simple "case"s. As for multiple keys performing the same function: you can have multiple cases execute a single block of code:

switch(whatever) {
    case 0:
        function0();
        break;
    case 1:
        function1();
        break;
    case 2: // Perform the same function for cases 2, 3, and 4
    case 3:
    case 4:
        function2();
        break;
    case 5:
        function3();
        break;
}

Now I'll freely admit that in this example I've give the code is perfectly understandable, but once you start adding alot of if / else, switch statements or loops in place of the comments I put above things soon start getting extremely complicated to follow. I guess Methods could be used for each key, but in some system that would be creating an awefull lot of methods, some of which wouldn't have more than a line or two of code. I just wondered what anyone else had to say about the matter.

If a case has only a line or two of code, don't extract a function. If it is longer, do extract a function. There is no rule stating that either all cases call a function or all cases execute local code. If you are worried about having too many functions, then ask your self whether you want one hundred clearly named functions that perform one hundred clearly defined operations or one single function which has one hundred unnamed operations embedded in a switch, or worse yet mixed in a tangled web of elseifs. And if you create a function for each operation you can use the same function elsewhere, for example, in response to a menu selection or a button click.

 

[Cags]

Excuse my bluntness, but you have quite a way of saying thank you.

I apologise if my response sounded ungrateful or just plain rude, that was not my intention, but I was in a rush when I posted. I was just concerned that you were taking the extremely simple example I gave and taking that at face value rather than looking at the larger picture.

I've looked at using the switch statement (and I am in some parts of the method). The problem I was having with that was in the places I wanted all 'letter' keys to call the same function. As you point out this could be achieved by doing the following, but that looks extremely unsightly with 26 cases+.

switch(whatever) {
    case 0:
    case 1:
    case 2: 
    case 3:
    case 4:
...
    case 50:// Perform the same function for all cases
        function1();
        break;
}

The way I've been solving this problem is to use if / else statements, which allow for...

if(whatever >= 0 && whatever <= 50)
{
    function1();
}
else
{
    // etc, etc
}

... and I personally find this looks much better. Unfortunately when this is part of a nested set of if statements things get complicated. I've already been simplifying it slightly by using some broad scope methods, such as...

if(ApplicationModeA = AppModeA.Edit)
{
    ParseKeyEditMode(e);
}
else if(ApplicationModeA = AppModeA.View)
{
    ParseKeyViewMode(e);
}

The only problem I'm having with this approach is that I sometimes find it difficult to work out which order to segment the code. For example do I split the methods based on mode first, or do I parse the key first, then work out the mode if required.

I guess what I'm seeing from your replies is that the best option is going to be to take this a step further and have these methods split into more methods. I think my real reluctance to-do this is the difficulty that will be involved in naming each method efficiently (so that the name is identifiable, but not practically an essay). Also using regions to logically group the methods should keep the project from getting too difficult to work with.

Right I've started to babble now so I'll call that a wrap and hope I haven't inadvertently insulted somebody this time.

 

[mskeel]

It sounds like you have the right idea. And it is OK to have 100 methods if that's what your application needs. It's OK to write 1000's of lines of code in a single class if that's what is needed by the system. Big stuff like that is perfectly legit so long as you are actively making the decisions of what the code will look like. Just burping up a 500 line function is not necesarily a great idea -- but if you have to do it, then do it. One of the past versions of Aegis, the missile defense program for the US Navy held the record for the largest functional program ever written. Functional programming is a little different than OO, but they can have millions of lines of code and thousands of functions. I mean, Aegis is a huge, complex beast, that is expected to help protect people when it all hits the fan -- and it was written in thousands of functions.

Sometimes you just can't get around it, esspecially in complex situations. Grouping things by regions and adopting other conventions is a good way of helping to keep everything organized. My advice would be always actively decide what your program will look like and you should be fine, even if turns out big.

 

[Nerseus]

Early returns - I like them, if used "correctly". Obviously, if your program works - it works. The rest is window dressing.

Some argue that early returns make code less readable. The arguement is that a function always flows to the bottom - hence one exit point. Having small functions goes hand in hand with that. Meaning, it would be Ok to have only one exit point per function if every function were so small that you could visually see all of the code and fit it in your brain at one time.

If you happen to have a larger function then having some early returns to handle the exception cases makes sense. Or, if you have a function that does nothing but delegate to other functions, you could use early returns. For example, you may have a Validate() function that calls multiple other functions to handle all the details. If you choose to use bool testing on each function, you may having something like this:

private bool Validate()
{
    if (!ValidateName()) return false;
    if (!ValidateAddress()) return false;
    if (!ValidateEmail()) return false;

    return true;
}

Some OO programmers would say that code like that above should be broken down, I think it looks fine. The argument would be that each ValidateNNN() function would belong in the appropriate class. While I love programming with classes and breaking them down as much as possible, I also realize the real world constraints of delivering on time. So, it's a trade-off.



Here's one last sample where I'd use an early return and others might not.
This is a static function in a library class. It's meant to get a value out of a DataSet, substituting in a default value when the column is DBNull. NullDateTime is a constant. GetVal returns null if the column doesn't exist (another helper function).

Technically, this could probably be refactored into two functions: the top half of the code handles the exception cases while the bottom half takes care of pulling out the DateTime value. Don't critisize the code, I'm sure it could be cleaned up even more. It's only to illustrate an "early - out" function.

public static DateTime GetDateTime (DataRow dr, string column, DateTime defaultValue)
{
	Debug.Assert(column!=null && column.Length>0);

	if(dr==null) return defaultValue;

	object o = GetVal(dr, column);
	if((o == null) || (o==System.DBNull.Value)) return NullDateTime;

	DateTime val;
	try
	{
		val = (DateTime)o;
	}
	catch
	{
		throw new Exception(String.Format("Cannot convert param to DateTime. Column={0}.{1}, Param={2}", dr.Table.TableName, column, o.ToString()));
	}

	return val;
}

-ner

 

[Cags]

Funnily enough I have a method practically identical to the Validate() example you gave in my sudoku solver application. I tried to refactor it, but every time I did it just looked more confusing.

 

[snarfblam]

Early returns - I like them, if used "correctly". Obviously, if your program works - it works. The rest is window dressing.

I have to respect your ability to defy "general rules" with discretion and without shooting yourself in the foot. Some people can do that. Many people can't. For them it is all or none. It doesn't make them a bad programmer; it just means that they need certain guidelines to help keep their code neat, readable and maintainable. I suppose that alot of people can't see both sides, which is when people begin to view general programming practice as mandatory programming practice, and others don't understand how we can function in a world where return is anything but the last line.

My advice (to Cags as well as everyone else) is that when you see something that is considered "bad programming practice" understand why, and then decide for yourself when if ever it is appropriate. If it makes your code neater, more readable, or optimizes a bottleneck, by all means, do what makes sense.

 

[PlausiblyDamp]

Personally I tend to use early returns only as a 'guard clause' on a method i.e. when validating input as only using a single return can result in the entire method being an if block.

i.e.

//rather than
public bool TestMethod(string s)
{
if ((string != null) || (string != String.Empty))
    {
    //real code here
   
    //for many lines

    //which may not all fit on a single screen

    //which eventually results in a 
    return true;
    }
else
    {
     return false;
    }
}

//is far simpler to read as 
public bool TestMethod2(string s)
{
if ((s == null) || (s == String.Empty))
   return false;

//real code here
   
//for many lines

//which may not all fit on a single screen

//which eventually results in a 
return true;
}

 

[mskeel]

Similiarly to plausibly damp's example, I also ditch out as soon as I know I can in potentially lengthy operations such as searching through a list.

public bool ContainsName(string nameToFind)
{
   foreach (string name in this._names)
   {
      if (name == nameToFind)
      {
          return true;
      }
   }

   return false;
}