Avoid code Repetition in conditions

[xWhiplash]

Hello,

 

I have been programming for over 15 years now.  I consider myself a very good programmer, but I understand (like all of us) there are things that I need to work on.  One of these things is code repetition when dealing with conditions.  I will give a generic sample:

if(condition1)
{
     //perform some logic
     if(condition2)
     {
          //perform some logic
          if(condition3)
          {
               //Perform logic
          }
          else
          {
               //MethodA(param)
          }
     }
     else
     {
          //MethodA(param)
     }
}
else
{
     //MethodA()
}

Now, I cannot make it easy by doing the following:

if(condition1 && condition2)
{

}
else
{

}

I cannot do this since I need to perform some logic if condition1 is true and before I test condition2.

 

Is there a way to structure if...else blocks to where if you need to call a method in each else blocks, you are not repeating yourself?

[adrynalyne]

Any reason why you cannot use a switch statement?

[xWhiplash]

Any reason why you cannot use a switch statement?

 

Wouldn't that get a bit messy if it needs to have nested conditions and logic before each condition?

 

Edit, this might help.  Here is an ASP.NET C# Example of checking for a persistent logins when a new session is detected:

protected void Session_Start(object sender, EventArgs e)
{
    //Check persistent login token
    string cookieName = Properties.Settings.Default.AuthenticationCookieName;
    HttpCookie cookie = Request.Cookies[cookieName];
    if (cookie != null)
    {
        int userID;
        string token = cookie["Token"];
        if (int.TryParse(cookie["UserID"], out userID) && !string.IsNullOrEmpty(token) && Authentication.IsAuthenticated(userID, token))
        {
            Account account = new Account(userID);
            if (account != null && account.Login())  //Checks if the user was created successfully, and if they were able to login successfully (not banned for example)
            {
                Session.Add("Account", account);
            }
            else
            {
                ResetAuthentication(userID);  //Overload, removes the authentication record in the database if it exists, then calls ResetAuthentication().
            }
        }
        else
        {
            ResetAuthentication();  //Redirect to login page
        }
    }
    else
    {
        ResetAuthentication();  //Redirect to login page
    }
}

[Depicus]

Would this work ?

if(condition1 && condition2 && condition3)
{

}
else if (condition1 && condition2)
{

}
else if (condition1)
{

}
else
{

}

[xWhiplash]

 

Would this work ?

if(condition1 && condition2 && condition3)
{

}
else if (condition1 && condition2)
{

}
else if (condition1)
{

}
else
{

}

 

I would still have to write ResetAuthentication (from the ASP.NET example I posted previously) multiple times with this right?  Maybe I was right, there is no real way to avoid it.  I just hate repeating code.

[Depicus]

Well just had a pizza so feeling very programmery :)

 

The only other way I can think is remove all the else statements and do the ResetAuthentication outside the if block and then you could exit the sub after setting Session.Add("Account", account);

 

Not sure this is good practice however.

[xWhiplash]

Is repeating a method call a few times bad?  I just try to avoid as much code repetition as possible.

[rfirth]

protected void Session_Start(object sender, EventArgs e)
{
    string cookieName = Properties.Settings.Default.AuthenticationCookieName;
    HttpCookie cookie = Request.Cookies[cookieName];
    if (cookie != null)
    {
        int userID;
        string token = cookie["Token"];
        if (int.TryParse(cookie["UserID"], out userID) && !string.IsNullOrEmpty(token) && Authentication.IsAuthenticated(userID, token))
        {
            Account account = new Account(userID);
            if (account != null && account.Login())
                Session.Add("Account", account);
            else
                ResetAuthentication(userID);
            return;
        }
    }

    ResetAuthentication(); 
}

?

?

[+Red King Subscriber²]

My suggestion,

	string cookieName = Properties.Settings.Default.AuthenticationCookieName;
	HttpCookie cookie = Request.Cookies[cookieName];
	
	if (cookie == null)
	{
		ResetAuthentication(); 
		return;
	}

	// Rest of logic here
	

