How would you refactor this to make it nicer?

Question

How would you refactor something like this?

protected void btnAdd_Click(object sender, EventArgs e)
{
    try
    {
        string username = txtUsername.Text.Trim().ToLower();
        string password = txtPassword.Text.Trim().ToLower();
        string email = txtEmail.Text.Trim().ToLower();
        string status = ddlStatus.SelectedValue.Trim();

        IUser user = UserFactory.getInstance().createUser(username, password, email,status);

        if (user.save())
        {
            jsMsgBox("Successfully added new user");
            Response.Redirect(ConfigurationManager.AppSettings["AdminLink"], true);
        }
        else
        {
            jsMsgBox("An error was encountered while trying to add a new user.");
        }
    }
    catch (Exception ex)
    {
        jsMsgBox("An Error was encountered while trying to add a new user.");
        lblInfo.Text = ex.Message;
        lblInfo.Visible = true;
    }
}

protected void btnUpdate_Click(object sender, EventArgs e)
{
    try
    {
        string username = txtUsername.Text.Trim().ToLower();
        string password = txtPassword.Text.Trim().ToLower();
        string email = txtEmail.Text.Trim().ToLower();
        int userPK = Int32.Parse(txtUserPK.Text.ToString());
        string status = ddlStatus.SelectedValue.Trim();

        IUser user = UserFactory.getInstance().createUser(userPK, username, password, email,status);

        if (user.save())
        {
            jsMsgBox("Successfully updated selected users information.");
            Response.Redirect(ConfigurationManager.AppSettings["AdminLink"], true);
        }
        else
        {
            jsMsgBox("An error was encountered while trying to update the selected users information.");
        }
    }
    catch (Exception ex)
    {
        jsMsgBox("An Error was encountered while trying to update the selected users information.");
        lblInfo.Text = ex.Message;
        lblInfo.Visible = true;
    }
}

Take care

Answer 1

Encasuplate the assigment of variables in a method in order to reuse

private IUser GetUser()
{

    string username = FormatText(txtUsername.Text)
    string password = FormatText(txtPassword.Text);
    string email = FormatText(txtEmail.Text);       
    string status = ddlStatus.SelectedValue.Trim();
    if(txtUserPK.Text!=string.empty){
        int userPK = Int32.Parse(txtUserPK.Text);
        IUser user = UserFactory.getInstance().
                         createUser(userPK, username, password, email, status);
    }
    else
    {
        IUser user = UserFactory.getInstance().
                         createUser(username, password, email, status);
    }
    return user;
}

private string FormatText(string input)
{
    return input.Trim().ToLower();
}

Answer 2

For a start:

protected string cleaned(string raw) {
    raw.Text.Trim().ToLower()
    }
protected void attempt_to_save(IUser user, string task) {
    if (user.save()) {
        jsMsgBox("Successfully finished "+task);
        Response.Redirect(ConfigurationManager.AppSettings["AdminLink"], true);
      } else {
        jsMsgBox("An error was encountered while "+task);
        }
    }

protected void btnAdd_Click(object sender, EventArgs e)
{
    try
    {
        IUser user = UserFactory.getInstance().createUser(
            cleaned(txtUsername), 
            cleaned(txtPassword), 
            cleaned(txtEmail),
            ddlStatus.SelectedValue.Trim()
            );

        attempt_to_save(user,"adding a new user.");
    }
    catch (Exception ex)
    {
        jsMsgBox("An Error was encountered while trying to add a new user.");
        lblInfo.Text = ex.Message;
        lblInfo.Visible = true;
    }
}

protected void btnUpdate_Click(object sender, EventArgs e)
{
    try
    {

        IUser user = UserFactory.getInstance().createUser(
            Int32.Parse(txtUserPK.Text.ToString()), 
            cleaned(txtUsername), 
            cleaned(txtPassword), 
            cleaned(txtEmail),
            ddlStatus.SelectedValue.Trim()
            );

        attempt_to_save(user,"updating the selected users information.");
        }
    }
    catch (Exception ex)
    {
        jsMsgBox("An Error was encountered while trying to update the selected users information.");
        lblInfo.Text = ex.Message;
        lblInfo.Visible = true;
    }
}

Note that it was necessary to reword some of the messages slightly.

Answer 3

