10
\$\begingroup\$

I use this method to cycle the movement for players in my game. I've been told this method is too large and needs to be cleaned up into smaller pieces, I really can't see much that shouts out "put me in my own method", it all seems code that doesn't relate to anything, generic I suppose. It seems like each line does something totally different.

I'm pretty new to C# and would like some help in taking the necessary steps to cleaning this up. I've cleaned it up a little before posting but it still seems large, it was 400+.

public void OnCycle()
{
    int userCounter = 0;

    try
    {

        List<RoomUser> ToRemove = new List<RoomUser>();

        foreach (RoomUser User in GetUserList().ToList())
        {
            if (User == null)
            {
                continue;
            }

            if (!IsValid(User))
            {
                if (User.GetClient() != null)
                {
                    RemoveUserFromRoom(User.GetClient(), false);
                }
                else
                {
                    RemoveRoomUser(User);
                }
            }

            if (User.NeedsAutokick && !ToRemove.Contains(User))
            {
                ToRemove.Add(User);
                continue;
            }

            bool updated = false;
            User.IdleTime++;
            User.HandleSpamTicks();

            if (!User.IsBot && !User.IsAsleep && User.IdleTime >= 600)
            {
                User.IsAsleep = true;
                _room.SendPacket(new SleepComposer(User, true));
            }

            if (User.CarryItemId > 0)
            {
                User.CarryTimer--;
                if (User.CarryTimer <= 0)
                {
                    User.CarryItem(0);
                }
            }

            if (_room.GotFreeze())
            {
                _room.GetFreeze().CycleUser(User);
            }

            bool InvalidStep = false;

            if (User.isRolling)
            {
                if (User.rollerDelay <= 0)
                {
                    UpdateUserStatus(User, false);
                    User.isRolling = false;
                }
                else
                {
                    User.rollerDelay--;
                }
            }

            if (User.SetStep)
            {
                if (_room.GetGameMap().IsValidStep2(User, new Vector2D(User.X, User.Y), new Vector2D(User.SetX, User.SetY), (User.GoalX == User.SetX && User.GoalY == User.SetY), User.AllowOverride))
                {
                    if (!User.RidingHorse)
                    {
                        _room.GetGameMap().UpdateUserMovement(new Point(User.Coordinate.X, User.Coordinate.Y), new Point(User.SetX, User.SetY), User);
                    }

                    List<Item> items = _room.GetGameMap().GetCoordinatedItems(new Point(User.X, User.Y));
                    foreach (Item Item in items.ToList())
                    {
                        Item.UserWalksOffFurni(User);
                    }

                    if (!User.RidingHorse)
                    {
                        User.X = User.SetX;
                        User.Y = User.SetY;
                        User.Z = User.SetZ;
                    }

                    if (!User.IsBot && User.RidingHorse)
                    {
                        RoomUser Horse = GetRoomUserByVirtualId(User.HorseID);
                        if (Horse != null)
                        {
                            Horse.X = User.SetX;
                            Horse.Y = User.SetY;
                        }
                    }

                    if (User.X == _room.GetGameMap().Model.DoorX && User.Y == _room.GetGameMap().Model.DoorY && !ToRemove.Contains(User) && !User.IsBot)
                    {
                        ToRemove.Add(User);
                        continue;
                    }

                    List<Item> Items = _room.GetGameMap().GetCoordinatedItems(new Point(User.X, User.Y));
                    foreach (Item Item in Items.ToList())
                    {
                        Item.UserWalksOnFurni(User);
                    }

                    UpdateUserStatus(User, true);
                }
                else
                {
                    InvalidStep = true;
                }

                User.SetStep = false;
            }

            if (User.PathRecalcNeeded)
            {
                User.Path = PathFinder.FindPath(User, _room.GetGameMap().DiagonalEnabled, _room.GetGameMap(), new Vector2D(User.X, User.Y), new Vector2D(User.GoalX, User.GoalY));

                if (User.Path.Count > 1)
                {
                    User.PathStep = 1;
                    User.IsWalking = true;
                }

                User.PathRecalcNeeded = false;
            }

            if (User.IsWalking && !User.Freezed)
            {
                if (InvalidStep || User.PathStep >= User.Path.Count || (User.GoalX == User.X && User.GoalY == User.Y)) //No path found, or reached goal (:
                {
                    User.IsWalking = false;
                    User.RemoveStatus("mv");
                    User.RemoveStatus("sign");

                    if (User.IsBot && User.BotData.TargetUser > 0)
                    {
                        if (User.CarryItemId > 0)
                        {
                            var Target = _room.GetRoomUserManager().GetRoomUserByHabbo(User.BotData.TargetUser);

                            if (Target != null && Gamemap.TilesTouching(User.X, User.Y, Target.X, Target.Y))
                            {
                                User.SetRot(Rotation.Calculate(User.X, User.Y, Target.X, Target.Y), false);
                                Target.SetRot(Rotation.Calculate(Target.X, Target.Y, User.X, User.Y), false);
                                Target.CarryItem(User.CarryItemId);
                            }
                        }

                        User.CarryItem(0);
                        User.BotData.TargetUser = 0;
                    }

                    if (User.RidingHorse && User.IsPet == false && !User.IsBot)
                    {
                        RoomUser mascotaVinculada = GetRoomUserByVirtualId(User.HorseID);
                        if (mascotaVinculada != null)
                        {
                            mascotaVinculada.IsWalking = false;
                            mascotaVinculada.RemoveStatus("mv");
                            mascotaVinculada.UpdateNeeded = true;
                        }
                    }
                }
                else
                {
                    Vector2D NextStep = User.Path[(User.Path.Count - User.PathStep) - 1];
                    User.PathStep++;

                    if (!_room.GetGameMap().IsValidStep(new Vector2D(User.X, User.Y), new Vector2D(NextStep.X, NextStep.Y), (User.GoalX == User.SetX && User.GoalY == User.SetY), User.AllowOverride))
                    {
                        User.Path = PathFinder.FindPath(User, _room.GetGameMap().DiagonalEnabled, _room.GetGameMap(), new Vector2D(User.X, User.Y), new Vector2D(User.GoalX, User.GoalY));

                        if (User.Path.Count > 1)
                        {
                            User.PathStep = 1;
                            User.IsWalking = true;
                            User.PathRecalcNeeded = false;
                        }

                        User.PathRecalcNeeded = false;
                        NextStep = User.Path[(User.Path.Count - User.PathStep) - 1];
                    }

                    if (User.FastWalking && User.PathStep < User.Path.Count)
                    {
                        int s2 = (User.Path.Count - User.PathStep) - 1;
                        NextStep = User.Path[s2];
                        User.PathStep++;
                    }

                    if (User.SuperFastWalking && User.PathStep < User.Path.Count)
                    {
                        int s2 = (User.Path.Count - User.PathStep) - 1;
                        NextStep = User.Path[s2];
                        User.PathStep++;
                        User.PathStep++;
                    }

                    int nextX = NextStep.X;
                    int nextY = NextStep.Y;

                    User.RemoveStatus("mv");

                    if (_room.GetGameMap().IsValidStep2(User, new Vector2D(User.X, User.Y), new Vector2D(nextX, nextY), (User.GoalX == nextX && User.GoalY == nextY), User.AllowOverride))
                    {
                        double nextZ = _room.GetGameMap().SqAbsoluteHeight(nextX, nextY);

                        if (!User.IsBot)
                        {
                            if (User.isSitting || User.isLying)
                            {
                                User.Z += 0.35;
                                User.UpdateNeeded = true;
                            }

                            User.isLying = false;
                            User.Statusses.Remove("lay");

                            User.isSitting = false;
                            User.Statusses.Remove("sit");
                        }

                        if (!User.IsBot && !User.IsPet && User.GetClient() != null)
                        {
                            User.GetClient().GetHabbo().IsTeleporting = false;
                            User.GetClient().GetHabbo().TeleporterId = 0;

                            User.GetClient().GetHabbo().IsHopping = false;
                            User.GetClient().GetHabbo().HopperId = 0;
                        }

                        if (!User.IsBot && User.RidingHorse && User.IsPet == false)
                        {
                            RoomUser Horse = GetRoomUserByVirtualId(User.HorseID);
                            if (Horse != null)
                            {
                                Horse.SetStatus("mv", nextX + "," + nextY + "," + TextHandling.GetString(nextZ));
                            }

                            User.SetStatus("mv", +nextX + "," + nextY + "," + TextHandling.GetString(nextZ + 1));

                            User.UpdateNeeded = true;
                            Horse.UpdateNeeded = true;
                        }
                        else
                        {
                            User.SetStatus("mv", nextX + "," + nextY + "," + TextHandling.GetString(nextZ));
                        }


                        int newRot = Rotation.Calculate(User.X, User.Y, nextX, nextY, User.moonwalkEnabled);

                        User.RotBody = newRot;
                        User.RotHead = newRot;

                        User.SetStep = true;
                        User.SetX = nextX;
                        User.SetY = nextY;
                        User.SetZ = nextZ;
                        UpdateUserEffect(User, User.SetX, User.SetY);

                        updated = true;

                        if (User.RidingHorse && User.IsPet == false && !User.IsBot)
                        {
                            RoomUser Horse = GetRoomUserByVirtualId(User.HorseID);
                            if (Horse != null)
                            {
                                Horse.RotBody = newRot;
                                Horse.RotHead = newRot;

                                Horse.SetStep = true;
                                Horse.SetX = nextX;
                                Horse.SetY = nextY;
                                Horse.SetZ = nextZ;
                            }
                        }

                        _room.GetGameMap().GameMap[User.X, User.Y] = User.SqState; // REstore the old one
                        User.SqState = _room.GetGameMap().GameMap[User.SetX, User.SetY]; //Backup the new one

                        if (_room.RoomBlockingEnabled == 0)
                        {
                            RoomUser Users = _room.GetRoomUserManager().GetUserForSquare(nextX, nextY);
                            if (Users != null)
                            {
                                _room.GetGameMap().GameMap[nextX, nextY] = 0;
                            }
                        }
                        else
                        {
                            _room.GetGameMap().GameMap[nextX, nextY] = 1;
                        }
                    }
                }
                if (!User.RidingHorse)
                {
                    User.UpdateNeeded = true;
                }
            }
            else
            {
                if (User.Statusses.ContainsKey("mv"))
                {
                    User.RemoveStatus("mv");
                    User.UpdateNeeded = true;

                    if (User.RidingHorse)
                    {
                        RoomUser Horse = GetRoomUserByVirtualId(User.HorseID);
                        if (Horse != null)
                        {
                            Horse.RemoveStatus("mv");
                            Horse.UpdateNeeded = true;
                        }
                    }
                }
            }

            if (User.RidingHorse)
            {
                User.ApplyEffect(77);
            }

            if (User.IsBot && User.BotAI != null)
            {
                User.BotAI.OnTimerTick();
            }
            else
            {
                userCounter++;
            }

            if (!updated)
            {
                UpdateUserEffect(User, User.X, User.Y);
            }
        }

        foreach (RoomUser toRemove in ToRemove.ToList())
        {
            GameClient client = PlusEnvironment.GetGame().GetClientManager().GetClientByUserId(toRemove.HabboId);
            if (client != null)
            {
                RemoveUserFromRoom(client, true);
            }
            else
            {
                RemoveRoomUser(toRemove);
            }
        }

        if (userCount != userCounter)
        {
            UpdateUserCount(userCounter);
        }
    }
    catch (Exception e)
    {
        Logger.Error(e);
    }
}
\$\endgroup\$
  • \$\begingroup\$ I've cleaned it up a little before posting but this is still your real code, right? You did not remove or change anything important? I'd be great if you could write a little more detailed description about what's going on inside this method. \$\endgroup\$ – t3chb0t Mar 29 '18 at 5:22
  • 1
    \$\begingroup\$ You really should register yourself as a user here. Then you can get your accounts merged. \$\endgroup\$ – Heslacher Mar 29 '18 at 5:27
  • 1
    \$\begingroup\$ You should learn inheritance: instead of all these isa methods, it would be responsability of the specific OnCycle method to do what it is needed in each case. \$\endgroup\$ – N74 Mar 29 '18 at 6:40
  • \$\begingroup\$ RemoveUserFromRoom is not included. \$\endgroup\$ – paparazzo Mar 29 '18 at 15:01
  • \$\begingroup\$ Why all the .ToList()? Just enumerate. \$\endgroup\$ – paparazzo Mar 29 '18 at 15:24
