Server Background Task - CheckOnRoomItemsTask

Question

I have a game server than emulates a multi player isometric game. The server has a background task (CheckOnRoomItemsTask), which loops through active rooms and checks on the furniture items, processing and broadcasting (network packet) any updates.

I currently have a very large method (GetRollerUpdates) and was wondering how I could separate this down as I currently think its too large.

This code works, but I am seeking code quality improvements.

Code:

public class CheckOnRoomItemsTask(RoomRepository roomRepository) : IServerTask
{
 public TimeSpan PeriodicInterval => TimeSpan.FromMilliseconds(1000);
 public DateTime LastExecuted { get; set; }
 
 public Task ExecuteAsync()
 {
 Parallel.ForEachAsync(roomRepository.GetAllRooms(), BroadcastItemUpdates);
 return Task.CompletedTask;
 }
 private static async ValueTask BroadcastItemUpdates(RoomLogic room, CancellationToken ctx)
 {
 var writersToBroadcast = GetItemUpdates(room);
 
 foreach (var writer in writersToBroadcast)
 {
 await room.UserRepository.BroadcastDataAsync(writer);
 }
 }
 private static IEnumerable<AbstractPacketWriter> GetItemUpdates(IRoomLogic room)
 {
 var writers = new List<AbstractPacketWriter>();
 
 var roomRollers = room
 .FurnitureItems
 .Where(x => x.FurnitureItem!.InteractionType == FurnitureItemInteractionType.Roller);
 writers.AddRange(GetRollerUpdates(room, roomRollers));
 return writers;
 }
 private static IEnumerable<RoomObjectsRollingWriter> GetRollerUpdates(
 IRoomLogic room, 
 IEnumerable<PlayerFurnitureItemPlacementData> rollers)
 {
 var writers = new List<RoomObjectsRollingWriter>();
 var userIdsProcessed = new List<int>();
 var itemIdsProcessed = new List<int>();
 foreach (var roller in rollers)
 {
 var x = roller.PositionX;
 var y = roller.PositionY;
 
 var nextStep = RoomTileMapHelpers.GetPointInFront(x, y, roller.Direction);
 
 if (!room.TileMap.TileExists(nextStep))
 {
 continue;
 }
 
 var rollerPosition = new Point(x, y);
 
 var nextRoller = RoomTileMapHelpers
 .GetItemsForPosition(nextStep.X, nextStep.Y, room.FurnitureItems)
 .FirstOrDefault(fi => fi.FurnitureItem!.InteractionType == FurnitureItemInteractionType.Roller);
 
 var nextHeight = nextRoller?.FurnitureItem?.StackHeight ?? 0;
 
 var users = room.UserRepository
 .GetAll()
 .Where(u => !userIdsProcessed.Contains(u.Id));
 
 var rollingUsers = RoomTileMapHelpers.GetUsersForPoints([rollerPosition], users);
 
 foreach (var rollingUser in rollingUsers)
 {
 userIdsProcessed.Add(rollingUser.Id);
 if (rollingUser.StatusMap.ContainsKey(RoomUserStatus.Move))
 {
 continue;
 }
 
 writers.Add(new RoomObjectsRollingWriter
 {
 X = x,
 Y = y,
 NextX = nextStep.X,
 NextY = nextStep.Y,
 Objects = [],
 RollerId = roller.PlayerFurnitureItem.Id,
 MovementType = 2,
 RoomUserId = rollingUser.Id,
 Height = rollingUser.PointZ.ToString(),
 NextHeight = nextHeight.ToString()
 });
 
 room.TileMap.UserMap[rollingUser.Point].Remove(rollingUser);
 room.TileMap.AddUserToMap(nextStep, rollingUser);
 
 rollingUser.Point = nextStep;
 rollingUser.PointZ = nextRoller?.FurnitureItem?.StackHeight ?? 0;
 }
 var unprocessedNonRollers = room.FurnitureItems.Where(i =>
 !itemIdsProcessed.Contains(i.Id) && i.FurnitureItem!.InteractionType !=
 FurnitureItemInteractionType.Roller);
 var nonRollerItemsOnRoller = RoomTileMapHelpers.GetItemsForPosition(
 roller.PositionX, 
 roller.PositionY,
 unprocessedNonRollers);
 
 if (nonRollerItemsOnRoller.Count == 0)
 {
 continue;
 }
 nonRollerItemsOnRoller.ForEach(MoveItemOnRollerAsync);
 
 continue;
 async void MoveItemOnRollerAsync(PlayerFurnitureItemPlacementData item)
 {
 writers.Add(new RoomObjectsRollingWriter
 {
 X = item.PositionX,
 Y = item.PositionY,
 NextX = nextStep.X,
 NextY = nextStep.Y,
 Objects = [new RoomRollingObjectData
 {
 Id = item.PlayerFurnitureItem.Id,
 Height = item.PositionZ.ToString(), 
 NextHeight = nextHeight.ToString()
 }
 ],
 RollerId = roller.PlayerFurnitureItem.Id,
 MovementType = 2,
 RoomUserId = 0,
 Height = roller.PositionZ.ToString(),
 NextHeight = nextHeight.ToString()
 });
 
 item.PositionX = nextStep.X;
 item.PositionY = nextStep.Y;
 item.PositionZ = nextHeight;
 
 itemIdsProcessed.Add(item.Id);
 
 await RoomFurnitureItemHelpers.BroadcastItemUpdateToRoomAsync(room, item);
 }
 }
 return writers;
 }
}

Explaining some concepts

1. RoomLogic - A room, with users and furniture in
2. RoomObjectsRollingWriter - Network packet that tells client a roller is rolling
3. User - A player in the room, room user

Answer 1

private setter

 public DateTime LastExecuted { get; set; }

In CheckOnRoomItemsTask I understand the public getter. But surely it only makes sense for LastExecuted to change during actual execution, right?

repeated filtering

In GetItemUpdates we interrogate all furniture items in a room, looking for rollers. I don't know how crowded each room gets, nor whether rollers represent a large fraction of the population. But if you have many items yet just a few rollers, consider caching a filtered list of them. Do a cache invalidate on the rare occasions that furniture leaves or enters a room. Timestamps or serial numbers can help with that.

appropriate datastructure

 var userIdsProcessed = new List<int>();
 ...
 .Where(u => !userIdsProcessed.Contains(u.Id));

The cost of a .Contains() check is linear with number of users in a room. Prefer to store processed IDs in a Set, for \$O(1)\$ constant time lookups.

Maybe IDs are inconvenient here, and we'd rather store processed User objects instead?

The !itemIdsProcessed.Contains(i.Id) test similarly motivates using a Set for items.

magic number

 MovementType = 2,

Minimally, please offer us a comment here. Better would be to find the corresponding symbolic name.

BTW, using string type for Height is a little surprising, but I'm sure there's some reason for it. It appears Z values are sometimes string and sometimes the integer 0.

extract helper

currently have a very large method

 foreach (var rollingUser in rollingUsers)

The body of this loop looks like a good candidate for extracting into a separately testable helper function. Yes, there will be a bunch of parameters, that's life.

I encourage you to write proper documentation comments for at least GetRollerUpdates() and the new helper. The exercise of writing out those English sentences will help you to better see if each function has a single responsibility. Helpers are good at dealing with nitty gritty details, allowing the calling code to operate at a higher level of abstraction. That lets us write an English sentence which is closer to the Business Domain, more likely to fit into a Requirements document.

• Josh Hallow: Thanks! Just to confirm, are you advising using HashSet here for ids and still using LINQ Contains?
• J_H: That is correct.