To avoid deep nesting, a little code repetition is not bad IMO.

[simplezz]

Wouldn't that get a bit messy if it needs to have nested conditions and logic before each condition?

 

Edit, this might help.  Here is an ASP.NET C# Example of checking for a persistent logins when a new session is detected:

protected void Session_Start(object sender, EventArgs e)
{
    //Check persistent login token
    string cookieName = Properties.Settings.Default.AuthenticationCookieName;
    HttpCookie cookie = Request.Cookies[cookieName];
    if (cookie != null)
    {
        int userID;
        string token = cookie["Token"];
        if (int.TryParse(cookie["UserID"], out userID) && !string.IsNullOrEmpty(token) && Authentication.IsAuthenticated(userID, token))
        {
            Account account = new Account(userID);
            if (account != null && account.Login())  //Checks if the user was created successfully, and if they were able to login successfully (not banned for example)
            {
                Session.Add("Account", account);
            }
            else
            {
                ResetAuthentication(userID);  //Overload, removes the authentication record in the database if it exists, then calls ResetAuthentication().
            }
        }
        else
        {
            ResetAuthentication();  //Redirect to login page
        }
    }
    else
    {
        ResetAuthentication();  //Redirect to login page
    }
}

private bool
is_user_authenticated ( HttpCookie cookie, ref int user_id )
{
    if ( NULL == cookie )
       return false;

    string token = cookie [ "Token" ];
    if ( string.IsNullOrEmpty ( token ) || !int.TryParse ( cookie [ "UserID" ], out user_id ) )
       return false;

    return Authentication.IsAuthenticated ( user_id, token );
}

protected void 
Session_Start ( object sender, EventArgs e )
{
    int user_id;
    if ( !is_user_authenticated ( Request.Cookies [ Properties.Settings.Default.AuthenticationCookieName ], ref user_id ) ) {
        ResetAuthentication ();
        return;
    }   
    
    Account account = new Account ( user_id );
 
    /* Shouldn't your account class validate whether or not that user_id already exists and fail if it does (i.e throw exception)? Not quite sure of your logic and intentions here. */
    /* Separation of concerns could be an issue. */
    if ( NULL == account ) {
       ResetAuthentication ( user_id );
       return;
    }

    Session.Add ( "Account", account );
}

[simplezz]

Is repeating a method call a few times bad?  I just try to avoid as much code repetition as possible.

I think we all try and keep repetition to a minimum. Sometimes it's not always possible. All in all your code doesn't seem too bad. If anything, I'd say the worst things about it are readability and intentions. The code should be as self-explanatory as possible negating the need for comments in most cases. If that means splitting it up into discrete functions, then go right ahead. To me, readability is the most important thing. Repetition I can live with if the code is easy to understand.

[adrynalyne]

 

Wouldn't that get a bit messy if it needs to have nested conditions and logic before each condition?

 

Edit, this might help.  Here is an ASP.NET C# Example of checking for a persistent logins when a new session is detected:

protected void Session_Start(object sender, EventArgs e)
{
    //Check persistent login token
    string cookieName = Properties.Settings.Default.AuthenticationCookieName;
    HttpCookie cookie = Request.Cookies[cookieName];
    if (cookie != null)
    {
        int userID;
        string token = cookie["Token"];
        if (int.TryParse(cookie["UserID"], out userID) && !string.IsNullOrEmpty(token) && Authentication.IsAuthenticated(userID, token))
        {
            Account account = new Account(userID);
            if (account != null && account.Login())  //Checks if the user was created successfully, and if they were able to login successfully (not banned for example)
            {
                Session.Add("Account", account);
            }
            else
            {
                ResetAuthentication(userID);  //Overload, removes the authentication record in the database if it exists, then calls ResetAuthentication().
            }
        }
        else
        {
            ResetAuthentication();  //Redirect to login page
        }
    }
    else
    {
        ResetAuthentication();  //Redirect to login page
    }
}

