Avoid code Repetition in conditions

[xWhiplash]

Hello,

 

I have been programming for over 15 years now.  I consider myself a very good programmer, but I understand (like all of us) there are things that I need to work on.  One of these things is code repetition when dealing with conditions.  I will give a generic sample:

if(condition1)
{
     //perform some logic
     if(condition2)
     {
          //perform some logic
          if(condition3)
          {
               //Perform logic
          }
          else
          {
               //MethodA(param)
          }
     }
     else
     {
          //MethodA(param)
     }
}
else
{
     //MethodA()
}

Now, I cannot make it easy by doing the following:

if(condition1 && condition2)
{

}
else
{

}

I cannot do this since I need to perform some logic if condition1 is true and before I test condition2.

 

Is there a way to structure if...else blocks to where if you need to call a method in each else blocks, you are not repeating yourself?

[seta-san]

Hello,

 

I have been programming for over 15 years now.  I consider myself a very good programmer, but I understand (like all of us) there are things that I need to work on.  One of these things is code repetition when dealing with conditions.  I will give a generic sample:

if(condition1)
{
     //perform some logic
     if(condition2)
     {
          //perform some logic
          if(condition3)
          {
               //Perform logic
          }
          else
          {
               //MethodA(param)
          }
     }
     else
     {
          //MethodA(param)
     }
}
else
{
     //MethodA()
}

Now, I cannot make it easy by doing the following:

if(condition1 && condition2)
{

}
else
{

}

I cannot do this since I need to perform some logic if condition1 is true and before I test condition2.

 

Is there a way to structure if...else blocks to where if you need to call a method in each else blocks, you are not repeating yourself?

do a case statement with fall through.

[Andre S. Veteran]

do a case statement with fall through.

C# does not support case statement fallthrough except for empty cases.

[+theblazingangel MVC]

do a case statement with fall through.

 

Aside from the issue Andre points out above, I can't see how on earth a switch statement could possibly be used as a replacement for the nested if structure you're quoting. And I'm dismayed that you've not been the only person to suggest it

 

Just for fun, I had a go and came up with this, but it only uses a single case, no fall through which you seem to specifically think is necessary, and it's absolutely horrendous:

