Return to Answer

if (this.Parameters == null || this.Parameters.Count == 0)

Is a check this complex actually necessary? It's hard to say for sure without seeing the rest of the class, but it seems like "Parameters is never null" is an invariant that the class could guarantee. For example by initializing to an empty collection in the constructor, making sure nothing else in the class sets it to null, and not exposing a public (or protected) setter so that nothing else can set it to null.

Doing this isn't always possible, but when it is, it does a good deal for reducing the potential of bugs and decreasing cognitive load by reducing the number of possible states you have to worry about the system being in.

As for your potential alternative- yes, it probably is better! In fact if you had ReSharper, it would recommend you did exactly that. It calls it "inverting if statement to reduce nesting".

This probably isn't a completely uncontroversial opinion though, so I'll try to sum up the pros and cons of each:

Pro inversion

• Deeper nesting is generally harder to read
• When reading into a nested if statement, you have to keep a "mental stack" of the conditions that each set of curly braces corresponds to. It has to be a stack so that you can readily "pop" off the last conditional once a set of curly braces ends. By contrast, when an if statement returns, you simply know that that condition is never true for the rest of the method, rather than having to be ready to jump between the true and false case. Cognitively this is much easier.

Anti-inversion

• There's a style point that has lost popularity recently but some people still adhere to that a method should only have one exit point. I guess the reasoning is that having a single exit point corresponds to a single "path" through the code- even if conditionals mean that path can fork. There's some more analysis of this here.

• Your conditional is probably slightly more likely to be a negative one if you do this. So instead of saying "if X then do Y", you're saying "if not X then don't do Y", which is mentally a bit harder to understand. The "don't do Y" part is unavoidable, but the "if not X" bit can be avoided by extraction of your conditional into a well-named method. For example:

   private bool HasParameters()
   {
   return this.Parameters != null && this.Parameters.Count > 0;
   }
  

Some misc points:

• You may have noticed HasParameters has slightly different logic than the line in your second snippet. That's because your one has a bug! In your case, a non-null but empty value for Parameters would have a value of true for this conditional, but it should have false.

• Assuming you're using standard naming conventions, you shouldn't have anything method-scoped whose name starts with a capital letter. So no need to keep writing this.Parameters. Just Parameters would be equally unambiguous.

if (this.Parameters == null || this.Parameters.Count == 0)

Is a check this complex actually necessary? It's hard to say for sure without seeing the rest of the class, but it seems like "Parameters is never null" is an invariant that the class could guarantee. For example by initializing to an empty collection in the constructor, making sure nothing else in the class sets it to null, and not exposing a public (or protected) setter so that nothing else can set it to null.

Doing this isn't always possible, but when it is, it does a good deal for reducing the potential of bugs and decreasing cognitive load by reducing the number of possible states you have to worry about the system being in.

As for your potential alternative- yes, it probably is better! In fact if you had ReSharper, it would recommend you did exactly that. It calls it "inverting if statement to reduce nesting".

This probably isn't a completely uncontroversial opinion though, so I'll try to sum up the pros and cons of each:

Pro inversion

• Deeper nesting is generally harder to read
• When reading into a nested if statement, you have to keep a "mental stack" of the conditions that each set of curly braces corresponds to. It has to be a stack so that you can readily "pop" off the last conditional once a set of curly braces ends. By contrast, when an if statement returns, you simply know that that condition is never true for the rest of the method, rather than having to be ready to jump between the true and false case. Cognitively this is much easier.

Anti-inversion

• There's a style point that has lost popularity recently but some people still adhere to that a method should only have one exit point. I guess the reasoning is that having a single exit point corresponds to a single "path" through the code- even if conditionals mean that path can fork. There's some more analysis of this here.

• Your conditional is probably slightly more likely to be a negative one if you do this. So instead of saying "if X then do Y", you're saying "if not X then don't do Y", which is mentally a bit harder to understand. The "don't do Y" part is unavoidable, but the "if not X" bit can be avoided by extraction of your conditional into a well-named method. For example:

   private bool HasParameters()
   {
   return this.Parameters != null && this.Parameters.Count > 0;
   }
  

Some misc points:

• You may have noticed HasParameters has slightly different logic than the line in your second snippet. That's because your one has a bug! In your case, a non-null but empty value for Parameters would have a value of true for this conditional, but it should have false.

• Assuming you're using standard naming conventions, you shouldn't have anything method-scoped whose name starts with a capital letter. So no need to keep writing this.Parameters. Just Parameters would be equally unambiguous.

