Processing game user movement

Question

This code is a cycle method. It gets ran every 500 milliseconds to process the movement for users in the game. I've been told that it's been poorly coded and could use some optimizations but they didn't really go into detail. I'm asking if anyone could leave any suggestions on how I can get this method to run more efficiently.

It confuses me a little as I can't see any way to improve it, maybe that's just me though.

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, 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.IsBot)
                    {
                        User.X = User.SetX;
                        User.Y = User.SetY;
                        User.Z = User.SetZ;
                    }
                    else if (User.IsBot && !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)
            {
                if (User.Path.Count > 1)
                    User.Path.Clear();

                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;
                }
                else
                {
                    User.PathRecalcNeeded = false;
                    if (User.Path.Count > 1)
                        User.Path.Clear();
                }
            }

            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");

                    if (User.Statusses.ContainsKey("sign"))
                        User.RemoveStatus("sign");

                    if (User.IsBot && User.BotData.TargetUser > 0)
                    {
                        if (User.CarryItemId > 0)
                        {
                            RoomUser 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;
                        }
                        else
                        {
                            User.PathRecalcNeeded = false;
                            if (User.Path.Count > 1)
                                User.Path.Clear();
                        }

                        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.Statusses.Remove("sit");
                                User.Z += 0.35;
                                User.isSitting = false;
                                User.UpdateNeeded = true;
                            }
                            else if (User.isLying)
                            {
                                User.Statusses.Remove("sit");
                                User.Z += 0.35;
                                User.isLying = false;
                                User.UpdateNeeded = true;
                            }
                        }
                        if (!User.IsBot)
                        {
                            User.Statusses.Remove("lay");
                            User.Statusses.Remove("sit");
                        }

                        if (!User.IsBot && !User.IsPet && User.GetClient() != null)
                        {
                            if (User.GetClient().GetHabbo().IsTeleporting)
                            {
                                User.GetClient().GetHabbo().IsTeleporting = false;
                                User.GetClient().GetHabbo().TeleporterId = 0;
                            }
                            else if (User.GetClient().GetHabbo().IsHopping)
                            {
                                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, false);
            }
            else
                RemoveRoomUser(toRemove);
        }

        if (userCount != userCounter)
            UpdateUserCount(userCounter);
    }
    catch (Exception e)
    {
        Log.Error(e);
    }
}

Answer 1

One of the most important things to do while programming is beeing consistent in the coding style.

Using always the same style is important because if you encounter some different style you will pause and wonder what this is about. Having different style makes it hard to grasp as first glance what the code is about.

• Different casing styles for variables names. Sometimes you use camelCase and sometimes you use PascalCase casing for naming your variables.
• Usage of braces {}. Sometimes you use braces for single line if statements and sometimes you don't. I would like to encourage you to always use them. Omitting braces may lead to hidden and therfor hard to find bugs.

For naming issues you should take a look at the .NET Naming Guideline.

---

Regarding that god-method, you really should break it into smaller methods. Smaller methods are easier to read and understand. Maintaining smaller methods or finding a bug in them is easier as well.

---

You have a lot of duplicated code so I will give you some examples to remove the duplication

if (User.PathRecalcNeeded)
{
    if (User.Path.Count > 1)
        User.Path.Clear();

    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;
    }
    else
    {
        User.PathRecalcNeeded = false;
        if (User.Path.Count > 1)
            User.Path.Clear();
    }
}

• You don't need to check wether User.Path.Count > 1 just Clear() it. But at taking a closer look at it you can just remove it at all, because you set that property some lines below.
• PathFinder.FindPath() seems to be possible to find more than one path, hence you should rename it to FindPaths().
• How should it be possible in the else part that User.Path.Count > 1 ever will be true?

This can be refactored like so (I left FindPath so you may copy&paste)

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;
}

---

Some of the above applies here as well

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;
    }
    else
    {
        User.PathRecalcNeeded = false;
        if (User.Path.Count > 1)
            User.Path.Clear();
    }

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

Refactored it will look like so

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;
    NextStep = User.Path[(User.Path.Count - User.PathStep) - 1];
}

---

Only a portion of the really big if branch

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.Statusses.Remove("sit");
            User.Z += 0.35;
            User.isSitting = false;
            User.UpdateNeeded = true;
        }
        else if (User.isLying)
        {
            User.Statusses.Remove("sit");
            User.Z += 0.35;
            User.isLying = false;
            User.UpdateNeeded = true;
        }
    }
    if (!User.IsBot)
    {
        User.Statusses.Remove("lay");
        User.Statusses.Remove("sit");
    }

Why would you remove "sit" from the Statusses if the user isLying ? As you can see there is a lot of duplicated code. The difference between the innner if..else is the removing of the status and the setting of ``isSittingvsisLying, where the removing of thestatus` could be removed because you later remove it anyway.

Refoctored it would look like

 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.isSitting = false;
        }
        else if (User.isLying)
        {
            User.isLying = false;
        }
        if (User.isSitting || User.IsLying)
        {
            User.Z += 0.35;
            User.UpdateNeeded = true;
        }
        User.Statusses.Remove("lay");
        User.Statusses.Remove("sit");
    }

---

This line makes me wonder as well

if (!User.IsBot && User.RidingHorse && User.IsPet == false)

You should either use the negotiation operator ! or compare the boolean to false. Most say you should use the ! because it is the idiomatic way. Some say using == false is easier to see with bad eyes.

Whatever you use is up to you but as said before, stick to one style.

---

Some more wondering is about the overall User. In your code a user may be

• a Bot
• a Pet

• a normal user ?

• maybe you should use a base or abstract class user which is inherited by a Pet and a Bot class ?

• maybe you should break the loop into several methods and process pets, bots and user separately ?

---

Whats the difference between User.RemoveStatus("mv"); and User.Statusses.Remove("lay"); yes, at least "mv" vs "lay" but that is not the point. Again the point is beeing consistent. Either use the method RemoveStatus() of the user or use the Remove() method of the Statusses property.

But while we are at User.Statusses.Remove("lay");, what is the user doing ? His the user lying or laying ? Check its property.

---

There is much more to say but I guess I gave you a start here so you may know what to do yourself.