protected void Session_Start(object sender, EventArgs e
{    
    bool userIDSupplied = false;
    int status = 0;
    switch(status) {
    case 0:
        if (cookie == null)
            break;

        int userID;
        string token = cookie["Token"];
        if (!int.TryParse(cookie["UserID"], out userID) || string.IsNullOrEmpty(token) || !Authentication.IsAuthenticated(userID, token))
            break;

        userIDSupplied = true;
        Account account = new Account(userID);
        if (account == null || !account.Login())
            break;
        Session.Add("Account", account);
    };

    if (userIDSupplied)
        ResetAuthentication(userID);
    else
        ResetAuthentication();
}

[+Majesticmerc MVC]

Thanks.  When you say I have grouped too much, is that just too much for a single method to do

 

In my opinion, yes. You're better off breaking that single large method down a bit. I've highlighted your original Session_Start function to emphasise the distinct operations...

 

 

Red = Get data from cookie

Green = Validate cookie data

Blue = Retrieve Account Information.

 

So we could try to refactor your code like this (bear in mind that this one one of many options, everyone's responses in the thread so far have been perfectly valid):

 

 

(Spoiler below contains un-annotated code)

protected void Session_Start (object sender, EventArgs e)
{
    if (!TryRestoreAccountFromPersistentLogin())
    {
        RedirectToLoginPage();
    }
}

bool TryRestoreAccountFromPersistentLogin()
{
    int userID;
    string token;
    
    if (!TryGetPersistentLoginInfo(out userID, out token))
        return false;
        
    if (!Authentication.IsAuthenticated(userID, token))
        return false;
        
    Account account = new Account(userID);
    if (account.Login())
    {
        HttpContext.Current.Session.Add(account);
        return true;
    }
    else
    {
        RemoveAuthenticationRecord(userID);
        return false;
    }
}

bool TryGetPersistentLoginInfo(out int userID, out string token)
{
    // Check persistent login token
    HttpCookie cookie = Request.Cookies[Properties.Settings.Default.AuthenticationCookieName];
    
    if (!int.TryParse(cookie["UserID"], out userID))
        return false;
        
    token = cookie["Token"];
    if (string.IsNullOrEmpty(token))
        return false;
        
    return true;
}

 

A few things to note in this refactored code:

• The Session_Start method is now very easy to read and understand.
• We have entirely removed (almost) all comments from the code, the code is now self-documenting.
• All code is grouped by colour, and the cookie retrieval code has been moved into a separate method. This enforces the "Single-responsibility principle" by stopping us mixing the cookie code and the authentication code.
• I have removed the overloads for "ResetAuthentication()", and replaced it with two separate methods. RedirectToLoginPage() and RemoveAuthenticationRecord(int). Overloads for functions should only really be used when they perform the exact same task with different inputs. By giving two distinctly different methods the same name, it makes your code more confusing, and requires you to add comments to your code.
• Note that there is still repetition in the code, however this repetition only exists to provide a return value, and, in my opinion, is much better than repeating a function call many times as this could potentially have side-effects.

Notice that when I say "each method should only do one task", I don't mean that the code should only do "one thing". In my refactored code above, the TryRestoreAccountFromPersistentLogin() method does four things...

• it get's the persistent login data out of the cookie
• it authenticates the cookie data
• it restores the account
• It does account login to verify account is not banned, etc.

It does these four things in order to perform a single task, which is to restore the account. It's worth noting that you already did a good job of breaking your code down. The authentication and account login parts are about as good as they're going to get (as far as I can tell from this small sample, at least), it's just the cookie stuff and use of return codes that improve on your original design :).

[+theblazingangel MVC]

^ this is a fantastic example of how I'd like to see this implemented!

 

You're sacrificing a tiny bit of performance with extra function calls compared to the original, but sometimes, like here, it's worth it. This example sticks perfectly to the principle of separation of concerns we talked about. It's very readable, it's very maintainable, and it's very re-usable.

 

The only tiny change I'd make is to move these new functions (TryRestoreAccountFromPersistentLogin and TryGetPersistentLoginInfo) into a separate file, again on the principles of separation of concerns and re-usability - this global.asax file is not authentication specific; these functions, which contain implementations of authentication routines, not simply calls to authentication methods like Session_Start, belong in a file dedicated to authentication. This would help this code to be re-used in other projects as I described previously.

 

I'm a little concerned with your code having been posted only as image attachments @Majesticmerc; I appreciate the highlighting, but I'm worried about the images getting lost over time. I think that this particular code is very valuable to this thread and something that worth preserving for any future readers. Would you mind please posting a copy in text form also?

[bitslasher]

Hello,

 

I have been programming for over 15 years now.  I consider myself a very good programmer, but I understand (like all of us) there are things that I need to work on.  One of these things is code repetition when dealing with conditions.  I will give a generic sample:

if(condition1)
{
     //perform some logic
     if(condition2)
     {
          //perform some logic
          if(condition3)
          {
               //Perform logic
          }
          else
          {
               //MethodA(param)
          }
     }
     else
     {
          //MethodA(param)
     }
}
else
{
     //MethodA()
}

Now, I cannot make it easy by doing the following:

if(condition1 && condition2)
{

}
else
{

}

I cannot do this since I need to perform some logic if condition1 is true and before I test condition2.

 

Is there a way to structure if...else blocks to where if you need to call a method in each else blocks, you are not repeating yourself?

 

Hi started reading this and it seems to have deviated to different code example than the one you gave originally, so I'm not sure your actual question was answered.  So the answer to your original question is yes, it can be simplified.  I often will do something like this, YMMV:

bool runMethodAFlag = true

if(condition1)
{
    //perform some logic
    if(condition2)
    {
        //perform some logic
        if(condition3)
        {
           //Perform logic
           runMethodAFlag = false
        }
}

if (runMethodAFlag)
{
    //MethodA(param)
}

[Praetor]

Hello,

why not using a CASE condition?

[Andre S. Veteran]

Hello,

why not using a CASE condition?

How would that improve anything?

[Crimson Rain]

Is repeating a method call a few times bad?  I just try to avoid as much code repetition as possible.

This.

Repeating same lines of code is not good.

Repeating same method calls is just fine.

[xWhiplash]

 

In my opinion, yes. You're better off breaking that single large method down a bit. I've highlighted your original Session_Start function to emphasise the distinct operations...

 

xWhipLashCodeAnnotation.png

 

Red = Get data from cookie

Green = Validate cookie data

Blue = Retrieve Account Information.

 

So we could try to refactor your code like this (bear in mind that this one one of many options, everyone's responses in the thread so far have been perfectly valid):

 

xWhipLashCodeBetterAnnotation.png

 

A few things to note in this refactored code:

• The Session_Start method is now very easy to read and understand.
• We have entirely removed (almost) all comments from the code, the code is now self-documenting.
• All code is grouped by colour, and the cookie retrieval code has been moved into a separate method. This enforces the "Single-responsibility principle" by stopping us mixing the cookie code and the authentication code.
• I have removed the overloads for "ResetAuthentication()", and replaced it with two separate methods. RedirectToLoginPage() and RemoveAuthenticationRecord(int). Overloads for functions should only really be used when they perform the exact same task with different inputs. By giving two distinctly different methods the same name, it makes your code more confusing, and requires you to add comments to your code.
• Note that there is still repetition in the code, however this repetition only exists to provide a return value, and, in my opinion, is much better than repeating a function call many times as this could potentially have side-effects.

Notice that when I say "each method should only do one task", I don't mean that the code should only do "one thing". In my refactored code above, the TryRestoreAccountFromPersistentLogin() method does four things...

• it get's the persistent login data out of the cookie
• it authenticates the cookie data
• it restores the account
• It does account login to verify account is not banned, etc.

It does these four things in order to perform a single task, which is to restore the account. It's worth noting that you already did a good job of breaking your code down. The authentication and account login parts are about as good as they're going to get (as far as I can tell from this small sample, at least), it's just the cookie stuff and use of return codes that improve on your original design :).

 

That is great!  Thanks.  Excellent example for how to organize a lengthy method!

 

So if there is a lengthy method that does not need any results, should you split them up to several void methods and just call them one-by-one?

[REM2000]

This.

Repeating same lines of code is not good.

Repeating same method calls is just fine.

 

I agree with this because i always work under the assumption that if there is a bug in a set of lines of code then you have to remember to correct all repeating lines of code, however calling the same method means you only have to maintain the single method. 

 

Thats how i code my apps, 

[+Majesticmerc MVC]

I'm a little concerned with your code having been posted only as image attachments @Majesticmerc; I appreciate the highlighting, but I'm worried about the images getting lost over time. I think that this particular code is very valuable to this thread and something that worth preserving for any future readers. Would you mind please posting a copy in text form also?

 

Good shout, I've added the text version of the code into the spoiler box :)

[Praetor]

How would that improve anything?

 

ups, my bad: i meant a switch + case.

[+Majesticmerc MVC]

So if there is a lengthy method that does not need any results, should you split them up to several void methods and just call them one-by-one?

 

If it makes the code easier to read, yes I believe so. Consider the following example from some code I wrote a long time ago (forgive my abuse of all sensible conventions, I was a young and arrogant Padawan at the time):

 

protected void ParseIncomingReport(HIDDeviceReport Report)
{
    switch ((InputReportType)Report.Type)
    {
        //Do nothing for acknowledgements
        case InputReportType.Acknowledgement:
            break;

        //Incoming button data
        case InputReportType.CoreButtons:
            {
                WiiRemoteButtons oldButtonLayout = new WiiRemoteButtons(buttons);
                this.buttons = ParseButtonsInfo(Report);

                //Raise events
                if (ButtonStateChanged != null) ButtonStateChanged(Report, new ButtonChangedEventArgs(buttons, oldButtonLayout));

                break;
            }

        //Incoming buttons and accelerometer data
        case InputReportType.CoreButtonsAccel:
            {
                WiiRemoteButtons oldButtonLayout = new WiiRemoteButtons(buttons);
                this.buttons = ParseButtonsInfo(Report);
                this.accelData.SetLastReading(ParseAccelerometerReading(Report));

                //Raise events
                if (ButtonStateChanged != null) ButtonStateChanged(Report, new ButtonChangedEventArgs(buttons, oldButtonLayout));
                if (AccelerationChanged != null) AccelerationChanged(Report, new AccelerationChangedEventArgs(accelData.LastReading));

                break;
            }

        //Buttons, Accelerometer, and 16 extension bytes
        case InputReportType.CoreButtonsAccelExten16:
            {
                WiiRemoteButtons oldButtonLayout = new WiiRemoteButtons(buttons);
                this.buttons = ParseButtonsInfo(Report);
                this.accelData.SetLastReading(ParseAccelerometerReading(Report));
                //EXTENSION PARSING HERE!

                //Raise events
                if (ButtonStateChanged != null) ButtonStateChanged(Report, new ButtonChangedEventArgs(buttons, oldButtonLayout));
                if (AccelerationChanged != null) AccelerationChanged(Report, new AccelerationChangedEventArgs(accelData.LastReading));
                //EXTENSION EVENT HERE

                break;
            }
            
        ... many more cases...
    }
}

This is (part of) a method I wrote in C# for a class that interfaces with a Nintendo Wii remote. It's 181 lines in total, and while fairly readable, it could be improved easily by forwarding each case to another void method, like this:

protected void ParseIncomingReport(HIDDeviceReport Report)
{
    switch ((InputReportType)Report.Type)
    {
        case InputReportType.Acknowledgement:
            ProcessAcknowledgement();
            break;

        case InputReportType.CoreButtons:
            ProcessCoreButtonsReport(Report);
            break;

        case InputReportType.CoreButtonsAccel:
            ProcessCoreButtonsAccelReport(Report);
            break;

        case InputReportType.CoreButtonsAccelExten16:
            ProcessCoreButtonsAccelExten16Report(Report);
            break;
            
        ... many more cases...
    }
}

By delegating the actual processing of the "Report" parameter, the method becomes much shorter. Short methods/functions are easier to design, easier to maintain, and much easier to read.

[xWhiplash]

 

If it makes the code easier to read, yes I believe so. Consider the following example from some code I wrote a long time ago (forgive my abuse of all sensible conventions, I was a young and arrogant Padawan at the time):

 

protected void ParseIncomingReport(HIDDeviceReport Report)
{
    switch ((InputReportType)Report.Type)
    {
        //Do nothing for acknowledgements
        case InputReportType.Acknowledgement:
            break;

        //Incoming button data
        case InputReportType.CoreButtons:
            {
                WiiRemoteButtons oldButtonLayout = new WiiRemoteButtons(buttons);
                this.buttons = ParseButtonsInfo(Report);

                //Raise events
                if (ButtonStateChanged != null) ButtonStateChanged(Report, new ButtonChangedEventArgs(buttons, oldButtonLayout));

                break;
            }

        //Incoming buttons and accelerometer data
        case InputReportType.CoreButtonsAccel:
            {
                WiiRemoteButtons oldButtonLayout = new WiiRemoteButtons(buttons);
                this.buttons = ParseButtonsInfo(Report);
                this.accelData.SetLastReading(ParseAccelerometerReading(Report));

                //Raise events
                if (ButtonStateChanged != null) ButtonStateChanged(Report, new ButtonChangedEventArgs(buttons, oldButtonLayout));
                if (AccelerationChanged != null) AccelerationChanged(Report, new AccelerationChangedEventArgs(accelData.LastReading));

                break;
            }

        //Buttons, Accelerometer, and 16 extension bytes
        case InputReportType.CoreButtonsAccelExten16:
            {
                WiiRemoteButtons oldButtonLayout = new WiiRemoteButtons(buttons);
                this.buttons = ParseButtonsInfo(Report);
                this.accelData.SetLastReading(ParseAccelerometerReading(Report));
                //EXTENSION PARSING HERE!

                //Raise events
                if (ButtonStateChanged != null) ButtonStateChanged(Report, new ButtonChangedEventArgs(buttons, oldButtonLayout));
                if (AccelerationChanged != null) AccelerationChanged(Report, new AccelerationChangedEventArgs(accelData.LastReading));
                //EXTENSION EVENT HERE

                break;
            }
            
        ... many more cases...
    }
}

This is (part of) a method I wrote in C# for a class that interfaces with a Nintendo Wii remote. It's 181 lines in total, and while fairly readable, it could be improved easily by forwarding each case to another void method, like this:

protected void ParseIncomingReport(HIDDeviceReport Report)
{
    switch ((InputReportType)Report.Type)
    {
        case InputReportType.Acknowledgement:
            ProcessAcknowledgement();
            break;

        case InputReportType.CoreButtons:
            ProcessCoreButtonsReport(Report);
            break;

        case InputReportType.CoreButtonsAccel:
            ProcessCoreButtonsAccelReport(Report);
            break;

        case InputReportType.CoreButtonsAccelExten16:
            ProcessCoreButtonsAccelExten16Report(Report);
            break;
            
        ... many more cases...
    }
}

By delegating the actual processing of the "Report" parameter, the method becomes much shorter. Short methods/functions are easier to design, easier to maintain, and much easier to read.

 

 

Even if I do not do anything else but call methods, is that still good?

public void MethodA()
{
     CallMethodB();
     CallMethodD();
     CallMethodE();
     CallMethodF();
     CallMethodG();
     //More
}

[simplezz]

Even if I do not do anything else but call methods, is that still good?

public void MethodA()
{
     CallMethodB();
     CallMethodD();
     CallMethodE();
     CallMethodF();
     CallMethodG();
     //More
}

As others have mentioned, if calling all those functions constitutes a single related operation, then yes that's fine. You can even mix code as well as delegating subtasks as long as they all work towards the same goal.

 

There are some exceptions to that rule, such as entry point / main functions, which often do more varied work, but if you generally try and keep all related code in the same module / class, then it'll be easier to read and maintain.

 

Think along the lines of responsibilities. What module or class is responsible for performing this kind of work, etc. For example, the C code I posted was part of a program to read and parse block device partition tables and display that relevant information in a GUI or in the terminal. It's completely modular. This allows me to either build a no-gui or gui version:

 

build-nogui.sh
gcc ui_fmt.c block_dev.c fs.c partition.c output.c -o disks-nogui -O2 -s -std=c99 -pedantic -Wall

 or

build-gui.sh
gcc ui_fmt.c block_dev.c fs.c partition.c disk_layout.c ui.c -o disks-gui -O0 -g -std=c99 -pedantic -Wall `pkg-config --cflags --libs gtk+-3.0`

 

The only difference is, I created two distinct UI entry point files, ui.c and output.c. I could even setup an automake macro to do the same thing, then pass the option to ./configure --enable-gui

 

The point being, if you make your program highly modular, it yields many benefits, such as readability, reusability, and easy maintenance.

[+Majesticmerc MVC]

 

Even if I do not do anything else but call methods, is that still good?

public void MethodA()
{
     CallMethodB();
     CallMethodD();
     CallMethodE();
     CallMethodF();
     CallMethodG();
     //More
}

 

Within reason, sure. It's more of a judgement call than a rule. I mean, something like this would be fine:

class Car
{

public void TurnOnAirConditioning()
{
    DisableBasicFan();
    StartCoolantPump();
    StartTemperatureMonitoring();
    UpdateDashbordLights();
}

}

However, something like this I would consider to be poor style, and possibly a code smell:

class ExampleClass
{

public void DoStuff()
{
    DoStuffPart1();
    DoStuffPart2();
    DoStuffPart3();
    DoStuffPart4();
}

}

In the second example, DoStuffPart1() to DoStuffPart4() are all part of a single DoStuff method that does not have any logical breakdown into functions. In this case, it's probably better to just have it as a single larger function. In the first example however, turning on the air conditioning in your car is likely to require several smaller tasks, therefore it's logical to perform each task in it's own function.

[Andre S. Veteran]

 

Even if I do not do anything else but call methods, is that still good?

public void MethodA()
{
     CallMethodB();
     CallMethodD();
     CallMethodE();
     CallMethodF();
     CallMethodG();
     //More
}

If these are private methods that are only ever called from that function, then the only thing you've really achieved is improve the readability of MethodA(). You're not improving testability or composability: all these small methods are really designed to be called in this order in this specific method and probably can't be re-used in another way; furthermore, they're all private and don't take or return parameters, so they're as untestable as a method can be.

 

So, just splitting a large method into small chunks doesn't necessarily achieve much, although it may be one step towards a more significant refactor.

[xWhiplash]

If these are private methods that are only ever called from that function, then the only thing you've really achieved is improve the readability of MethodA(). You're not improving testability or composability: all these small methods are really designed to be called in this order in this specific method and probably can't be re-used in another way; furthermore, they're all private and don't take or return parameters, so they're as untestable as a method can be.

 

So, just splitting a large method into small chunks doesn't necessarily achieve much, although it may be one step towards a more significant refactor.

 

Okay.  What about button events that do not perform a similar task but they need to be done when the button is clicked?

 

I will try to give an example.  Lets say I write up a way to edit the first post of a topic and there is only one button.  I want the click event to handle:

 

1. Changing the title of the thread (Thread in database)
2. Changing the contents of the original post (Post in database that matches the Thread ID and is the first post)
3. If the check box is checked, make the thread sticky (Thread in the database)
4. if the check box is checked, send the thread back to moderator approval (Thread in the database, ModeratorNotifications in the database, ...)

There might be a few cases where a button event has to do twice the amount of what I listed.  I am sure we have all been there where we have a button or something that HAS to do various things.  Is that okay to do?  It is performing more than one task (it needs to modify topic and initial post).  It is bad to have 4-8+ buttons on the page though.  What are your thoughts?

 

Sorry for the bad example, I couldn't think of anything at the moment.  But I am sure you know what I am asking here.  A single button that needs to perform several tasks (maybe entirely unrelated).  It would be bad UI design to just have buttons everywhere.

[Andre S. Veteran]

Okay.  What about button events that do not perform a similar task but they need to be done when the button is clicked?

 

A single button that needs to perform several tasks (maybe entirely unrelated).  It would be bad UI design to just have buttons everywhere.

Well, let's clear up some terminology first. Events don't perform tasks, they signal that something happened. A button click event is a signal that this button was clicked. Any particular event may be observed (i.e. have event handlers registered) by one or more different objects; these objects might react in different ways to this event. If you find that one class is doing all sorts of unrelated stuff with complicated logic in response to an event, then maybe you need to split responsibilities between multiple classes that all can watch that particular event.

[xWhiplash]

Well, let's clear up some terminology first. Events don't perform tasks, they signal that something happened. A button click event is a signal that this button was clicked. Any particular event may be observed (i.e. have event handlers registered) by one or more different objects; these objects might react in different ways to this event. If you find that one class is doing all sorts of unrelated stuff with complicated logic in response to an event, then maybe you need to split responsibilities between multiple classes that all can watch that particular event.

 

You can't do that with ASP.NET Button OnClick event since it is tied to the page.  It calls one method, on that page's code behind.

[simplezz]

• Changing the title of the thread (Thread in database)
• Changing the contents of the original post (Post in database that matches the Thread ID and is the first post)
• If the check box is checked, make the thread sticky (Thread in the database)
• if the check box is checked, send the thread back to moderator approval (Thread in the database, ModeratorNotifications in the database, ...)

These operations all seem to involve database access and domain logic. That makes them a good candidate for independent domain logic and data access layers. It enforces separation of concerns, modularises the program, and allows you to reuse the code.

Your presentation layer, which involves handling UI events, should call into your domain layer, have it perform specific site logic, then return the results for the presentation to render.

 

There might be a few cases where a button event has to do twice the amount of what I listed.

Delegate domain logic to the relevant layer, and keep the UI entry point functions for presentation logic and calling domain routines only.

https://en.wikipedia.org/wiki/Business_logic#Business_logic_and_tiers.2Flayers

[tiagosilva29]

Boy, this thread escalated rather quickly.

[Andre S. Veteran]

You can't do that with ASP.NET Button OnClick event since it is tied to the page.  It calls one method, on that page's code behind.

That's not a big problem, your code behind class can expose an event that gets called on the actual OnClick event handler and then other objects can listen to that event instead. Anyway it's usually not a good idea to have the UI implementing the logic of your application, have you no separation between your view and business logic?

[xWhiplash]

That's not a big problem, your code behind class can expose an event that gets called on the actual OnClick event handler and then other objects can listen to that event instead. Anyway it's usually not a good idea to have the UI implementing the logic of your application, have you no separation between your view and business logic?

 

But what if I need to test for the page control states?  Of course I have separation, but my view does need to have some code in it.

 

I cannot make other objects listen to an edit event if I need to access certain page controls that other objects would not be able to see.

 

For example:

protected void EditBtn_Click(Object sender, EventArgs e)
{
     int threadID = Convert.ToInt32(Session["ThreadID"]);
     if(!Thread.TryChangeTitle(threadID))
     {
          EditStatus.Text = "Could not change the title of this thread.";
     }
     
     if(!Thread.TryChangeOriginalPost(threadID, PostContents.Text))
     {
          EditStatus.Text = "Could not change the original post for this thread.";
          return;
     }

     if(ThreadSticky.Checked && !Thread.TryMakeSticky(threadID))
     {
          EditStatus.Text = "Could not make this thread sticky";
          return;
     }

     if(RequireModeratorApproval.Checked)
     {
          if(!Thread.TryRequireModeratorApproval(threadID))
          {
               EditStatus.Text = "Could not send the thread to moderator approval;
               return;
          }
          else
          {
               ModeratorNotification.NewThreadApproval(threadID, DateTime.Now, loggedInUser.ID);
          }
     }
     
}

Also, what is the right way to handle exceptions for cases like this?  Like the SqlException since all the above will modify the database.  I cannot recover from the SqlException since I need to modify the database.  I also do not want to have the yellow screen of death or some generic Error page show up.