6

What is an elegant way of writing this?

if (lastSelection != null)
{
    lastSelection.changeColor();
}
else
{
    MessageBox.Show("No Selection Made");
    return;
}

changeColor() is a void function and the function that is running the above code is a void function as well.

1
  • Also, anything like this, if there's a chance that lastSelection could change, you should store the current value in a local, or use a null object pattern per my response. Otherwise, you're still risking a null ref exception if lastSelection changes between the 'if' and when you use it.
    – kyoryu
    Mar 14, 2010 at 6:36

7 Answers 7

14

You can reduce clutter by reversing the condition:

if (lastSelection == null)
{
    MessageBox.Show("No Selection Made");
    return;
}

lastSelection.changeColor();
0
9

Although I personally really dislike having multiple return statements in a function, according to the defensive programming I've seen, you'd trap the error condition and exit, and let anything else through:

if(lastSelection == null)
{
    MessageBox.Show("No Selection Made");
    return;
}

lastSelection.changeColor();

It's hard to say how I'd do this in my own work without seeing the entire function this belongs in.

1
  • :-) Two minds entering the same solution at the same time. Yours has more commentary though.
    – Bevan
    Mar 14, 2010 at 3:56
2

I'd be tempted to make sure that lastSelection was never null, and have it point to an "empty" object in cases where it would be null in the current design. Then, there's no need for a null check. Engineering possible errors out of the system is always good practice.

IOW, I'd ask if the inelegance in the code is how you do the null check, or whether it's the fact that a null check is required at all.

6
  • A "null" check may not be required, but this appears to be UI code, and it might be nice to tell the user "No Selection Made" if no selection has been made. In that case, even using the Null Object Pattern, it would be necessary to check for the null object in order to tell the user. Mar 14, 2010 at 4:26
  • 2
    @John Saunders: And then I'd ask why the option to change color even existed if it was not a viable action - in that case, I'd prefer to block the action by disabling the control/whatever before it even got to this code. Alternately, the null object could trigger the message box to appear, directly or indirectly (ex: via an event on the object hooked up to the code in the UI that displays the box, to keep the null object unaware of the UI details).
    – kyoryu
    Mar 14, 2010 at 4:44
  • @John Saunders: (BTW, it may be that there isn't a viable option to avoid the null check - but I'd certainly try to see if there was one).
    – kyoryu
    Mar 14, 2010 at 4:46
  • @kyoryu: I don't understand. This may be a form used to do data entry for a new object. A new object might not have a default color, so the user has to select one. I suppose you could disable the "OK" button until a valid color was selected, but then how do you tell the user why the button is disabled? Mar 14, 2010 at 4:51
  • @John Saunders: Given the fact that the object was called 'lastSelection', I kind of assumed it was more interactive. Without knowing the details more fully, it's really hard to say - we're both speculating here. And, as I've said, it may not be possible to avoid the null. My point is just that I would tend to prefer that solution, and try to achieve it if possible. I would also tend to design UI in such a way that invalid actions cannot be performed. These are obviously not achievable in all cases.
    – kyoryu
    Mar 14, 2010 at 5:22
1

Seeing everybody else got the good answers, DON'T do this:

if (lastSelection == null) goto ERR;
lastSelection.changeColor();
//... possibly more stuff...
return;
ERR:
MessageBox.Show("No Selection Made");
1
  • 1
    Heh. This is good advice. Just to be clear, when I said I dislike multiple return statements, I didn't mean to advocate for gotos in any way! Mar 14, 2010 at 4:51
1

This is yet another way of implementing this.

        Action _action = (lastSelection != null
                              ?
                                  new Action(lastSelection.changeColor)
                              :
                                  () => Console.WriteLine("No Selection Made"));
        _action.Invoke();

Perhaps a bit overkill, but you will certainly have the people maintaining your code scratching their heads :-)

2
  • Reading this code made me itch, but the itch was not in my head :)
    – BillW
    Mar 14, 2010 at 8:58
  • 1
    This code is very clever. Wait, something's wrong... I was sure that clever was a four-letter word...
    – kyoryu
    Mar 14, 2010 at 10:35
0

I think your code is good: you checked for the normal case (lastSelection != null) first then for the abnormal case.

I may just add a single modification:

if (lastSelection != null)
{
    lastSelection.changeColor();
    //your other normal code
}
else
{
    MessageBox.Show("No Selection Made");
    //return; //no need for this now
}
7
  • 1
    @Sameh: your suggestion works if that's all the code there is in the method. But how would you do it if there were 10 copies of the same code, perhaps with each one checking a different variable for null? Mar 14, 2010 at 4:32
  • @John: this is a good point, but his problem seems simple enough to say what I have said. Mar 14, 2010 at 4:48
  • @Sameh: it just seemed to me he didn't show us all his code, and that the code he showed wasn't the only code in the method. I could be wrong about that. Mar 14, 2010 at 4:49
  • @Sameh: Except that he purposefully used only one return statement in which case it can be implied more code exists than is shown, in which case your answer might not be a safe bet. BTW, you don't need the curly braces in your example because comments are ignored.
    – John K
    Mar 14, 2010 at 4:51
  • 1
    Error conditions must be checked first to avoid further scanning.
    – pokrate
    Mar 14, 2010 at 4:52
0

A practice catching up fast is to write null first as below :


if (null == lastSelection)
{
    MessageBox.Show("No Selection Made");
    return;
}

lastSelection.changeColor();

5
  • 5
    @pokrate: "catching up fast"? Man, that's an artifact from back in the days of C, when the compiler wouldn't warn you about doing if (lastSelection = null). That's nearly two decades old! Mar 14, 2010 at 5:05
  • @pokrate, in case you didn't know, you'll actually get a compiler error if you try to assign a single variable in an if in c#. I'm so sad that these kinds of constructs will be all but lost.... Mar 14, 2010 at 5:10
  • Thats a new practice, I have seen in almost all asp.net starter kits and here in StackOverflow. And I am using VS2008 and I cant see any compiler warning too.
    – pokrate
    Mar 14, 2010 at 9:26
  • @overslacked: You are correct, unless the variable is of type bool. if(bFoo = true) will compile, and always execute the block.
    – kyoryu
    Mar 14, 2010 at 10:37
  • "A practice catching up fast" - Somebody from the past has invented a time machine and visited StackOverflow! But all fun aside and to be fair, maybe this is catching on in the microcosm of your team, maybe a code style policy that has recently been allowed.
    – John K
    Mar 14, 2010 at 14:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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