It could be worse.  I am maintaining some software at work that has multiple nested conditionals...in an mvc view.  I didn't write it, but I get to suffer through it.

[+theblazingangel MVC]

<snip>

 

Edit, this might help.  Here is an ASP.NET C# Example of checking for a persistent logins when a new session is detected:

protected void Session_Start(object sender, EventArgs e)
{
    //Check persistent login token
    string cookieName = Properties.Settings.Default.AuthenticationCookieName;
    HttpCookie cookie = Request.Cookies[cookieName];
    if (cookie != null)
    {
        int userID;
        string token = cookie["Token"];
        if (int.TryParse(cookie["UserID"], out userID) && !string.IsNullOrEmpty(token) && Authentication.IsAuthenticated(userID, token))
        {
            Account account = new Account(userID);
            if (account != null && account.Login())  //Checks if the user was created successfully, and if they were able to login successfully (not banned for example)
            {
                Session.Add("Account", account);
            }
            else
            {
                ResetAuthentication(userID);  //Overload, removes the authentication record in the database if it exists, then calls ResetAuthentication().
            }
        }
        else
        {
            ResetAuthentication();  //Redirect to login page
        }
    }
    else
    {
        ResetAuthentication();  //Redirect to login page
    }
}

 

From the way you've written this, I wonder whether you're stuck in the mindset of 'single point of exit'.

 

You could refactor your function like this:

protected void Session_Start(object sender, EventArgs e)
{
    //Check persistent login token
    HttpCookie cookie = Request.Cookies[Properties.Settings.Default.AuthenticationCookieName];
    if (cookie != null)
    {
        int userID;
        string token = cookie["Token"];
        if (int.TryParse(cookie["UserID"], out userID) && !string.IsNullOrEmpty(token) && Authentication.IsAuthenticated(userID, token))
        {
            Account account = new Account(userID);
            if (account != null && account.Login())  //Checks if the user was created successfully, and if they were able to login successfully (not banned for example)
            {
                Session.Add("Account", account);
                return;
            }
        }
        
        ResetAuthentication(userID);  //Redirect to login page
    }
    
    ResetAuthentication();  //Redirect to login page
}

With some refactoring of ResetAuthentication, it might even be possible to cut this down to a single call here.

[xWhiplash]

 

From the way you've written this, I wonder whether you're stuck in the mindset of 'single point of exit'.

 

You could refactor your function like this:

protected void Session_Start(object sender, EventArgs e)
{
    //Check persistent login token
    HttpCookie cookie = Request.Cookies[Properties.Settings.Default.AuthenticationCookieName];
    if (cookie != null)
    {
        int userID;
        string token = cookie["Token"];
        if (int.TryParse(cookie["UserID"], out userID) && !string.IsNullOrEmpty(token) && Authentication.IsAuthenticated(userID, token))
        {
            Account account = new Account(userID);
            if (account != null && account.Login())  //Checks if the user was created successfully, and if they were able to login successfully (not banned for example)
            {
                Session.Add("Account", account);
                return;
            }
        }
        
        ResetAuthentication(userID);  //Redirect to login page
    }
    
    ResetAuthentication();  //Redirect to login page
}

With some refactoring of ResetAuthentication, it might even be possible to cut this down to a single call here.

 

I was under the impression that multiple points of exit was bad design.

[Lant]

I was under the impression that multiple points of exit was bad design.

 

Always write code that is readable. If the design choices that people say are bad are making you write more complicated spaghetti code, just ignore it.

The reason for these rules is that bad code can be written using certain designs, but there will be many cases when you do want to do these things.

 

For example, goto can be acceptable in C in certain conditions (freeing memory on error conditions). But with C++ it never makes sense as you have RAII.

 

 

If you do not have exceptions then commonly the following style is used. Using multiple exit point helps prevent the code from becoming a horrible nested mess.

 
if (!a) return BAD_A;
 
if (!b) return BAD_B;
 
result = do_stuff();
if (!result) return result;
 
