Skip to main content
added 12 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238

yourYour GetNextFrame shouldshould have less iffewer if statements, in. In other words, you should merge some of them together, the. The ones that return early especially, those should especially be in an if ..then .. else if.. then structure, like this. it It works either way, but this looks cleaner, and is more to the point about what the code does.

public Rectangle GetNextFrame(GameActions gameAction)
{
    var sequence = GetSequence(gameAction);
    if (gameAction == GameActions.Idle) {
        frameTimer = 0;

        prevFrame.X = 0;
        prevGameAction = gameAction;

        return prevFrame;
    } else if(frameTimer != 0) {
        UpdateTimer();

        return prevFrame;
    } else if (sequence == null) {
        return prevFrame;
    }

    UpdateTimer();

    Rectangle nextFrame;

    if (gameAction != prevGameAction)
    {
        nextFrame = sequence.First();
    }
    else
    {
        nextFrame = sequence.Next(prevFrame);
    }

    prevFrame = nextFrame;
    prevGameAction = gameAction;

    return nextFrame;
}

yourYour Animate Methodmethod has some extra lines in it as well where the ifif statements can be merged together.

public void Animate(Actions action)
{
    Animation animation;

    if (!animations.TryGetValue(action, out animation) || !animation.IsReady())
    {
        return;
    }

    frame = animation.GetNextFrame();

    if (animation == prevAnimation)
    {
        return;
    }

    if (prevAnimation != null)
    {
        prevAnimation.Reset();
    }

    prevAnimation = animation;
}

thatThat one ifif statement with the orOR clause || does the exact same thing that your code did, but with lessfewer lines and in a cleaner manner.

your GetNextFrame should have less if statements, in other words you should merge some of them together, the ones that return early especially, those should be in an if ..then .. else if.. then structure, like this. it works either way but this looks cleaner, and is more to the point about what the code does.

public Rectangle GetNextFrame(GameActions gameAction)
{
    var sequence = GetSequence(gameAction);
    if (gameAction == GameActions.Idle) {
        frameTimer = 0;

        prevFrame.X = 0;
        prevGameAction = gameAction;

        return prevFrame;
    } else if(frameTimer != 0) {
        UpdateTimer();

        return prevFrame;
    } else if (sequence == null) {
        return prevFrame;
    }

    UpdateTimer();

    Rectangle nextFrame;

    if (gameAction != prevGameAction)
    {
        nextFrame = sequence.First();
    }
    else
    {
        nextFrame = sequence.Next(prevFrame);
    }

    prevFrame = nextFrame;
    prevGameAction = gameAction;

    return nextFrame;
}

your Animate Method has some extra lines in it as well where the if statements can be merged together.

public void Animate(Actions action)
{
    Animation animation;

    if (!animations.TryGetValue(action, out animation) || !animation.IsReady())
    {
        return;
    }

    frame = animation.GetNextFrame();

    if (animation == prevAnimation)
    {
        return;
    }

    if (prevAnimation != null)
    {
        prevAnimation.Reset();
    }

    prevAnimation = animation;
}

that one if statement with the or clause || does the exact same thing that your code did, but with less lines and cleaner.

Your GetNextFrame should have fewer if statements. In other words, you should merge some of them together. The ones that return early should especially be in an if ..then .. else if.. then structure, like this. It works either way, but this looks cleaner and is more to the point about what the code does.

public Rectangle GetNextFrame(GameActions gameAction)
{
    var sequence = GetSequence(gameAction);
    if (gameAction == GameActions.Idle) {
        frameTimer = 0;

        prevFrame.X = 0;
        prevGameAction = gameAction;

        return prevFrame;
    } else if(frameTimer != 0) {
        UpdateTimer();

        return prevFrame;
    } else if (sequence == null) {
        return prevFrame;
    }

    UpdateTimer();

    Rectangle nextFrame;

    if (gameAction != prevGameAction)
    {
        nextFrame = sequence.First();
    }
    else
    {
        nextFrame = sequence.Next(prevFrame);
    }

    prevFrame = nextFrame;
    prevGameAction = gameAction;

    return nextFrame;
}

Your Animate method has some extra lines in it as well where the if statements can be merged together.

public void Animate(Actions action)
{
    Animation animation;

    if (!animations.TryGetValue(action, out animation) || !animation.IsReady())
    {
        return;
    }

    frame = animation.GetNextFrame();

    if (animation == prevAnimation)
    {
        return;
    }

    if (prevAnimation != null)
    {
        prevAnimation.Reset();
    }

    prevAnimation = animation;
}

That one if statement with the OR clause || does the exact same thing that your code did, but with fewer lines and in a cleaner manner.

added 237 characters in body
Source Link
Malachi
  • 29.1k
  • 11
  • 87
  • 188

