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?

[+theblazingangel MVC]

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:

• 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, ...)

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.

 

 

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.

 

 

On "Performing More Than One Task"

 

Every function/class/file should have a single purpose/concern/responsibility. This does not mean that a function cannot perform more than one action, even if they are unrelated (they should still be related to the purpose of the function).

 

Separation of Concerns (SoC)  is a design principle for separating a computer program into distinct sections, such that each section addresses a separate concern.

http://en.wikipedia.org/wiki/Separation_of_concerns

The Single Responsibility Principle (SRP) "states that every context (class, function, variable, etc.) should have a single responsibility, and that responsibility should be entirely encapsulated by the context. All its services should be narrowly aligned with that responsibility."

http://en.wikipedia.org/wiki/Single_responsibility_principle

 

The example here might also be useful: http://weblogs.asp.net/arturtrosin/separation-of-concern-vs-single-responsibility-principle-soc-vs-srp

 

There is no strict rule on where the boundaries of concern and responsibility lie in your application, that's something you have to figure out for yourself and on a case by case basis.

 

If we go back to your Session_Start event handler for a moment. This event handler is concerned with initialising a fresh session. It is entirely acceptable for it to performs multiple actions (sub-tasks) in doing so. It is not concerned however with the details of them, only getting them done. Attempting to resume a user's persistent login is something which must be done upon this event, it is one sub-task of potentially many. Furthermore you have an entirely separate collection of code concerned with and responsible for authentication related matters. That is where the implementation of the 'resume login persistence' mechanism belonged.

 

Single/Multiple Event Handers

 

The event in question here, the click of a given button, is one for which the language in question only allows one event handler to be registered. However, you may need to execute multiple tasks upon one event...

 

As I've said previously, every function should have one clear purpose/responsibility.

• Where it is possible to register multiple event handlers, one could be registered for each distinct task or small group of related tasks that must be performed upon this event occurring. In which case, the task/purpose/objective/responsibility of each of those event handlers would be to execute the one or more related sub-tasks for which it was specifically created to execute.
• Being able to create only one general event handler, its task/purpose/objective/responsibility is to execute all actions/sub-tasks that need executing upon the given event occurring. To be clear, the task/purpose/objective/responsibility of such a general event handler is to make sure everything is done that needs to be done upon occurrence of this event. This may mean that it has multiple sub-tasks to execute that are completely unrelated to each other.

Note that general purpose of an event handler is to execute actions in response to an event. Proper consideration of the SoC and SRP principles should help you decide for each event handler what code they contain.

 

The benefit of being able to register multiple event handlers is that two entirely unrelated bits of code, both interested in an event, can register a callback (event handler) for it, by themselves, entirely independently, instead of having to hard code calls into one global function.

 

Given only one event handler per event, if you really needed to, you could always try to make up for it by providing your own mechanism. Create a custom event registration mechanism, allow event handlers to be registered to it, and the single native event handler for this event would make a call to that custom event registration mechanism to execute all of the registered event handlers. This was described to you a few posts back. This however is obviously somewhat complex. I'd only try to do this if you really, really need it. Where possible stick to the KISS principle (keep it simple, stupid - for those who haven't heard of it).

 

Your Example :: SoC - Stepping Outside of its Responsibilities?

 

The event handler in your example is part of the presentation layer of your application; business logic is non of its concern, so should definitely go into separate functions, as you've done. So good!

Your Example :: SRP - Doing too Much?

 

Having one single button here is absolutely fine. I would not say that it's doing too much.

 

You described this event handler as performing multiple tasks, perhaps unrelated. I completely disagree with this description. The task/purpose/objective/responsibility of this button is to save any changes to this small group of form elements. The task/purpose/objective/responsibility of this event handler, which has been created very specifically for handling the click of event for this specific button, is exactly the same as that of the button's existence as just described.

 

This is exactly what your event handler is doing. It does not matter that it is doing so in a number of separate actions/sub-tasks.

 