return SUCCESS;
 

The alternative would be:

final_result = nil;

if (a)
{
  if (b)
  {
    result = do_stuff();
    if (result)
    {
      final_result = SUCCESS;
    }
    else
    {
      final_result = result;
    }
  }
  else
  {
    final_result = BAD_B;
  }
}
else
{
  final_result = BAD_A;
}

return final_result;
 

Edited August 23, 2014 by Lant

[+theblazingangel MVC]

I was under the impression that multiple points of exit was bad design.

 

Not at all. Switching from procedural programming to functional programming gave us single point of entry, which is fantastic. Some people seem to think that if single entry is better then single exit must be better also, but this is illogical. Multi exit is fine.

[xWhiplash]

Okay how is this?

//Check persistent login token
string cookieName = Properties.Settings.Default.AuthenticationCookieName;
HttpCookie cookie = Request.Cookies[cookieName];
if (cookie == null)
{
    ResetAuthentication();
    return;
}

int userID;
string token = cookie["Token"];
if (int.TryParse(cookie["UserID"], out userID) && !string.IsNullOrEmpty(token) && Authentication.IsAuthenticated(userID, token))
{
    Account account = Account.GetUser(userID);
    if (account != null && account.Login())  //Checks if the user was created successfully, and if they were able to login successfully (not banned for example)
    {
        Session.Add("Account", account);
        return;
    }
}

ResetAuthentication(userID);

The reason I do not like to use exceptions for this type of stuff is that all I am doing is logging the error:

//Check persistent login token
string cookieName = Properties.Settings.Default.AuthenticationCookieName;
HttpCookie cookie = Request.Cookies[cookieName];
if (cookie == null)
{
    ResetAuthentication();
    return;
}

int userID;
string token = cookie["Token"];
if (int.TryParse(cookie["UserID"], out userID) && !string.IsNullOrEmpty(token) && Authentication.IsAuthenticated(userID, token))
{
    try
    {
        Account account = new Account(userID);
        if (account.Login())  //Checks if the user was created successfully, and if they were able to login successfully (not banned for example)
        {
            Session.Add("Account", account);
            return;
        }
    }
    catch (AccountException ae)
    {
        //Log error in database/event viewer
        ErrorLogging.LogError(ae);
        throw;
    }
}

ResetAuthentication(userID);

Another thing I was taught is do not swallow exceptions.  If you catch exceptions, use throw again.  That will cause the site to give them the yellow screen of death, or a custom error page instead of just displaying a message though.

[simplezz]

Okay how is this?

//Check persistent login token
string cookieName = Properties.Settings.Default.AuthenticationCookieName;
HttpCookie cookie = Request.Cookies[cookieName];
if (cookie == null)
{
    ResetAuthentication();
    return;
}

int userID;
string token = cookie["Token"];
if (int.TryParse(cookie["UserID"], out userID) && !string.IsNullOrEmpty(token) && Authentication.IsAuthenticated(userID, token))
{
    Account account = Account.GetUser(userID);
    if (account != null && account.Login())  //Checks if the user was created successfully, and if they were able to login successfully (not banned for example)
    {
        Session.Add("Account", account);
        return;
    }
}

ResetAuthentication(userID);

It's hard to say because I don't really know what your code is doing in detail. Perhaps I'm not the best person to ask as I don't do C# or ASP. However, just from looking at it, I can see that you're doing some kind of authentication based on a cookie. Where it becomes vague is regarding your Account class. Is that a custom class? It seems to me that just returning NULL from the constructor is a little shall we say ambiguous from a readability standpoint. Clearly you're trying to indicate that constructor failed, but the reason for it failing can only be ascertained from your comment (possibly). Perhaps it might be better to do all that validation inside the class itself and raise an exception/return an error if it fails in some way, for the sake of encapsulation / separation of concerns. I'm probably just being pedantic though so..

Another thing I was taught is do not swallow exceptions.  If you catch exceptions, use throw again.  That will cause the site to give them the yellow screen of death, or a custom error page instead of just displaying a message though.

