CODEWITHSUNDEEP
c#performancebooleanxna
Bruno Filip
Bruno Filip

Reputation: 139

Is this kind of a bool method a bad practice?

Sometimes I find myself writing a bool method that looks like this:

public bool isRunning()
    {
        if (!(move == Moving.None) && staminaRegan == true)
        {
            if (keyState.IsKeyDown(Keys.Space))
            {
                EntityAnimation.interval = 10;
                return true;
            }
            else
            {
                EntityAnimation.interval = 65;
                return false;
            }
        }
        else 
        {
            EntityAnimation.interval = 65;
            return false;
        }
    }

(This is XNA by the way) As you can see, I have a bool isRunning in which I made an if statement where Im checking if (Player is moving) && (regains stamina, which is set to false once stamina reaches value lesser than 6.0f) and then I simply check if Space is pressed, if yes then my Animation is faster(the smaller the interval, the faster is spritesheet changing), and then It sends true value, which means that Player is running, else Im not cause Space is not pressed.

And then I have to repeat this 'else' code outside of the first if statement so it sends that Player is not running if Player is not moving or his stamina Regan is false;

So I was just wondering is this kind of a bool method considered a bad practice(where you retrun true and false value in nested if, and then return false outside nested if and repeat the same code) ?

Upvotes: 3

Views: 825

Answers (5)

Fabio
Fabio

Reputation: 32445

I think we write code for people(other developers), of course machine execute a code but 80% of developer's work is reading the code.
Based on that I think flow of reading must be exactly same as flow of executing code - that's why I think multiply return statement not a bad thing, even better then only one return statement on the bottom of your method.

Upvotes: 1

Dmitrii Bychenko
Dmitrii Bychenko

Reputation: 186668

The method has a side effect, that's why it's a bad practice:

 public bool isRunning()

When looking on method's signature we expect just true/false answer and nothing more. However, the method changes the instance's state:

  ...
  if (!(move == Moving.None) && staminaRegan == true)
    {
        if (keyState.IsKeyDown(Keys.Space))
        {
            EntityAnimation.interval = 10; // <- Aaa! The interval is changed 
            return true;
        }
  ...

I suggest splitting the initial method into a property and a method

 // No side effect: just answer is running or not
 public bool IsRunning {
   get {
     return (move != Moving.None) && staminaRegan && KeyState.IsKeyDown(Keys.Space);
   }
 }

 // Put the right interval based on instance internal state 
 // (if it's running etc.)
 public void AdjustInterval() {
   if (IsRunning) // and may be other conditions
     EntityAnimation.interval = 10; //TODO: move magic number into constant 
   else 
     EntityAnimation.interval = 65; //TODO: move magic number into constant
 }

Upvotes: 8

Matthew Watson
Matthew Watson

Reputation: 109557

You can rewrite the code as follows; then the code isn't repeated:

public bool isRunning()
{
    if (move != Moving.None && staminaRegan && keyState.IsKeyDown(Keys.Space))
    {
        EntityAnimation.interval = 10;
        return true;
    }
    else
    {
        EntityAnimation.interval = 65;
        return false;
    }
}

Or if you don't want the redundant else:

public bool isRunning()
{
    if (move != Moving.None && staminaRegan && keyState.IsKeyDown(Keys.Space))
    {
        EntityAnimation.interval = 10;
        return true;
    }

    EntityAnimation.interval = 65;
    return false;
}

I would consider introducing a named boolean to self-document somewhat, and I'd rename staminaRegan to staminaIsRegenerating

public bool isRunning()
{
    bool isMovingQuickly = (move != Moving.None) && staminaIsRegenerating && keyState.IsKeyDown(Keys.Space);

    if (isMovingQuickly)
        EntityAnimation.interval = 10;
    else
        EntityAnimation.interval = 65;

    return isMovingQuickly;
}

Most importantly, though, you should rename the method to more accurately describe what it's doing:

public bool CheckIfRunningAndSetAnimationInterval()

Upvotes: 1

Zein Makki
Zein Makki

Reputation: 30022

It is a good practice to have one return statement inside a method. Some argue about this, but it is an opinion.

it is also a good practice to make the if statement clear by removing unnecessary code:

public bool isRunning()
{
    bool result = false;
    if (move != Moving.None && staminaRegan)
    {
        if (keyState.IsKeyDown(Keys.Space))
        {
            EntityAnimation.interval = 10;
            result = true;
        }
        else
        {
            EntityAnimation.interval = 65;
        }
    }
    else
    {
        EntityAnimation.interval = 65;
    }

    return result;
}

Upvotes: 1

ViRuSTriNiTy
ViRuSTriNiTy

Reputation: 5155

I like this style and i use it too. First, you can read the code more easily and second it has a debugging advantage as you can set breakpoints for the individual else cases. Otherwise you would need to use breakpoint conditions.

Upvotes: 0

Related Questions