if (this.Parameters == null || this.Parameters.Count == 0)

Is a check this complex actually necessary? It's hard to say for sure without seeing the rest of the class, but it seems like "Parameters is never null" is an invariant that the class could guarantee. For example by initializing to an empty collection in the constructor, making sure nothing else in the class sets it to null, and not exposing a public (or protected) setter so that nothing else can set it to null.

Doing this isn't always possible, but when it is, it does a good deal for reducing the potential of bugs and decreasing cognitive load by reducing the number of possible states you have to worry about the system being in.

As for your potential alternative- yes, it probably is better! In fact if you had ReSharper, it would recommend you did exactly that. It calls it "inverting if statement to reduce nesting".

This probably isn't a completely uncontroversial opinion though, so I'll try to sum up the pros and cons of each:

Pro inversion

• Deeper nesting is generally harder to read
• When reading into a nested if statement, you have to keep a "mental stack" of the conditions that each set of curly braces corresponds to. It has to be a stack so that you can readily "pop" off the last conditional once a set of curly braces ends. By contrast, when an if statement returns, you simply know that that condition is never true for the rest of the method, rather than having to be read to jump between the true and false case. Cognitively this is much easier.

Anti-inversion

• There's a style point that has lost popularity recently but some people still adhere to that a method should only have one exit point. I guess the reasoning is that having a single exit point corresponds to a single "path" through the code- even if conditionals mean that path can fork. There's some more analysis of this here.

• Your conditional is probably slightly more likely to be a negative one if you do this. So instead of saying "if X then do Y", you're saying "if not X then don't do Y", which is mentally a bit harder to understand. The "don't do Y" part is unavoidable, but the "if not X" bit can be avoided by extraction of your conditional into a well-named method. For example:

   private bool HasParameters()
   {
   return this.Parameters != null && this.Parameters.Count > 0;
   }
  

Some misc points:

• You may have noticed HasParameters has slightly different logic than the line in your second snippet. That's because your one has a bug! In your case, a non-null but empty value for Parameters would have a value of true for this conditional, but it should have false.

• Assuming you're using standard naming conventions, you shouldn't have anything method-scoped whose name starts with a capital letter. So no need to keep writing this.Parameters. Just Parameters would be equally unambiguous.

if (this.Parameters == null || this.Parameters.Count == 0)

Is a check this complex actually necessary? It's hard to say for sure without seeing the rest of the class, but it seems like "Parameters is never null" is an invariant that the class could guarantee. For example by initializing to an empty collection in the constructor, making sure nothing else in the class sets it to null, and not exposing a public (or protected) setter so that nothing else can set it to null.

Doing this isn't always possible, but when it is, it does a good deal for reducing the potential of bugs and decreasing cognitive load by reducing the number of possible states you have to worry about the system being in.

As for your potential alternative- yes, it probably is better! In fact if you had ReSharper, it would recommend you did exactly that. It calls it "inverting if statement to reduce nesting".

This probably isn't a completely uncontroversial opinion though, so I'll try to sum up the pros and cons of each:

Pro inversion

• Deeper nesting is generally harder to read
• When reading into a nested if statement, you have to keep a "mental stack" of the conditions that each set of curly braces corresponds to. It has to be a stack so that you can readily "pop" off the last conditional once a set of curly braces ends. By contrast, when an if statement returns, you simply know that that condition is never true for the rest of the method, rather than having to be ready to jump between the true and false case. Cognitively this is much easier.

Anti-inversion

• There's a style point that has lost popularity recently but some people still adhere to that a method should only have one exit point. I guess the reasoning is that having a single exit point corresponds to a single "path" through the code- even if conditionals mean that path can fork. There's some more analysis of this here.

• Your conditional is probably slightly more likely to be a negative one if you do this. So instead of saying "if X then do Y", you're saying "if not X then don't do Y", which is mentally a bit harder to understand. The "don't do Y" part is unavoidable, but the "if not X" bit can be avoided by extraction of your conditional into a well-named method. For example:

   private bool HasParameters()
   {
   return this.Parameters != null && this.Parameters.Count > 0;
   }
  

Some misc points:

• You may have noticed HasParameters has slightly different logic than the line in your second snippet. That's because your one has a bug! In your case, a non-null but empty value for Parameters would have a value of true for this conditional, but it should have false.

• Assuming you're using standard naming conventions, you shouldn't have anything method-scoped whose name starts with a capital letter. So no need to keep writing this.Parameters. Just Parameters would be equally unambiguous.