ASP C# How to program neat GUI code

Question

For about a few months i'm programming ASP C#. I always program a lot code in the events and in the load event i check the querystring for valid data. This is some sample code i have in one of my projects:

protected void Page_Load(object sender, EventArgs e)
{
    if (Controller.Manual == null)
    {
        Response.Redirect("login.aspx");
    }

    lblLocation.Text = "<a href='viewdocument.aspx'>" + Controller.Manual.Title + "</a>";

    if (Request.QueryString["gchap"] != null)
    {
        if (Controller.IsNumeric(Request.QueryString["gchap"].ToString()))
        {
            genchap = Convert.ToInt32(Request.QueryString["gchap"]);

            FillGeneralList();

            SetChapterTitle();
        }
    }
    if (Request.QueryString["qchap"] != null)
    {
        if (Controller.IsNumeric(Request.QueryString["qchap"].ToString()))
        {
            qualchap = Convert.ToInt32(Request.QueryString["qchap"]);

            FillQualityList();

            SetChapterTitle();
        }
    }

    // Check document Id is set (did)
    if (Request.QueryString["did"] != null)
    {
        if (Controller.IsNumeric(Request.QueryString["did"].ToString()))
        {
            docId = Convert.ToInt32(Request.QueryString["did"]);

            DetermineView();
        }
    }

}

I know there must be a way to accomplish this on a more neat way. And this is just the load event. On other events, like click and onchange events i have similar code. I think this is spaghetti code and not well-arranged. So can you tell me how i can arrange my code?

EDIT:

What i want to know is, is there a more neat way to, let's say, fill a listbox? And where do i check whether a querystring value has valid data? Where do i check whether the (input/querystring) data is a number? And where should you put the code that validates the Querystring? Also in the load event?

Answer 1

you should follow layered approach. ie: put all your data access code in data access layer, put all your business logic (which also includes validations) in your business layer, put all your model code in your business object layer

and finally for ui - try to never generate html mark up from within the code as far as possible. also, always create a root class for your aspx pages where it has common methods already implemented. then subclass this root class for every other aspx pages

if you are going to hardcode html markup within your c# code - i can assure you this would always result in a lot of chaos (based on my own experience)

but there are situations where you simply cant avoid it. for such cases - this is what i do - i get rid of the code behind and simply put that code in my aspx / ascx file itself. that way when i have to change my ui based on never ending client requests, i dont have to recompile my code - i simply replace my aspx / ascx files on the staging / production server.

you know how clients are : hmmm can u make the black strip look a bit like gray, can u increase the spacing between lines, can u change the text of this hyper link... requests like these never seem to end :-)

Answer 2

Try using TryParse (for example) and you can simplify all the code that looks like

xx.IsNumeric(Request.QueryString["qchap"].ToString())

and

Convert.ToInt32(Request.QueryString["gchap"]);

and reduce the number of calls to Request.QueryString variables

You could try something like

Original code

if (Request.QueryString["gchap"] != null)
{
    if (Controller.IsNumeric(Request.QueryString["gchap"].ToString()))
    {
        gchap = Convert.ToInt32(Request.QueryString["gchap"]);

        FillGeneralList();

        SetChapterTitle();
    }
}

Suggestion

int? gchap; //nullable types thanks Richard :D
if (!int.TryParse(Request.QueryString["gchap"], out id)) {gchap = null};

if (gchap != null) {
     FillGeneralList();
     SetChapterTitle();
}
// you could make this neater with your own little method

Have a look at this post http://stackoverflow.com/questions/349742/how-do-you-test-your-request-querystring-variables

Answer 3

I feel your pain with some of the organization issues with ASP.NET websites. I've had similar code to yours on several projects.

If you have the choice of your frameworks you might look into ASP.NET MVC. This allows you to have clear separation between the View (Html), the Controllers (All actions and business logic) and the Model (database). That way you have zero code in your codebehind files, it all stays nice and neat in controllers.