Well, throwing it allows you to unwind the call stack. However, at the top (usually the UI/Main loop/entry point), don't you generally handle the exception in a controlled manner? such as, displaying an error message, or logging to a file or something. I usually unwind my call stacks in C like that too (returning a result and raising it up the stack until it reaches the top). I guess it depends if it's an expected or an unexpected error. If something is likely to happen, logging/displaying an error is probably the best course of action.

[Andre S. Veteran]

I'd code the main function so it does just the happy path and returns success or failure. The caller is responsible for handling the error case(s).

protected void Session_Start(object sender, EventArgs e)
{
    int userID;
    if (!TryAddAccount(out userID))
    {
        if (userID >= 0)
        {
            ResetAuthentication(userID);
        }
        else
        {
            ResetAuthentication();
        }
    }
}

bool TryAddAccount(out int userID)
{
    userID = -1;
    //Check persistent login token
    string cookieName = Properties.Settings.Default.AuthenticationCookieName;
    HttpCookie cookie = Request.Cookies[cookieName];
    if (cookie != null)
    {
        int userID;
        string token = cookie["Token"];
        if (int.TryParse(cookie["UserID"], out userID) && !string.IsNullOrEmpty(token) && Authentication.IsAuthenticated(userID, token))
        {
            Account account = new Account(userID);
            if (account != null && account.Login())  //Checks if the user was created successfully, and if they were able to login successfully (not banned for example)
            {
                Session.Add("Account", account);
                return true;
            }
        }
    }
    return false;
}

This assumes that -1 can be used as a magic value to indicate error. Otherwise you could use a Nullable<int> or encapsulate it in some object.

 

