views:

35

answers:

2

Hi, For some time now I am trying to figure out how I can refactor some of my code to reduce redundancy throughout my application. I am just learning the basics of OOP and can create simple classes and methods but my knowledge is limited in terms of practical applicability. The following bit of code illustrates my frustration:

#region DELETE selected users - button

protected void btnDeleteSelected_Click(object sender, EventArgs e)
{
    try
    {
        foreach (GridViewRow row in GridView1.Rows)
        {
            CheckBox cb = (CheckBox)row.FindControl("chkRows");
            if (cb != null && cb.Checked)
            {
                // get the row index values (DataKeyNames) and assign them to variable
                string userName = GridView1.DataKeys[row.RowIndex].Value.ToString();

                // delete selected users and their profiles
                ProfileManager.DeleteProfile(userName);
                Membership.DeleteUser(userName);

                Msg.Text = "User(s) were sucessfully <b>DELETED</b>!";
                Msg.Visible = true;
            }
        }
    }
    catch (Exception ex)
    {
        Msg.Text = "Oops! " + ex.Message;
        Msg.Visible = true;
    }
    finally
    {
        // refresh gridview to reflect changes
        GridView1.DataBind();
    }
}

#endregion

This bit of code is used on several pages of my project in the pages codebehind file. How can I move this to a class file. I don't know how to reference an object like a gridview in a class because it does not exist like it does on the actual page.

Could some one help out please? Thank you.

+1  A: 

There are many principles that you generally apply when trying to refactor code. Currently, you're trying to refactor your code as to not violoate the DRY principle (DRY = don't repeat yourself). It would be a great move to refactor that code.

But, some other principals might come in to play that you might want to consider. The single responsibility principle would suggest that each method does only one unambiguous thing. Think about the operations your current method does. It extracts the usernames from the GridView, and then deletes some data associated with that user. That might be better off as two methods.

Also, loosely coupled code is good. You don't want a bunch of dependencies between your classes. For example, if you moved your whole method, as is, to a separate class or library, that class or library would be dependent on the ASP.NET libraries, because it makes a specific reference to the GridView control. But it doesn't need to. You could have a separate method that pulls the username out of the GridView (this method would be tightly coupled with ASP.NET), and then a separate one that does the rest of your actions (such as deleting the user's data), which only requires the username. That second method would not be coupled to ASP.NET in any way. So by having separate methods that each have a single responsibility, you'll have more loosely coupled code. Two for one.

As for what you said here:

I don't know how to reference an object like a gridview in a class because it does not exist like it does on the actual page.

You'd just want to pass a reference to your GridView when you call the method that would extract the usernames. Something like this:

public static class Util
{
    public static IEnumerable<string> GetUsernames(GridView gv)
    {
        List<string> userNames = new List<string>();
        foreach (GridViewRow row in gv.Rows)
        {
            CheckBox cb = (CheckBox)row.FindControl("chkRows");
            if (cb != null && cb.Checked)
            {
                // get the row index values (DataKeyNames) and assign them to variable
                string userName = gv.DataKeys[row.RowIndex].Value.ToString();
                userNames.Add(userName);
            }
        }
        return userNames;
    }
}

Now, in any of your asp.net code behind pages, you can do:

IEnumerable<string> usernames = Util.GetUsernames(GridView1);
foreach(string username in usernames)
    doSomething(username);

The doSomething(username) would be a call to some other method that does your delete operations, or whatever you want.

Hope this helps.

Oh, and if you're just learning the basics of OOP, I would recommend something like Head First Object-Oriented Analysis and Design, or any book that seems to adequately cover the subject. I like the O'Reilly Head First series. The information is very digestible.

Samuel Meacham
Wow! Samuel thank you kindly for an awesome response. I had to do some research on the util and IEnumerable in c# but for the most part I understood it at first glance. Your explanation really helped.I do have lots of books on c# but for the most part they are so incomprehensibly dry and boring, it is hard to translate the information into practical and usable nuggets of knowledge. I will take a look at the book you've suggested.
John Walsh
A: 

In Webforms applications, a common technique for factoring out application and/or business logic out of the code behind is to use the MVP pattern. This doesn't mean all the code typically found in the code-behind just gets moved to another class, as this really isn't any different than a code-behind. The UI rendering logic (e.g. Databinding setup, access to UI controls, etc.) is best left to the code-behind, but the business logic (in this case, the removal of users) is performed by a method within the Presenter.

In your case, I would build up a collection of usernames and call a RemoveUsers() method on the Presenter, passing in the list of usernames, which would handle interfacing with the ProfileManager and Membership components(ideally through abstractions). This allows you to write unit tests for the logic within the Presenter.

Derek Greer