protected void btnAdd_Click(object sender, EventArgs e) { SaveUser();}
protected void btnUpdate_Click(object sender, EventArgs e) { SaveUser();}
private void SaveUser()
{
      int? userId = string.IsNullOrEmpty(txtUserPK.Text)? null:
                   Int32.Parse(txtUserPK.Text.ToString());
      UserFactory.Save(userId , txtUsername.Text, txtPassword.Text, 
                      txtEmail.Text, ddlStatus.SelectedValue);  
      jsMsgBox("Successfully added new user");            
      Response.Redirect(
          ConfigurationManager.AppSettings["AdminLink"], true);         
}

Then put the Save parameter processing in the Factory (or in another method somewhere

.. In UserFactory...
 Save(int? userId, string userName, string password, 
         string eMail, string selvalue)
 {
     // code to process input parameters and save/update user
     userName = userName.Trim().ToLower();
     password = password.Trim().ToLower();
     eMail    = eMail.Trim().ToLower();
     selvalue = selvalue.Trim().ToLower();
     IUser user = userId.HasValue?
                    UserFactory.getInstance().createUser(
                            userId.Value, username, password, 
                            email,status):
                    UserFactory.getInstance().createUser(
                            username, password, 
                            email, status);
     try { user.Save(); }
     catch(DBException dbX)  //   catch/rethrow Database exception 
                             //   as custom expcetion here
     {
         throw new MyCustomAppException(
                 "Save User failed: + dbX.Message",
                 dbX);
     }  
 }

and put the exception handling in the web form's main entry point...

public partial class MyWebPage:  System.Web.UI.Page     
{
    static void Page_Load(object sender, EventArgs e)         
    {
         try 
         {
             // asp.Net Page Load code here ...
         }
         catch(MyMainCustomApplicationException X)
         {
             jsMsgBox("An Error was encountered: " + X.Message);  
             lblInfo.Text = X.Message;        
             lblInfo.Visible = true;
         }
         catch(Exception eX) 
         {  
              // Maybe Log it here...
              throw;   // Important to retrhow all unexpected exceptions to
                       // MAKE web page crash !! as you have no idea
                       // whether web app is in consistent state now.
         }
    }
}

Answer 4

Also, you fetch user/password/email/status twice. I'd encapsulate those four at least into a class with a single Get method or property. If they're together on the form, I'd encapsulate them in a user control, exposing the individual values as properties of the user control.

Not refactoring, but still:

I'd get rid of the try/catch block. The message you're displaying to the user is nothing they'll want to see, or can do anything about. Better to let it propagate and get logged by ASP.NET. Use a custom error page to give the user something prettier to look at, like "sorry, we goofed".

Answer 5

There are two ways I usually go about this when two methods are THAT similar.

In either case, I usually change them into a single method and put the lines that are similar next to each other.

One is to have a variable that knows which method you are in, and use it to determine what to do.

if(update) // use update line else // use add line

You also have to be able to handle cases where you might not initialize a variable in one state (like update sets up userPK, but add does not use it)

This is a quick and easy refactor that can easily combine 2 methods and is hard to mess up if your code is reasonably clean.

The other way is more complex and used for making this "Combined" method into something reusable. I often do the previous one first, then move on to this if necessary...

There are two ways to make your combined class more reusable. One is to "Pass in" the differences. Pass in an object that implements the different lines of code.

The second is to make it a "Parent" class and implement the different lines in a method of your children. So when the parent hit the point where it had to do the create, it would just call a create() method instead. The create() method would be overridden in the child class to run one method or the other. You would probably delegate a method like this for every code difference.

Oh, and if only "Data" differs, say your "Combined" methods look like this:

if(user==0)
    System.out.println("Bill was here");
else if(user==1)
    System.out.println("Bob was here");

I usually fix that by creating an indexed array or enum so that it ends up looking like this:

System.out.println(users[user]+" was here");

I know that seems really obvious, but extracting data is a HUGE help in refactoring, it makes a great start if you don't know what to do next...

Answer 6

Try This

First create a user info object

 class UserInfo
{
    public string username {get;set;}
    public string password {get;set;}
    public string email {get;set;}
    public string status {get;set;}
}

then refactor your code like this

protected void btnAdd_Click(object sender, EventArgs e)
{
    UserInfo myUser = GetUserInfo();
    try
    {
        IUser user = UserFactory.getInstance().createUser(myUser);

        if (user.save())
        {
            jsMsgBox("Successfully added new user");
            Response.Redirect(ConfigurationManager.AppSettings["AdminLink"], true);
        }
        else
        {
            jsMsgBox("An error was encountered while trying to add a new user.");
        }
    }
    catch (Exception ex)
    {
        jsMsgBox("An Error was encountered while trying to add a new user.");
        lblInfo.Text = ex.Message;
        lblInfo.Visible = true;
    }
}

protected void btnUpdate_Click(object sender, EventArgs e)
{
    UserInfo myUser = GetUserInfo();
    int userPK = Int32.Parse(txtUserPK.Text.ToString());

    try
    {       
        IUser user = UserFactory.getInstance().createUser(userPK,myUser);

        if (user.save())
        {
            jsMsgBox("Successfully updated selected users information.");
            Response.Redirect(ConfigurationManager.AppSettings["AdminLink"], true);
        }
        else
        {
            jsMsgBox("An error was encountered while trying to update the selected users information.");
        }
    }
    catch (Exception ex)
    {
        jsMsgBox("An Error was encountered while trying to update the selected users information.");
        lblInfo.Text = ex.Message;
        lblInfo.Visible = true;
    }
}

private UserInfo GetUserInfo()
{
    UserInfo myUser = new UserInfo();

    UserInfo.username = txtUsername.Text.Trim().ToLower();
    UserInfo.password = txtPassword.Text.Trim().ToLower();
    UserInfo.email = txtEmail.Text.Trim().ToLower();
    UserInfo.status = ddlStatus.SelectedValue.Trim();

    return myUser;
}

Answer 7

First, get rid of these try/catch, have asp.net configured with a friendly error page and log any unhandled exceptions.

On the handlers, you want them to read pretty clear what's going on. I find this version pretty clear for the scenario:

protected void btnAdd_Click(object sender, EventArgs e)
{
    bool isInsert = true;
    bool saved = SaveUserFromControlInfo(isInsert);
    ShowJsMessageBox(
        saved, 
        "Successfully added new user", 
        "An error was encountered while trying to add a new user."
        );
    if(saved) RedirectToAdmin();
}
protected void btnUpdate_Click(object sender, EventArgs e)
{
    bool isInsert = false;
    bool saved = SaveUserFromControlInfo(isInsert);
    ShowJsMessageBox(
        saved,
        "Successfully updated selected users information.",
        "An error was encountered while trying to update the selected users information."
        );
    if (saved) RedirectToAdmin();
}

Supporting methods would be:

void RedirectToAdmin()
{
    Response.Redirect(ConfigurationManager.AppSettings["AdminLink"], true);
}
void ShowJsMessageBox(bool success, string succesMessage, string failedMessage)
{
    jsMsgBox(success ? succesMessage : failedMessage);
}
void SaveUserFromControlInfo(bool isInsert)
{
    string username = txtUsername.Text.Trim().ToLower();
    string password = txtPassword.Text.Trim().ToLower();
    string email = txtEmail.Text.Trim().ToLower();
    string status = ddlStatus.SelectedValue.Trim();
    var userFactory = UserFactory.getInstance();
    IUser user;
    if( isInsert )
        user = userFactory.createUser(username, password, email, status);
    else
    {
        int userPK = Int32.Parse(txtUserPK.Text);
        user = userFactory.createUser(userPK, username, password, email, status);
    }
    return user.save();
}

I would shuffle the SaveUserFromControlInfo a bit. I would prefer to have a class associated with the CreateUser request, that holds an optional userPK.

Ps. I don't usually do it like this, since I use MVP, and this view would only have code to get the user info in the controls (with a class defined elsewhere) and forward of the events to the presenter.

Answer 8

I'm going to go out on a limb here and suggest that this block of code is NOT in particular need of refactoring. There may be room to make it more elegant, more concise, or more robust, but as it stands the code is dead simple and easy to read. My gut reaction in this case is, "if it ain't broke, don't fix it."

edit: All that having been said, I'm upvoting at least three of the other answers. There are some great ideas there.

edit #2: After having another look, the most obvious problem to me is the duplication of the following code:

    string username = txtUsername.Text.Trim().ToLower();
    string password = txtPassword.Text.Trim().ToLower();
    string email = txtEmail.Text.Trim().ToLower();
    string status = ddlStatus.SelectedValue.Trim();

The real problem here is that if the UI changes to add, remove, or rename one of the fields, this supporting code needs to be changed in two places. It's much better to have this happen in a supporting class, as is suggested by several of the other answers.