Jump to content



Photo

Avoid code Repetition in conditions

c# language angnostic

  • Please log in to reply
80 replies to this topic

#1 xWhiplash

xWhiplash

    Neowinian Senior

  • Joined: 07-March 08

Posted 23 August 2014 - 16:55

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?




#2 adrynalyne

adrynalyne

    Neowinian Senior

  • Tech Issues Solved: 1
  • Joined: 29-November 09

Posted 23 August 2014 - 17:10

Any reason why you cannot use a switch statement?



#3 OP xWhiplash

xWhiplash

    Neowinian Senior

  • Joined: 07-March 08

Posted 23 August 2014 - 17:19

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
    }
}


#4 Depicus

Depicus

    depicus.com

  • Joined: 18-February 11
  • Location: United Kingdom
  • OS: OS X 10.9 - Windows 8.1
  • Phone: Nexus 5

Posted 23 August 2014 - 17:48

Would this work ?

if(condition1 && condition2 && condition3)
{

}
else if (condition1 && condition2)
{

}
else if (condition1)
{

}
else
{

}


#5 OP xWhiplash

xWhiplash

    Neowinian Senior

  • Joined: 07-March 08

Posted 23 August 2014 - 17:49

 

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.



#6 Depicus

Depicus

    depicus.com

  • Joined: 18-February 11
  • Location: United Kingdom
  • OS: OS X 10.9 - Windows 8.1
  • Phone: Nexus 5

Posted 23 August 2014 - 18:04

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.



#7 OP xWhiplash

xWhiplash

    Neowinian Senior

  • Joined: 07-March 08

Posted 23 August 2014 - 18:06

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



#8 rfirth

rfirth

    Software Engineer

  • Tech Issues Solved: 2
  • Joined: 11-September 09
  • Location: Baton Rouge, Louisiana
  • OS: Windows 8
  • Phone: Nokia Lumia 620

Posted 23 August 2014 - 18:29

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(); 
}







#9 +_Alexander

_Alexander

    Neowinian

  • Tech Issues Solved: 1
  • Joined: 21-January 13
  • Location: USA
  • OS: W8.1 u1
  • Phone: Nokia 521

Posted 23 August 2014 - 18:44

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.

Edited by _Alexander, 23 August 2014 - 18:48.


#10 simplezz

simplezz

    Neowinian Senior

  • Tech Issues Solved: 8
  • Joined: 01-February 12

Posted 23 August 2014 - 19:09

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 );
}


#11 simplezz

simplezz

    Neowinian Senior

  • Tech Issues Solved: 8
  • Joined: 01-February 12

Posted 23 August 2014 - 19:18

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.

#12 adrynalyne

adrynalyne

    Neowinian Senior

  • Tech Issues Solved: 1
  • Joined: 29-November 09

Posted 23 August 2014 - 19:24

 

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.



#13 +theblazingangel

theblazingangel

    Software Engineer

  • Tech Issues Solved: 6
  • Joined: 25-March 04
  • Location: England, UK

Posted 23 August 2014 - 19:59

<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.



#14 OP xWhiplash

xWhiplash

    Neowinian Senior

  • Joined: 07-March 08

Posted 23 August 2014 - 20:03

 
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.



#15 Lant

Lant

    Neowinian Senior

  • Tech Issues Solved: 1
  • Joined: 13-April 06

Posted 23 August 2014 - 20:08

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 by Lant, 23 August 2014 - 20:13.