Which of the following code blocks are more logical?

Imagine a condintion should be tr开发者_StackOverflow社区ue for a method to do its stuff. Which block represents the best approach (performance related and readability), or if not what is your suggestion?!

private void method()
{
    if(!condition)
    {
     MessageBox.Show("ERROR!");
     return;
    }     
    else
    {
        //DO STUFF
    }
}

OR

private void method()
{
    if(condition)
    {
         //DO STUFF
    }     
    else
    {
         MessageBox.Show("ERROR!");
         return;
    }
}

---

Neither. Use a guard clause instead:

private void method()
{
    if(!condition)
    {
        MessageBox.Show("ERROR!");
        return;
    }     

    //inputs have been checked, proceed with normal execution
}

Done this way you can deal with all the exceptional behaviour up-front and avoiding excessive levels of indentation for the normal execution path.

---

Well, neither, as you wouldn't use both else and return.

So, you would either do:

private void method() {
  if (!condition) {
    MessageBox.Show("ERROR!");
  } else {
    //DO STUFF
  }
}

or:

private void method() {
  if (condition) {
    //DO STUFF
  } else {
    MessageBox.Show("ERROR!");
  }
}

or:

private void method() {
  if (!condition) {
    MessageBox.Show("ERROR!");
    return
  }
  //DO STUFF
}

or:

private void method() {
  if (condition) {
    //DO STUFF
    return;
  }
  MessageBox.Show("ERROR!");
}

Which you use depends mostly on what the code actually does. The code is seldom as simple as in the examples, so it matters what more the code will do.

The first two have the advantage of having a single exit point, which often makes it easier to follow the code. You would usually put the shorter code first, as it's easier to spot there than in an else after a larger code block.

The third is often used to validate input before continuing with the main code, and you can easily have more than one validation:

private void method() {
  if (!condition) {
    MessageBox.Show("ERROR!");
    return
  }
  if (!anotherCondition) {
    MessageBox.Show("ANOTHER ERROR!");
    return
  }
  //DO STUFF
}

The fourth is useful if you have several conditions that you don't want to put in the same if statement:

private void method() {
  if (condition) {
    var data = GetSomeData();
    if (data.IsValid) {
      var moreData = GetSomeMoreData();
      if (moreData.IsValid) {
        //DO STUFF
        return;
      }
    }
  }
  MessageBox.Show("ERROR!");
}

---

Second! Second!

But I do admit to doing the first sometimes if the "//DO STUFF" is really long and nested.

---

I prefer an "If condition" approach as opposed to the negation of a condition, but that's just personal preference.

---

It depends.

In most cases, the second version.

if the amount of code in the (!condition) block is only a few lines of code, and the code in the (condition) block is a LOT of code, then I'd reverse the answer. it's easier to read through the if statement if you can see the "else" without having to scroll.

---

I prefer a guard clause as David mentions, but in the general case you should put the most common case first. It makes it easier to follow the flow of a method.

---

Readability/standards wise. I would accept number 2. I don't think there is a difference performance wise, but I'm not a low-level guy.

---

As usually this is a question which asks for the following answer: "it depends" and I'll show by two examples.
IF NOT CONDITION For the ASP .Net Web Forms validation I'm seeing very often this code

protected void btSubmit_OnClick(object sender, EventArgs e)
{
  Page.Valide();
  if (!Page.IsValid)
     return;
  var customer = new Customer();
  // init 20 properties of customer
 ....
  var bo = new CustomerBO();
  bo.Save(customer);
}

There is another one much more popular:

 protected void Page_Load(object sender, EventArgs e)
        {
            if (!Page.IsPostBack)
                    {
                    }
        }

IF CONDITION

public void UpdateCustomer(int customerId, string ...../*it might be a customer type also*/)
{
   using (var ctx= CreateContext())
 {
    var customer = ctx.Customers.FirstOrDefault(c=>c.CustomerId = customerId);
    if ( customer != null)
    {
           /*code update 20 properties */
    }
 }
}

I hope the code is clear :P

---

This is more of a style question than a "logical" question. Both of these approaches work, and which one you will use generally depends on your style as a thinker/developer.

That said, once you start using either one of these styles, it generally pays to be consistent. Having large classes where some functions do it the first way and others the second way can lead to maintainability concerns later.

Robert Martin's Clean Code presents an interesting chapter on functions that suggests, whichever way you choose, the //DO STUFF part should be another function call

Functions Should Only Do One Thing