current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite

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();
}
share|improve this question
up vote 2 down vote accepted

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.

share|improve this answer
up vote 3 down vote

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.