0
\$\begingroup\$

I have a method which creates some default values in an ObservableCollection, nothing too fancy, but I'm trying to decide how to write the if statement.

private void CreateDefaultParameters()
    {
        //If there are no parameters, then create default ones.
        if (this.Parameters == null || this.Parameters.Count == 0)
        {
            this.Parameters = new ObservableCollection<TestResultDefinitionParameter>();

            this.Parameters.Add(new TestResultDefinitionParameter()
            {
                ParameterType = TestResultDefinitionParameter.ParameterTypes.Name
            });

            this.Parameters.Add(new TestResultDefinitionParameter()
            {
                ParameterType = TestResultDefinitionParameter.ParameterTypes.Axis,
                Value = "X"
            });

            //And so on.
        }
    }

The logic is essentially: If it's a new TestResultDefinition, then create default parameters, otherwise load the existing paramters from the database.

public TestResultDefinitionWithParameters(TestResultDefinition resultDefinition)
    {
        this.ResultDefinition = resultDefinition;

        //If it's a new result definition, create the default parameters.
        if (resultDefinition.ID == 0)
            CreateDefaultParameters();
        else
            RefreshParameters();
    }

The if statement could become very large, and I'm wondering if something like this would be better:

private void CreateDefaultParameters()
    {
        if (this.Parameters != null || this.Parameters.Count > 0)
            return;

        //Create parameters here

Would this help readability? Any suggestions?

\$\endgroup\$
  • \$\begingroup\$ Also, include some context — when would this method be called? \$\endgroup\$ – 200_success Mar 18 '15 at 11:58
  • \$\begingroup\$ @200_success The question isn't about the code per se, I'm asking for a review of the readability of a very large if statement, and whether changing it to the 2nd option would increase or decrease readability of code. I couldn't think of a better title to summarize my question. \$\endgroup\$ – Mike Eason Mar 18 '15 at 12:05
  • 2
    \$\begingroup\$ So you want to take an A/B popularity poll? That's not a Code Review question. \$\endgroup\$ – 200_success Mar 18 '15 at 12:07
  • \$\begingroup\$ @200_success My question is a comparison between two methods of implementation, in the attempt to improve readability. Perhaps I made the wrong assumption that I am allowed to ask "Can I improve readability?" questions after reading through similar questions like this, and this. \$\endgroup\$ – Mike Eason Mar 18 '15 at 12:18
  • \$\begingroup\$ How big is your set of parameters? Are any of them essential, without which the collection is useless? If so, what proportion (some/all)? \$\endgroup\$ – itsbruce Mar 18 '15 at 12:23
2
\$\begingroup\$
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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This is a beyond excellent answer. Thank you. \$\endgroup\$ – Mike Eason Mar 18 '15 at 12:45
  • \$\begingroup\$ @MikeEason Glad it was helpful! \$\endgroup\$ – Ben Aaronson Mar 18 '15 at 13:07
0
\$\begingroup\$

Unless there is a need to create all the parameters at the same time, in C# you can have lazy initialization, which looks very nice and helps with your problem.

I am going to make some assumptions in my answer but a nice solution is the following

class LazyClass
{
    private Lazy<TestResultDefinitionParameter> _testResult;
    public TestResultDefinitionParameter TestResult
    {
           get { return _testResult.Value; }
    }

    public LazyClass()
    {
        _testResult = new Lazy<TestResultDefinitionParameter>( 
            () => 
            { 
                return new TestResultDefinitionParameter(); 
            }
        );
    }
}

When you use TestResultDefinitionParamter the object will be allocated if it does not exist, otherwise returned. No if's, eh?

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

Your second option is a little more legible than the first but the best way to improve both the legibility and the clarity of your code is to completely separate the code which generates the default parameters from the logic which decides whether or not to add the default parameters.

Consider the name of your method: CreateDefaultParameters. This name is misleading. It will not always do what its name declares.

Another consideration is that you have mixed different levels of abstraction in this method (the low level work of generating each parameter and the higher level logic). This is only made more clear if you take Ben Aaronson's good advice and create a function to check the parameter status.

My advice would be to

  1. Add a DefaultParameters method which returns the set of default parameters.
  2. Add a HasNoParameters method. If you like, it could simply return ! HasParameters (or HasParameters could return the negation of hasNoParameters
  3. Change CreateDefaultParameters to a method which takes the output of DefaultParameters and inserts it into the collection. You might want it to clear the existing contents first.
  4. Change the line in your code which calls CreateDefaultParameters to this:

    if (HasNoParameters()) CreateDefaultParameters();
    

I think this wins on several points:

  1. The name of the CreateDefaultParameters becomes entirely honest. (Although SetDefaultParameters may be a better name).
  2. The code in that method becomes clearer
  3. Separation of higher level abstraction from lower. Clarity and simplicity at the higher level.
  4. Adds flexibility extensibility.

To explain point 4

  • You can change the logic which decides whether to set default parameters without having to edit the code which does set them. For example, if a minimum good set of parameters is necessary, you might want to use a HasSufficientParameters or HasAcceptableParameters check instead of plain old HasParameters.
  • You could compare the existing parameters with the default parameter set.
  • If only a partial set of parameters has been added, you could create a workable set from the union of the default parameters and the ones which have been added (only adding those default parameters which are missing).
  • The default parameter set is available for tests.
  • The default parameter set could be separated from the main class's code and injected. More flexibility, more possibility of code re-use, even better for tests. The default set becomes a configuration option.

The one drawback is that it does create the risk of accidentally replacing a good, user-added set of parameters with the default. But

  1. This is a private method
  2. You can create tests to make sure that doesn't happen.
  3. It's an ObservableCollection. You can track everything ;)
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.