CODEWITHSUNDEEP
c#
FPGA
FPGA

Reputation: 3855

Checking for a condition in the calling scope or inside the function

i have the following 3 examples which does the same thing

//case1 do it if the condition is valid
private void SetMultiplePropertyValues()
{
    if (Keyboard.GetKeyStates(Key.CapsLock) == KeyStates.Toggled)
    {
        //do somthing
    }
}

//case 2 return if the condition is not valid
private void SetMultiplePropertyValues()
{
    if (Keyboard.GetKeyStates(Key.CapsLock) != KeyStates.Toggled) return;
        //do somthing
}

//case 3 checking the condition in the calling scope
if (Keyboard.GetKeyStates(Key.CapsLock)== KeyStates.Toggled)
    SetMultiplePropertyValues())

private void SetMultiplePropertyValues()
{
    //do somthing
}

which one would you go with and why

Upvotes: 0

Views: 290

Answers (2)

Wagner Leonardi
Wagner Leonardi

Reputation: 4446

First of all, as we can see at code comments, they don't do the same thing. So I think that you're talking about code architecture rather than functionality.

Second, here in SO isn't about giving opinions, but I'll try say to you concrete things about these differences.

1- Common if approach

if (true == false)
{
    return true;
}

vs.

2 - Single line if approach

if (true == false)  return true;

Most of code convetions says to use the option 1, because they're easier to read and understant code, and avoid some mistakes. We need to also understand that convetions are not rules! so they're just convetions, but really try to avoid option 2 in most of the cases.

One more thing, some code convetions also says that's ok using option 2 when you need something very simple, like this given example which is really easy to read and understand. But take this like an exception from the 'rules'.

Upvotes: 0

Eric Lippert
Eric Lippert

Reputation: 660038

They do not do the same thing because in the first two cases the name of the method is a lie; the method name should be SetValuesIfTheKeyStateIsToggled or TryToSetValues or some such thing. Don't say you're going to do a thing and then not do it. More generally: separate your concerns. I would choose a fourth option: 

public void TryToFrob()
{
  if (CanFrob()) DoFrob();
}
private bool CanFrob()
{
  return Keyboard.GetKeyStates(Key.CapsLock) == KeyStates.Toggled;
}
private void DoFrob()
{
  // frob!
}

Notice what is public and what is private.

This is a silly looking example because each one is so simple, but one can easily imagine a situation in which these methods are complex. Keep your policies and your mechanisms logically separated. The mechanism is "is the keyboard in a particular state?" The policy is "I have some conditions under which I can frob; we must never frob unless those conditions are met".

Upvotes: 12

Related Questions