Calendar events based on user feedback

Question

Here's a bit of one of my web forms which is reached by clicking on a link in a calendar. Based on what you click, you end up at this page with a different QueryString. Based on that QueryString, we hide/show and enable/disable controls related to actions associated with the QueryString, as well as the role of user.

We have three different types of events:

• VacationDays (days where people took paid vacation)
• VacationDaysUnpaid (days where people took unpaid vacation)
• CalendarEvents (everything else such as meetings and client visits)

Here's a slimmed down version of how the page is prepared:

protected void Page_Load(object sender, EventArgs e)
{
    if (!IsPostBack)
    {
        InitializePage();
    }
}

private void InitializePage()
{
    CheckRoles();

    if (!String.IsNullOrEmpty(Request.QueryString["CalendarEventID"]))
    {
        if (!HttpContext.Current.User.IsInRole("Guest"))
        {
            //if this is just a regular calendar event, allow non-guests to edit
            btnedit.Enabled = true;
            btndelete.Visible = false;
        }
    }
    if (!String.IsNullOrEmpty(Request.QueryString["VacationDayID"]))
    {
        if (!HttpContext.Current.User.IsInRole("Guest"))
        {
            //for vacation days, edits are not allowed, but the ability to delete is
            btnedit.Visible = false;
            btndelete.Enabled = true;
            btndelete.Visible = true;
        }
    }
    if (!String.IsNullOrEmpty(Request.QueryString["VacationDayUnpaidID"]))
    {
        //unpaid vacation days can only be deleted by administrators
        if (HttpContext.Current.User.Identity.Name.ToString().ToLower() == "administrator")
        {
            btndelete.Visible = true;
        }
        else
        {
            btndelete.Visible = false;
        }
    }
}

private void CheckRoles()
{
    if (HttpContext.Current.User.IsInRole("Guest"))
    {
        //disable all write controls if guest
        btnedit.Visible = false;
        btndelete.Visible = false;
        btnsave.Visible = false;
    }
}

It's jumbled, it's confusing, and it's difficult to maintain. Is there any kind of common design or plan of attack for doing what I'm trying to do here? I mean this works, I can work with it, I guess, but I really feel like there must just be some technique that I don't know about which will help improve this into something much easier and manageable.

One idea that I'm considering, and that I know will work, is to separate the code related to the different types of events into separate files as "partial"

That way in my InitializePage() method, for each type of QueryString, I'll have code that says like EnableCalendarEventMode(), which would be located in the partial class and would contain all the code related to calendar events. This idea will make it easier to manage for me personally, but it still doesn't change the code, it just moves it around.

Answer 1

For one thing, everything is just setting booleans. Condense down all those nested if blocks down into boolean statements and use good names, and it'll even make sense in "plain English".

protected void Page_Load(object sender, EventArgs e) {
    if (IsPostBack) return;
    Func<string, bool> QueryStrHas = key => !String.IsNullOrEmpty(Request.QueryString[key]);
    var isCalEvent = QueryStrHas("CalendarEventID");
    var isVacDay  = QueryStrHas("VacationDayID");
    var isUnpaidVac = QueryStrHas("VacationDayUnpaidID");

    var user = HttpContext.Current.User;
    var isGuest = user.IsInRole("Guest");
    var isAdmin = user.Identity.Name.ToString().ToLower() == "administrator";

    // guests cannot edit
    // non-guests can edit regular calendar events, but not vacation days
    // non-guests can delete vacation days
    // unpaid vacation days can only be deleted by administrators
    btnedit.Enabled = !isGuest && isCalEvent && !isVacDay;
    btndelete.Enabled = !isGuest && isVacDay;
    btndelete.Visible = !isGuest &&
                        (isVacDay||
                        (isAdmin && isUnpaidVac));
}

Make sure you double-check the boolean arithmetic by testing it though. There're some missing cases (kind of), and you should make sure it's all well-defined.

Oh, by the way, just cosmetically "hiding" the btnedit when a user isn't an Admin does not count for true security. Anybody can actually modify all these values when it reaches their browser client-side.

Answer 2

Here's an alternative to the approved answer that provides a different perspective. I try to write self-documenting code so the next developer can understand it without have to think about it. I believe this code achieves that goal. It does end up being more code but when the requirements change it will be easy to make changes to this code.

protected void Page_Load(object sender, EventArgs e)
{
    if (!IsPostBack)
        InitializePage();
}

private void InitializePage()
{
    CheckRoles();
    CheckCalendarEvent();
    CheckVacationDay();
    CheckVacationDayUnpaid();
}

private void CheckRoles()
{
    if (IsGuest())
    {
        //disable all write controls if guest
        btnedit.Visible = false;
        btndelete.Visible = false;
        btnsave.Visible = false;
    }
}

private void CheckCalendarEvent()
{
    if (ParameterExists("CalendarEventID") && !IsGuest())
    {
        //if this is just a regular calendar event, allow non-guests to edit
        btnedit.Enabled = true;
        btndelete.Visible = false;
    }
}

private void CheckVacationDay()
{
    if (ParameterExists("VacationDayID") && !IsGuest())
    {
        //for vacation days, edits are not allowed, but the ability to delete is
        btnedit.Visible = false;
        btndelete.Enabled = true;
        btndelete.Visible = true;
    }
}

private void CheckVacationDayUnpaid()
{
    //unpaid vacation days can only be deleted by administrators
    btndelete.Visible = ParameterExists("VacationDayUnpaidID") && IsAdministrator();
}

private bool IsGuest()
{
    return HttpContext.Current.User.IsInRole("Guest");
}

private bool IsAdminstrator()
{
    return HttpContext.Current.User.Identity.Name.ToString().ToLower() == "administrator";
}

private bool ParameterExists(string p)
{
    return !String.IsNullOrEmpty(Request.QueryString[p]);
}