Flow control patterns and best practices

Question

We got in argument with co-worker about the patterns of flow control and general code design. Need your opinion - which is the better/cleaner/preferred way of writing the code ?

This is for MVC 3, but that doesn't really matter.
Version 1:

public ActionResult Login(LoginData loginData)
{
    if (!ModelState.IsValid)
    {
        ShowGlobalError("Invalid credentials.");
        return View(loginData);
    }

    UserProfile profile = null;
    try
    {
        // see if we have profile with those credentials
        profile = this.RetrieveUserProfile(loginData.Email.Trim(), loginData.Password.Trim());  // this just goes to db and tries to get user profile. Returns null if profile isn't found
    }
    catch (Exception ex)
    {
        ShowGlobalError("DB is down");
        LogError(...);
        return View(loginData);
    }


    if (profile == null)
    {
        // nope, we don't.. ask again
        ShowGlobalError("Invalid credentials.");
        return View(loginData);
    }

    // ok, we're good
    Session["Profile"] = profile;
    FormsAuthentication.SetAuthCookie(profile.Email, false);
    FormsAuthentication.RedirectFromLoginPage(profile.Email, loginData.EnablePermanentCookie);

    return View(loginData);
}

Version 2:

public ActionResult Login(Credentials credentials){
    try{
        PersonalProfile profile = AuthenticateUser(credentials);
        SetProfileSessionstate(profile);    // this does 'Session["Profile"] = profile;'
        SetFormsAuthenticationAndRedirect(profile); 
    }
    catch(Exception ex){
            ShowGlobalError("invalid login, please try again.");
    }
    return View(credentials);
}

public void SetFormsAuthenticationAndRedirect(PersonalProfile profile){
     FormsAuthentication.SetAuthCookie(profile.Email, loginData.EnablePermanentCookie);
     FormsAuthentication.RedirectFromLoginPage(profile.Email, loginData.EnablePermanentCookie);
}

Version 1 is littered with return statements, version 2 is using try/catch for flow control. So, which is the better way, or are we both doing it wrong, and there's a better way you can share ?

Thanks !

So is version 1 supposed to be equivalent to version 2? Because it doesn't look like they do the same thing. — Phil, Mar 10 '11 at 14:34

score 2 · Answer 1 · answered Mar 10 '11 at 15:03

2

I'd say the best approach to flow control in this scenario would be the tester-doer pattern. Personally I never use exceptions to control the flow of my application.

if (this.CanAuthenticateUser(credentials))
{
    PersonalProfile profile = AuthenticateUser(credentials);
    SetProfileSessionstate(profile);    // this does 'Session["Profile"] = profile;'
    SetFormsAuthenticationAndRedirect(profile);
}
else
{
    ShowGlobalError("invalid login, please try again.");
}

return View(credentials);

answered Mar 10 '11 at 15:03

MattDavey

8,897
3
31
54

1

AuthenticateUser goes to database and tries to get the user profile. So you still need a try/catch for this method, and you still need to either check if profile was actually retrieved, or have a try/catch for SetProfileSessionstate, as it will access profile's properties. – Evgeni Mar 10 '11 at 15:56
I think you missed the point of my post. There point here is the **pair** of methods, CanAuthenticateUser and AuthenticateUser. The CanAuthenticateUser (tester) returns a boolean value indicating whether or not AuthenticateUser (doer) will be successful. It's always a good idea to provide code paths like this that can avoid exceptions being thrown.. – MattDavey Mar 15 '11 at 13:26

score 1 · Accepted Answer · answered Mar 10 '11 at 21:07

1

I like #1 much better than #2

no.2 is lazy coding

no.1 explicitly capture errors

it is not a good idea to rely on exceptions to catch logic mistakes or errors in #2, if profile is null, you do not check that and rely on throwing exception to catch that exceptions are expensive operations in general and should only be relied on for unforseen logic outcomes (Exceptions!)

answered Mar 10 '11 at 21:07