An interesting read on the topic (difficult to apply to C# while it lacks discriminated unions, but still): http://fsharpforfunandprofit.com/posts/recipe-part2/

[xWhiplash]

It's hard to say because I don't really know what your code is doing in detail. Perhaps I'm not the best person to ask as I don't do C# or ASP. However, just from looking at it, I can see that you're doing some kind of authentication based on a cookie. Where it becomes vague is regarding your Account class. Is that a custom class? It seems to me that just returning NULL from the constructor is a little shall we say ambiguous from a readability standpoint. Clearly you're trying to indicate that constructor failed, but the reason for it failing can only be ascertained from your comment (possibly). Perhaps it might be better to do all that validation inside the class itself and raise an exception/return an error if it fails in some way, for the sake of encapsulation / separation of concerns. I'm probably just being pedantic though so..

Well, throwing it allows you to unwind the call stack. However, at the top (usually the UI/Main loop/entry point), don't you generally handle the exception in a controlled manner? such as, displaying an error message, or logging to a file or something. I usually unwind my call stacks in C like that too (returning a result and raising it up the stack until it reaches the top). I guess it depends if it's an expected or an unexpected error. If something is likely to happen, logging/displaying an error is probably the best course of action.

 

I did not show the correct code on my initial post with the ASP.NET.  The actual code was

Account account = Account.GetUser(userID);

Which would return a null object if the userID was not valid.  Or I could go the constructor way and throw an exception. 

 

I'd code the main function so it does just the happy path and returns success or failure. The caller is responsible for handling the error case(s).

protected void Session_Start(object sender, EventArgs e)
{
    int userID;
    if (!TryAddAccount(out userID))
    {
        if (userID >= 0)
        {
            ResetAuthentication(userID);
        }
        else
        {
            ResetAuthentication();
        }
    }
}

bool TryAddAccount(out int userID)
{
    userID = -1;
    //Check persistent login token
    string cookieName = Properties.Settings.Default.AuthenticationCookieName;
    HttpCookie cookie = Request.Cookies[cookieName];
    if (cookie != null)
    {
        int userID;
        string token = cookie["Token"];
        if (int.TryParse(cookie["UserID"], out userID) && !string.IsNullOrEmpty(token) && Authentication.IsAuthenticated(userID, token))
        {
            Account account = new Account(userID);
            if (account != null && account.Login())  //Checks if the user was created successfully, and if they were able to login successfully (not banned for example)
            {
                Session.Add("Account", account);
                return true;
            }
        }
    }
    return false;
}

This assumes that -1 can be used as a magic value to indicate error. Otherwise you could use a Nullable<int> or encapsulate it in some object.

 

An interesting read on the topic (difficult to apply to C# while it lacks discriminated unions, but still): http://fsharpforfunandprofit.com/posts/recipe-part2/

 

That is an interesting approach.  Thanks.

[Andre S. Veteran]

That is an interesting approach.  Thanks.

The main thing you get from it is separation of concerns - each function only does one thing. This makes it easy to reason about what any individual function does, and favors short, composable functions. It also avoids any code duplication - in this case for instance, each possible error case is only handled once.

[xWhiplash]

The main thing you get from it is separation of concerns - each function only does one thing. This makes it easy to reason about what any individual function does, and favors short, composable functions. It also avoids any code duplication - in this case for instance, each possible error case is only handled once.

 

I never really understood that argument.  In the case of ASP.NET Session_Start, it might basically do one thing (start a session), but what if I need the session to create the user object, and a few other things?  Do I just create more methods and call it from Session_Start in that case?

 

Or is the argument that (in this case) Session_Start should ONLY create the user object and nothing else?  How specific are the guidelines to "method only performs one action"? 

[Andre S. Veteran]

Your original function had duplicate code because it mixed two concerns: adding a new account, and resetting the authentication in case of any error. Splitting each concern into its own function gets rid of the duplication and makes the code easier to understand: each function is only concerned with one thing. If you wanted to change the error handling logic, for instance, you don't have to understand a complex code structure with HttpCookies and Accounts and Sessions and all these things irrelevant to your error handling logic.

 

Other approaches mentionned here like early returns, get rid of the deep nesting but not the code duplication.

 

This is just a general principle and there's no hard rule as to what's a "thing" or "concern" or when it's a good time to split a function into several. In general, if a function is getting long, or deeply nested, or has several local variables, or duplicate code, etc., it is a good time to stop and think about what this function does and how the things it do might be split into shorter, more composable and easier to understand functions.

[xWhiplash]

Your original function had duplicate code because it mixed two concerns: adding a new account, and resetting the authentication in case of any error. Splitting each concern into its own function gets rid of the duplication and makes the code easier to understand: each function is only concerned with one thing. If you wanted to change the error handling logic, for instance, you don't have to understand a complex code structure with HttpCookies and Accounts and Sessions and all these things irrelevant to your error handling logic.

 

This is just a general principle and there's no hard rule as to what's a "thing" or "concern" or when it's a good time to split a function into several. In general, if a function is getting long, or deeply nested, or has several local variables, or duplicate code, etc., it is a good time to stop and think about what this function does and how the things it do might be split into shorter, more composable and easier to understand functions.

 

Ah okay, so Session_Start can "basically" perform multiple things, just have it call methods that do dedicated tasks?

[simplezz]

I never really understood that argument.  In the case of ASP.NET Session_Start, it might basically do one thing (start a session), but what if I need the session to create the user object, and a few other things?  Do I just create more methods and call it from Session_Start in that case?

You can treat the Session_Start as an entry point function (It's an ASP callback function right?), so yes, you can do all kinds of things in it. However, if you do all of it in that one function, it will turn into a jumbled mess. So as I said earlier in this thread, split it into discrete functions, and give those tasks to them.

 

function main ()
{
     doWorkA();
     doWorkB();
     doWorkC();
    
     etc....
}

[Andre S. Veteran]

Ah okay, so Session_Start can "basically" perform multiple things, just have it call methods that do dedicated tasks?

It should only do one thing but that may imply delegating the rest of work to one or more other functions. In the example I gave, the only real logic coded in Session_Start is error handling; it delegates the actual work (the "happy path") to TryAddAccount.