I'm not too sure I like what it's doing in regards to moderator approval however. I'm not following why you've broken this down into two sub-actions, and so I'm not sure I'd agree that this is okay. If you read the wikipedia article on SRP, note the bit about 'reason to change'. Right now you're performing the action of applying requirement for moderator approval on a thread in two separate actions. It appears to me that this is exposing unnecessary detail to the presentation layer, and if you were to later change the "underlying" mechanism for this to use a single step, you're having to modify this "user" of this business logic in quite a significant way. Really the event handler should be calling a single method like the other actions. (Again though, I'm confused about why there are two calls necessary at all here).

 

Your Example :: Business Logic

The title of the thread, the content of the first post and the 'sticky' status here are all distinct components of the thread. It makes sense to break up the business logic (as you have) for changing these individually because it provides support for multiple different interface choices. This can support:

• The interface you described where one button click submits changes to all three at once.
• Changing your mind later, to have individual buttons instead for example, without having to change business logic.
• One page as you described, with a button to save all three at once, and another page where for example clicking on this "sticky" property sends an ajax request to toggle that, with immediate effect (reusable).

That's good design!

 

However, database queries are expensive, so you may find that it's a good idea to also provide one or more alternative business logic functions which combine things into fewer queries. It is best to avoid duplicating functionality like this, but sometimes there's sufficient justification - forcing these things to always be done separately, even where it could be possible to do some together, may have a significant performance penalty. It is possible to take this too far though.

 

Your Example :: Other Logic

 

Your 'TryChangeOriginalPost' function I'm not so sure about. Surely the logic for saving a change to the content of the first post is identical for saving a change to the content of any other post. There's nothing special about a first post that makes updating it significantly different from any other is there? The other data submitted when creating a thread, such as thread title are attached to the thread, not the first post... I wouldn't imagine that you yourself would be duplicating logic in any significant way, you're more likely to be calling some generic 'TryChangePostContent' function with it, but this indirection through 'TryChangeOriginalPost' is surely unnecessary and thus inefficient with the extra function call (unless that gets optimised away by the interpreter and by caching interpreted code)...

 

Action sequence: I would have thought sticky should go last. Surely it should only fail if there's a bug or sticky is not allowed. If it fails, the new title may have gone through, and the new post definitely will, do you really want set sticky failure to block setting a request for mod approval? Surely mod approval is more important; let sticky succeed or fail after that. Perhaps even roll back the new thread/post if setting mod approval required fails? (I'm a little confused about why this user has a choice of whether that gets set though, and why broken into two actions). I'm probably reading far too much into this example though, so perhaps ignore this paragraph

 

Exceptions :: General

 

The reason I do not like to use exceptions for this type of stuff is that all I am doing is logging the error:

Another thing I was taught is do not swallow exceptions.  If you catch exceptions, use throw again.  That will cause the site to give them the yellow screen of death, or a custom error page instead of just displaying a message though.

 

I also do not want to have the yellow screen of death or some generic Error page show up.

 

In general, it's considered very bad practice to allow exceptions to leak out of an application. For example, imagine trying to open a new tab in your web browser, only for it to incur a memory allocation exception. How do you think it is right for it to behave? Should it keep throwing the exception wherever it catches it, if it does at all, resulting in the program terminating and the OS picking up and displaying to the user an "unhandled exception was caught" error, or is it better that it captures the memory allocation exception, and just tries to recover from it as best as possible (it may simply allow anything to do with the attempt to open a new tab to be thrown away, but otherwise nothing).

 

I'll save further discussion on exception usage for another time, this post is large enough as it is, and I've been spending quite a while writing it, a break is in order, but first quickly...

 

Exceptions :: SQL

 

I cannot recover from the SqlException since I need to modify the database

 

What, why? If you're doing a single insert/update/delete type query and it fails with an exception, surely no data has been changed. So catch the exception, and report the failure in a clean way to the user. If you're doing multiple such queries, and if one fails you'd like to undo some or all of those that worked, you need to do them as a transaction. You start a transaction, you issue your queries, if all goes well you issue a commit, and if one or more queries fails, you catch the exception and issue a rollback. Here's a PHP based example overview:

try {
    $db->beginTransaction();

    //multiple queries done here

    $db->commit();
} catch (Exception $e) {
    $db->rollback();
}

 

what is the right way to handle exceptions for cases like this?

 

I think the simple true/false return method your event handler relies upon here makes it pretty neat and tidy. So perhaps that's enough. I'm not totally adverse to exceptions reaching code that called business logic (baring the rule of not throwing exceptions between modules, though that doesn't really apply to a PHP/ASP.net web app), though an sqlException reaching presentation logic... I'm not sure I'm comfortable with that. I'll have to have a think about it when I'm not so tired. I did say I wasn't going to get into this now...

Oh, do you require any of these items being saved to be saved as a transaction? If so, you need a function within your business logic to send these multiple items to at once, which may then in turn use the other functions you've created, or have its own combined sql queries, as discussed before, but most importantly, can wrap them within the transaction you require, along with capturing any exceptions, and returning a more simple and clean true/false for your event/handler.

 

I better stop now, pretty tired...

[simplezz]

If I was to make one suggestion, I would say to move domain/business logic into a separate file/module. And in the case of C#, probably into discrete classes. You can probably mix some data access with domain logic provided it's adhoc. If the data access appears generic and reusable, put it in a separate module/class too. I find it useful to be able to work on individual components this way once an interface is agreed upon.

[Andre S. Veteran]

Seeing as theblazingangel already did a terrific job explaining the principles and I don't have enough insight into your problem domain to make more concrete suggestions, I'll leave it at that for now.

[xWhiplash]

Thanks guys.
 

I am not having any trouble.  This was meant to be language agnostic, but to get my ideas across I posted some sample ASP.NET code.  As I said before, that was just an example.  That was not actual code for a site I am working on, just an example of how a button click would need to do more than one task.

[wrack]

Didn't bother to check all pages. Sorry, in hurry to get home. But wouldn't below work with minor tweaks?

            if (condition1)
            {
                //perform some logic
                if (condition2)
                {
                    //perform some logic
                    if (condition3)
                    {
                        //Perform logic
                        return;
                    }
                }

                //MethodA(param)
                return;
            }

            //MethodA()