your GetNextFrame should really have oneless if statementstatements, in other words you should merge some of them together, the ones that return early especially, those should be in an (with multiple else if's)if ..then .. else if.. then structure, like this. it works either way but this looks cleaner, and is more to the point about what the code does.

public Rectangle GetNextFrame(GameActions gameAction)
{
    var sequence = GetSequence(gameAction);
    if (gameAction == GameActions.Idle) {
        frameTimer = 0;

        prevFrame.X = 0;
        prevGameAction = gameAction;

        return prevFrame;
    } else if(frameTimer != 0) {
        UpdateTimer();

        return prevFrame;
    } else if (sequence == null) {
        return prevFrame;
    }

    UpdateTimer();

    Rectangle nextFrame;

    if (gameAction != prevGameAction)
    {
        nextFrame = sequence.First();
    }
    else
    {
        nextFrame = sequence.Next(prevFrame);
    }

    prevFrame = nextFrame;
    prevGameAction = gameAction;

    return nextFrame;
}

your Animate Method has some extra lines in it as well where the if statements can be merged together.

public void Animate(Actions action)
{
    Animation animation;

    if (!animations.TryGetValue(action, out animation) || !animation.IsReady())
    {
        return;
    }

    frame = animation.GetNextFrame();

    if (animation == prevAnimation)
    {
        return;
    }

    if (prevAnimation != null)
    {
        prevAnimation.Reset();
    }

    prevAnimation = animation;
}

that one if statement with the or clause || does the exact same thing that your code did, but with less lines and cleaner.

your GetNextFrame should really have one if statement (with multiple else if's)

public Rectangle GetNextFrame(GameActions gameAction)
{
    var sequence = GetSequence(gameAction);
    if (gameAction == GameActions.Idle) {
        frameTimer = 0;

        prevFrame.X = 0;
        prevGameAction = gameAction;

        return prevFrame;
    } else if(frameTimer != 0) {
        UpdateTimer();

        return prevFrame;
    } else if (sequence == null) {
        return prevFrame;
    }

    UpdateTimer();

    Rectangle nextFrame;

    if (gameAction != prevGameAction)
    {
        nextFrame = sequence.First();
    }
    else
    {
        nextFrame = sequence.Next(prevFrame);
    }

    prevFrame = nextFrame;
    prevGameAction = gameAction;

    return nextFrame;
}

your Animate Method has some extra lines in it as well where the if statements can be merged together.

public void Animate(Actions action)
{
    Animation animation;

    if (!animations.TryGetValue(action, out animation) || !animation.IsReady())
    {
        return;
    }

    frame = animation.GetNextFrame();

    if (animation == prevAnimation)
    {
        return;
    }

    if (prevAnimation != null)
    {
        prevAnimation.Reset();
    }

    prevAnimation = animation;
}

that one if statement with the or clause || does the exact same thing that your code did, but with less lines and cleaner.

your GetNextFrame should have less if statements, in other words you should merge some of them together, the ones that return early especially, those should be in an if ..then .. else if.. then structure, like this. it works either way but this looks cleaner, and is more to the point about what the code does.

public Rectangle GetNextFrame(GameActions gameAction)
{
    var sequence = GetSequence(gameAction);
    if (gameAction == GameActions.Idle) {
        frameTimer = 0;

        prevFrame.X = 0;
        prevGameAction = gameAction;

        return prevFrame;
    } else if(frameTimer != 0) {
        UpdateTimer();

        return prevFrame;
    } else if (sequence == null) {
        return prevFrame;
    }

    UpdateTimer();

    Rectangle nextFrame;

    if (gameAction != prevGameAction)
    {
        nextFrame = sequence.First();
    }
    else
    {
        nextFrame = sequence.Next(prevFrame);
    }

    prevFrame = nextFrame;
    prevGameAction = gameAction;

    return nextFrame;
}

your Animate Method has some extra lines in it as well where the if statements can be merged together.

public void Animate(Actions action)
{
    Animation animation;

    if (!animations.TryGetValue(action, out animation) || !animation.IsReady())
    {
        return;
    }

    frame = animation.GetNextFrame();

    if (animation == prevAnimation)
    {
        return;
    }

    if (prevAnimation != null)
    {
        prevAnimation.Reset();
    }

    prevAnimation = animation;
}

that one if statement with the or clause || does the exact same thing that your code did, but with less lines and cleaner.

Source Link
Malachi
  • 29.1k
  • 11
  • 87
  • 188

your GetNextFrame should really have one if statement (with multiple else if's)

public Rectangle GetNextFrame(GameActions gameAction)
{
    var sequence = GetSequence(gameAction);
    if (gameAction == GameActions.Idle) {
        frameTimer = 0;

        prevFrame.X = 0;
        prevGameAction = gameAction;

        return prevFrame;
    } else if(frameTimer != 0) {
        UpdateTimer();

        return prevFrame;
    } else if (sequence == null) {
        return prevFrame;
    }

    UpdateTimer();

    Rectangle nextFrame;

    if (gameAction != prevGameAction)
    {
        nextFrame = sequence.First();
    }
    else
    {
        nextFrame = sequence.Next(prevFrame);
    }

    prevFrame = nextFrame;
    prevGameAction = gameAction;

    return nextFrame;
}

your Animate Method has some extra lines in it as well where the if statements can be merged together.

public void Animate(Actions action)
{
    Animation animation;

    if (!animations.TryGetValue(action, out animation) || !animation.IsReady())
    {
        return;
    }

    frame = animation.GetNextFrame();

    if (animation == prevAnimation)
    {
        return;
    }

    if (prevAnimation != null)
    {
        prevAnimation.Reset();
    }

    prevAnimation = animation;
}

that one if statement with the or clause || does the exact same thing that your code did, but with less lines and cleaner.