Adding chat to an MVC database

Question

I have these relevant tables in an MVC website:

• Server
• ServerPlayer
• ServerPlayerChat

Servers have serverPlayers and ServerPlayers have ServerPlayerChats. Simple enough and expectable.

I have a feeling my code could be drastically condensed (particularly the GetServerPlayer method) and that the GetServerPlayer method is named wrong, although I couldn't think of a better name other than GetOrCreateServerPlayer, which is really long-winded. Also, this is in the Server model. I think I could move the add chat to the server player model, but my concern is that this might be more appropriate in a controller.

Is it good practice to do what I'm doing where I'm not only adding the new chat to the ServerUserChats collection, but also setting ServerPlayer = player inside the new chat? I'm aware that LINQ/EF/... is capable of figuring out relations like this from just one variable but it felt right to do this, so feedback on that is accepted too.

public void AddChat(string playername, string chat, int time)
{
    LastChat = time;
    ServerPlayer player = GetServerPlayer(playername);
    player.ServerUserChats.Add(new ServerPlayerChat() {
        Content = chat,
        Time = time,
        ServerPlayer = player
    });
}

public ServerPlayer GetServerPlayer(string playername)
{
    ServerPlayer player = GetPlayer(playername);
    if (player == null)
    {
        player = new ServerPlayer() {
            Server = this,
            UserName = playername,
        };

        Players.Add(player);
    }

    return player;
}

public ServerPlayer GetPlayer(string playername)
{
    return Players.Where(x => x.UserName == playername).FirstOrDefault();
}

Answer 1

GetOrCreate methods are inherently dual-purpose which violates the Single Responsibility Principle. The further GetOrCreate gets buried in your code, the harder it will be to strip that behavior out when you don't want it. Think of it this way: what objects/entities are actually able to create ServerChats? The answer isn't playernames, it's ServerPlayers. I would refactor like so:

public void AddChat(ServerPlayer player, string chat, int time)
{
    if(player == null)
        throw new ArgumentException("player";
    ...
}

public ServerPlayer GetServerPlayer(string playerName)
{
    return Players.SingleOrDefault(x => x.UserName == playername);
}

Now you have to consider when a ServerPlayer should be created, but at least it isn't "magic" and can be done explicitly elsewhere. Maybe you some sort of session management code that ensures someone on the server always has a ServerPlayer in place, etc. The point is that this is now possible:

ServerPlayer currentPlayer = GetServerPlayer(this.Request.PlayerName);

if(currentPlayer == null)
{
    RedirectToLogin();
}
else
{
    AddChat(currentPlayer, chat, time);
}

You may not have a requirement for RedirectToLogin now, but the "create by default" code isn't trapped deep in your system so it allows for this type of code.

Answer 2

Use Players.SingleOrDefault() instead of Where and FirstOrDefault. If you use the function GetPlayer just one time, remove it.

public ServerPlayer GetServerPlayer(string playername)
{
    ServerPlayer player =  Players.SingleOrDefault(x => x.UserName == player name);
    if (player == null)
    {
        player = new ServerPlayer() {
            Server = this,
            UserName = playername,
        };

        Players.Add(player);
    }

    return player;
}

Personally, I think you should change GetServerPlayer to GetOrCreateServerPlayer especially if you are a team. We don't have to know or look your function to understand its purpose. Or keep the name and add the appropriates comments (summary, return, etc).

In the case of ASP.NET which follow MVC pattern, I think you must separate the logic and the model. For my part I create Manager for logic and my controller just serve to call the appropriates methods of my manager.