11
\$\begingroup\$

Notice what you said:

I've been told this method is too large and needs to be cleaned up into smaller pieces, I really can't see much that shouts out "put me in my own method", it all seems code that doesn't relate to anything, generic I suppose. It seems like each line does something totally different.

This is a contradiction. If you think that this code belongs in a single method, that inherently means that the code is an indivisible chunk of logic.

But if you say that every line does something totally different, then you're essentially arguing that every line can be a method in and of itself.

It's one or the other.


The issue here, as I see it, is that you haven't clearly outlined your responsibility. Try explaining to me what this method's responsibility is, using only a single sentence (one main verb).

You actually already tried that:

I use this method to cycle the movement for players in my game.

Your description is not very clear. Based on this sentence alone, you wouldn't be able to give a developer enough information to recreate this method from scratch.

Here's my attempt at describing the responsibility of the method:

  • Handle the lobby registration process
  • Handles the kickbanning process
  • Performs player movement
  • Performs fast player movement
  • Performs super fast player movement
  • Performs player mounted movement
  • Validates intended movement
  • Tracks items carried by players
  • Performs player teleport
  • Performs player hopping movement
  • Recalculates player paths
  • Handles environmental movement (on/off furniture)
  • Applies effect 77
  • Handles bot behavior
  • ...

