ASP.NET MVC: Ugly code needs refactoring (returning three values) [closed]

Question

Closed. This question is opinion-based. It is not currently accepting answers.

Want to improve this question? Because this question may lead to opinionated discussion, debate, and answers, it has been closed. You may edit the question if you feel you can improve it so that it requires answers that include facts and citations or a detailed explanation of the proposed solution. If edited, the question will be reviewed and might be reopened.

Closed last year.

Improve this question

I've been brooding over a design issue (smelly code kinda thing) for a couple days now. Maybe you can help.

I have a "Login" Method in my RegistrationService, and currently it looks simplified like this:



public Boolean Login(String username, String password, 
                     out String successRedirectUrl, 
                     out IValidationDictionary validationResults)
{
    successRedirectUrl = "";
    if (!Validator.IsValid(username) || !Validator.IsValid(password)) return false;

    // Other logic
    // Distributed login requests etc.
    // Build Redirect Url if login was successful etc.
}

Okay, let me explain above code. The method's main return value (Boolean) is supposed to tell the caller whether or not the login request was successful. Now, IF it was successful, I need to redirect the user to a different Url (thus, the "out" parameter successRedirectUrl). If it was NOT successful, I need to tell the user in the view what was wrong - thus the ValidationDictionary (Modelstate).

This code is really ugly and hard to maintain though. I thought of getting rid of the Boolean return type (returning the successRedirectUrl directly and checking on the caller side if it is empty) but I felt things became even more unclear.

Any idea how to do this better?

Thank you!

I really think you mean: s/breeding/brooding. Breeding is something else entirely ;) . In fact, there you go- fixed it for you. — Joel Coehoorn
– Joel Coehoorn, Commented Aug 15, 2009 at 20:03

Reed Copsey · Accepted Answer · 2009-08-15 19:59:20Z

9

Make a custom class to hold all three values, and return it. Get rid of the "out" parameters.

answered Aug 15, 2009 at 19:59

Reed Copsey

567k80 gold badges1.2k silver badges1.4k bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Perpetualcoder · Accepted Answer · 2009-08-15 20:30:26Z

3

How about this??

public class LoginOutput{

private bool _isLoginSuccess=false;
public bool IsLoginSuccess{/*Usual get set block*/}

private string _successRedirectUrl = String.Empty();
public string SuccessRedirectUrl{/*Usual get set block*/}

public IValidationDictionary ValidationResultDict{/*Usual get set block*/}
}

//your method now could be

public LoginOutput Login(string username, string password){
// your logic goes here
}

answered Aug 15, 2009 at 20:30

Perpetualcoder

13.6k10 gold badges67 silver badges101 bronze badges

1 Comment

Perpetualcoder Over a year ago

you could handle the dictionary stuff by refactoring that part in another method..would be more testable

xanadont · Accepted Answer · 2009-08-15 20:49:30Z

1

Questions:

Is the redirect URL different for different users? - I would say it shouldn't be, but if it is different, the decision shouldn't be in your business layer. This is UI logic and should be there.

What's your IValidationDictionary interface? You can probably just use it directly in your UI logic:

public IValidationDictionary Login(string user, string password);

var user = "bob";
var validator = Login(user, "password");

if (validator.IsValid)
    Response.Redirect(GetUserPage(user));
else
    HandleLoginError();

Note that GetUserPage() should not be a database lookup or anything else complicated. Again, this should be simple UI logic, something like:

public string GetUserPage(string user)
{
    return "/MyPage/" + user;
}

answered Aug 15, 2009 at 20:49

xanadont

7,6326 gold badges39 silver badges51 bronze badges

1 Comment

Alex Over a year ago

Yes, the redirect URL is different for different users.

Collectives™ on Stack Overflow

ASP.NET MVC: Ugly code needs refactoring (returning three values) [closed]

3 Answers 3

Comments

1 Comment

1 Comment

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

1 Comment

1 Comment

Related