Answer 4

Try capturing the repetitive code in a separate function. (qchap / gchap)

e.g.:

qualchap = ConvertFillAndSet(Request.Querystring["qchap"]);
genchap = ConvertFillAndSet(Request.QueryString["gchap"]);

private int ConvertFillAndSet(string qrystring)
{
  int numberToReturn = 0;      

  //if the conversion was ok -> true, else false
  if (Int32.TryParse(qrystring,numberToReturn))
  {
    FillQualityList();
    SetChapterTitle();
  }

  //returns 0 if tryparse didn't work
  return numberToReturn;
}

Answer 5

Where to start. Unfortunately despite other comments, you're not really writing anything that is 'web forms' specific. So moving to MVC isn't going to magically make your code better.

---

1 Don't roll your own authentication: Use forms authentication unless you have a compelling reason not to. When using forms authentication, you don't need to write code on every page to check that you're logged in. The framework handles that for you.

---

2 Learn to use the server controls:

Also as others write you shouldn't be writing html in code, especially for something so trivial. Web forms doesn't make you do this either.

  <!-- this is in MyPage.aspx -->
  <asp:HyperLink id="viewLink" runat="server" />

  // in the code-behind file MyPage.aspx.cs
  viewLink.NavigateUrl = "~/viewdocument.aspx";
  viewLink.Text = Controller.Title;

If you're going to stick with web forms, you need to get familiar with the ASP.Net Page life-cycle

---

3 Your code is in need of refactoring. No matter if it's web forms, php, or MVC. Here are some basic refactorings, and none of this is really .net specific. I'll walk through these in small steps.

// this may be a good candidate for an extension method
int? ConvertNullable(string nullableInt) {
  if( string.IsNullOrEmpty(nullableInt) )
    return null;

  int value;
  if( Int32.TryParse(nullableInt, out value) )
    return value;

  return null;
}

which then allows you to write.

int genchap? = ConvertNullable(Request.QueryString["gchap"]);
int qualchap? = ConvertNullable(Request.QueryString["qualchap"]);
int docId? = ConvertNullable(Request.QueryString["did"]);

FillQualityList(genchap,qualchap);
SetChapterTitle(genchap,qualchap);
DetermineView(docId);

but passing a lot of primitives around is a hassle and prone to errors, so sometimes we make a small class to encapsulate the data, in this case the page initialization information.

class ChapterView
{
  public int? GenChapter {get; set;}
  public int? QualChapter {get; set;}
  public int? DocumentId {get; set;}
}

private ChapterView GetChapterView()
{
  return new ChapterView
  {
    GenChapter = ConvertNullable(Request.QueryString["gchap"]),
    QualChapter = ConvertNullable(Request.QueryString["qualchap"]),
    DocumentId = ConvertNullable(Request.QueryString["did"])
  }
}

Note that I've no idea what GenChap and QualChap are, but they're a bit terse and you could complete the refactoring to make them more readable in code. But even without better names, we now have more readable code.

ChapterView chapterView = GetChapterView();

FillQualityList(chapterView);
SetChapterTitle(chapterView);
DetermineView(chapterView);

And finally you may determine that you don't really need to call this every time the page executes to read from the query string. If you've read up on the Asp.Net Page LifeCycle you know that events may change GenChapter or something else that affects how the page is rendered. You may find it better to set up the view in the PreRender instead of calling FillQualityList over and over again.

ChapterView chapterView;

Page_Load()
{
  if( !IsPostback )
  {
    ChapterView chapterView = GetChapterView();
  }
  else
  {
    chapterView = (ChapterView) ViewState["chapterview"];
  }
}

NextChapter_Click()
{
  chaperView.NextChapter();
}

Page_PreRender()
{
  FillQualityList(chapterView);
  SetChapterTitle(chapterView);
  DetermineView(chapterView);}

  // make sure class is marked [Serializable]
  ViewState["chapterview"] = chapterView; 
}