ttt

26
1

So why not use #2 and just add "if (profile == null) { showError(); return; }"? – Phil Mar 10 '11 at 21:26
As a side note, I fully agree that catching general Exceptions rather than more specific ones is a bad idea. But I don't think that's what the question is about. – Phil Mar 10 '11 at 21:29
The reason I catch Exception is that this try/catch block only wraps database operation. So if an exception is thrown at that point, chances are it will be database-related, unless I get lucky to get OutOfMemoryException or something like that. So yeah, I'm being a bit lazy with that, but I don't see the value in having specific catches in this particular piece of code. And the moment you add if (profile == null) - it will pretty much turn into #1 :) – Evgeni Mar 10 '11 at 23:55

score 0 · Answer 3 · edited May 23 '17 at 12:34

0

I don't think version 2 is using try/catch for flow control. Using try/catch for flow control looks like this.

I think both approaches are valid, though the second one is easier for me to follow.

EDIT:

Exceptions exist for the purpose of stopping execution and notifying the calling method that "I couldn't finish executing, and here's why". In a way, that is flow control.

Consider the following code

try
{
    foo = doSomething1();  // Could throw an Exception here
    doSomething2(foo);
}
catch (Exception ex)
{
    // Show error message
}

This may technically be "flow control," but it is the kind of flow control that says "Hey, this process could not finish, and here's why." Which is exactly what Exceptions are for.

Consider the following code:

try
{
    foo = doSomething1();  // Could throw an Exception here
}
catch (Exception ex)
{
    // Show error message and return
}

if (foo != null)
{
    doSomething2(foo);
}
else
{
    // Show an error message and return
}

This code does exactly the same thing as the previous code segment, but is longer, has the same Exception handling overhead, and is more convoluted. In both cases, doSomething2 will not be executed unless doSomething1 completes successfully. So why not go with the simpler code?

EDIT:

At the very least, you could do this, and it would work around your flow-control rule:

public ActionResult Login(Credentials credentials)
{
    try
    {
        innerLogin(credentials);
    }
    catch(Exception ex)
    {
        ShowGlobalError("invalid login, please try again.");
    }
    return View(credentials);
}

private void innerLogin(Credentials credentials)
{
    PersonalProfile profile = AuthenticateUser(credentials);
    SetProfileSessionstate(profile);
    SetFormsAuthenticationAndRedirect(profile); 
}

edited May 23 '17 at 12:34

Community

1
1

answered Mar 10 '11 at 14:47

Phil

6,561
4
44
69

Version 1 is testing if profile was actually retrieved, version 2 tries to access profile properties, which will fail if profile does not exist - then it goes to catch statement. – Evgeni Mar 10 '11 at 14:54
1

If the profile was not received from AuthenticateUser(), an exception will be thrown, correct? You are catching that Exception in the catch block, meaning you don't need to test if it was actually received. – Phil Mar 10 '11 at 15:15
1

It would be different if AuthenticateUser() didn't throw an Exception, or if there were a method called CanAuthenticate(). If you could use something like CanAuthenticate(credentials) to avoid the Exception, then do that. – Phil Mar 10 '11 at 15:17
Then CanAuthenticate would have to return a tri-state structure (true, false, DatabaseExceptionTrown), and it gets messy at that point, imo. – Evgeni Mar 10 '11 at 16:04
1

If that's the case, then stay with what you've got. The point is, if an Exception ever occurs, you will need to stop the login process with an error message. Both version 1 and version 2 do that. Frankly, I don't think you should even catch any Exceptions in the Login() method. Let the Exceptions bubble up to the method that called Login, and let that method handle the errors. – Phil Mar 10 '11 at 18:07
This is the method that deals with UI.. So it has to catch the exceptions. – Evgeni Mar 10 '11 at 19:20
Ok, I edited the answer to try to deal with the root issue. Have a look. – Phil Mar 10 '11 at 19:44

Flow control patterns and best practices

3 Answers3