And that's where the problem lies. You haven't set any boundaries for your responsibilities, so you've never been able to conclusively exclude any logic from this method.

This method is the software equivalent of the attic (or garage) in most people's houses: it contains everything that didn't fit in any of the other rooms.


Instead of showing you the improved version of your code, I think it's more pertinent to address how you can optimize code when you don't see the wood for the trees. Most of these are a matter of applying common sense.

0 - General rule

If a certain chunk of logic can be separated, it should be separated (in theory).

But do it within reason. Splitting up a two-line method into two one-line methods is not as beneficial as splitting up a 400-line method into several chunks.

Mainly focus on methods that contain more lines than your screen can comfortably fit (this is of course just an estimate, we don't all have the same screen resolution/font size).

1 - Nesting

Nesting is a good first indication that you need to abstract the logic into separate methods. In most cases, the best place to break a single method into two is exactly on a code block.

Take this example:

public int CountAllLetters(List<string> words)
{
    int total = 0;

    foreach(var word in words)
    {
         if (word == null) 
             continue;

         total += word.Length;
    }

    return total;
 }

Note: This is an example. The method isn't long enough to warrant refactoring yet; but that's because I chose to keep the example simple for clarity's sake.

The body of the foreach is a prime location to split the logic off into a submethod:

public int CountAllLetters(List<string> words)
{
    int total = 0;

    foreach(var word in words)
    {
         total += CountAllLetters(word);
    }

    return total;
 }

public int CountAllLetters(string word)
{
    if (word == null) 
             return 0;

    return word.Length;
}

This applies to many nested structures:

  • When an if block's body handles more than setting a handful of variables, you can usually abstract that body into a separate method.
    • Your current code, if I counted correctly, goes as far as having five levels of nested if blocks. That is not a good sign. This should have been abstracted into several method layers.
  • A foreach quite often allows for easy abstraction into submethods.
    • Look at the above code example. Instead of having a single methods that takes a list, and processes every member of that list, we've created two separate methods: one to process the list, and one to process each member of the list.

2 - Think in logical groups.

Your current code does a lot of things. However, you can also easily see that some of them are grouped logically:

if (User.SetStep)
{    
     //Check if it's a valid step

     //Perform a move when not mounted

     //Perform "walk off" logic if user is on furniture

     //If user steps to a level exit, remove user from the level.
}

I skipped a few where I did not understand the purpose of the code.

Even if you think that all of the comments I listed belong to a single logical chunk; you should still see that these comments share a common trait; they are all executed only if SetStep == true.

We could call this chunk of code "step logic". The "step logic" group contains all code that only executes if SetStep == true.

This immediately opens the door to abstraction.

if (User.SetStep)
    ProcessStepLogic(User);

if (User.PathRecalcNeeded)
    RecalculatePath(User);

3 - Avoid handling every unique combination of values as a separate event.

Your code often falls into this trap. To give you a simpler example:

public void DoSomething(User user)
{
    if(user.IsWalking && user.IsTalking)
    {
        user.Walk();
        user.Talk();
    }
    else if (user.IsWalking)
    {
        user.Walk();
    }
    else if (user.IsTalking)
    {
        user.Talk();
    }
}

It's a silly example, but it gets the point across. You've made three cases out of two. What you instead should have done is:

public void DoSomething(User user)
{
    if (user.IsWalking)
    {
        user.Walk();
    }

    if (user.IsTalking)
    {
        user.Talk();
    }
}

To show you why the original code is such a bad idea; let's update our code. Instead of two states (walking/talking), let's look at happens when we add a third state (winking) to the mix:

public void DoSomething(User user)
{
    if(user.IsWalking && user.IsTalking && user.IsWinking)
    {
        user.Walk();
        user.Talk();
        user.Wink();
    }
    else if (user.IsWalking && user.IsWinking)
    {
        user.Walk();
        user.Wink();
    }
    else if (user.IsTalking && user.IsWinking)
    {
        user.Talk();
        user.Wink();
    }
    else if(user.IsWalking && user.IsTalking)
    {
        user.Walk();
        user.Talk();
    }
    else if (user.IsWalking)
    {
        user.Walk();
    }
    else if (user.IsTalking)
    {
        user.Talk();
    }
    else if (user.IsWinking)
    {
        user.Wink();
    }
}

Compare this to the much simpler:

public void DoSomething(User user)
{
    if (user.IsWalking)
    {
        user.Walk();
    }

    if (user.IsTalking)
    {
        user.Talk();
    }

    if (user.IsWinking)
    {
        user.Wink();
    }
}

The amount of events grows exponentially. A simple chart:

1 boolean = 2 possible outcomes (2^1)
2 booleans = 4 possible outcomes (2^2)
3 booleans = 8 possible outcomes (2^3)
4 booleans = 16 possible outcomes (2^4)
5 booleans = 32 possible outcomes (2^5)
6 booleans = 64 possible outcomes (2^6)

This 2^(amount of booleans) progression is the definition of exponential growth. And this is even assuming that you're only using binary choices, and not types that can have more than two distinct values!

How do you stop this growth?
Simply avoid handling combinations of variables unless you absolutely have to. Separate everything as much as you can, in order to keep yourself from repeating the same logic over and over.


This review is getting quite long. I think I'll leave it here, the biggest problems have been addressed. Some tips if you want to learn how to improve the quality of your code:

  • Generally speaking, it's a bad idea to add new chunks of logic to an existing method. When you first create a method, you are thinking of a specific purpose for that method. Stick to that initial purpose. If you need a second job to be handled, create a second method.
  • I get the feeling that the quality of this method degraded through scope creep. E.g. based on the code and my experience, I'm pretty sure that mounted movement was added at a later stage, because it seems to be a copy/paste job from the unmounted movement logic.
  • The code in your example wildly varies in what it tries to achieve. It handles player/lobby registration and moves the players on the map of a certain lobby. Those are two completely separate parts of the application, and I can see no reason why these two types of logic should be found in the same class (let alone the same method).
    • While a player may think that moving on the map or moving between maps is all part of the same movement logic; that is not the case if you look at the code. The developer shouldn't structure their code based on how the player thinks of the game. If the code handles lobby movement and map movement with different code, then these two things need to be separated. Regardless of how the player thinks of it.
  • A simple way to stress test the quality of your code is to introduce changes (make a backup first, of course! :)) If your code is well-written, then changing something about the logic should only require you to change one piece of the code. If you realize that a simple change would require you to scan through all of the code to find every occurence of this one feature, that proves that you didn't make that feature reusable (which you should). Some examples you could test:
    • Try to double the distance that players can travel with every step.
    • Try to make horses always move like a chess knight.
    • How would you handle a mount moving without a player riding them? (e.g. if a player dismounts, the horse still walks around).
    • ...
\$\endgroup\$
  • \$\begingroup\$ This is a well-articulated, well-organized answer with excellent explanations. \$\endgroup\$ – Rick Davin Mar 29 '18 at 12:30

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.