2

I'm not absolutely sure if this is the right place for this question. But I guess StackOverflow won't be the right place.

Is it for any reason a bad way to pass an object to a method from the same class? For some reason I don't feel well with the first solution.

The method Start starts the game with the given player.

CurrentGame.Start(CurrentGame.CurrentPlayer);

or

CurrentGame.StartWithCurrentPlayer();

But the last method could require a second method like:

public void SetCurrentPlayer(Player player)
{
    CurrentPlayer = player;
}
  • Does Start start the game with a given player as the one who goes first? Or does it start the player playing the game? – user40980 Oct 4 '13 at 22:23
  • In this case the method starts the game with the given player. – Lucas Oct 4 '13 at 22:43
3

A line such as CurrentGame.CurrentPlayer would be a code smell to me. In fact there's something fishy about the whole question.

You say that the second method, StartWithCurrentPlayer, would require an additional SetCurrentPlayer method. But how is that different from what CurrentGame.CurrentPlayer already is in the first method? Where does the value for CurrentPlayer come from?

There seem to be 3 possible scenarios here, none of which really make sense with either of your approaches:

  1. The game knows nothing about players until it's started.

    In this scenario, I'd expect to see a method like this inside the Game class:

     public void Start(Player player)
     {
         // Set stuff up
         this.currentPlayer = player;
     }
    
  2. The player can be changed at any time.

    This is a legitimate use case, e.g. arcade or multiplayer games. It's a little difficult to plan for, though. You'd have a class that looks like this:

     public void Start()
     {
         LoadLevel(CurrentPlayer.LastLevel);
         // Or whatever - some stuff that depends on CurrentPlayer
     }
    
     public Player CurrentPlayer
     {
         get { ... }
         set { ... }
     }
    

    You'd have to be very careful to make sure you do the proper setup/cleanup when CurrentPlayer is changed, otherwise I would foresee strange errors due to Sequential Coupling. For example, what happens when you try to start a game without setting any player? Still, this is a design decision, not necessarily an error.

  3. The player must be set up prior to starting a game.

    This essentially means that the Game has no lifetime or meaning of its own without a Player object, which cries out for a dependency tree:

     public class Game
     {
         private readonly Player player;
    
         public Game(Player player)
         {
           if (player == null) throw new ArgumentNullException("player");
           this.player = player;
         }
    
         public void Start() { ... }
     }
    

Your first example violates the Tell, Don't Ask principle. You should never (well, rarely) need to call a method on an object and provide the same object's data as a parameter. That's exactly the kind of what OOP is designed to avoid with encapsulation.

Your original second example is similar to #2. It's potentially correct, but only if that is actually your intent, and it is also the most complicated.

Try to think about object lifetimes. #1 implies that the Game lasts forever, but if you switch the Player then you need to restart the game. #2 implies that the Game lasts forever but continues from its current point even when the Player is changed. And #3 implies that the Player may last a lot longer than the Game (for example a role-playing game). All are valid, but use the appropriate one for your particular situation.

3

The first way seems best (CurrentGame.Start(CurrentGame.CurrentPlayer);). You can then feed it whatever player you want. it just happens by coincidence that your example uses an established player from your current scope.

2

This depends, in part, on which piece of code is responsible for controlling who the current player is. Is it CurrentGame or the code that uses CurrentGame ("the caller")?

If CurrentGame is responsible for setting the next player after CurrentPlayer's turn is over, then exposing SetCurrentPlayer would give the caller a way interfere with CurrentGame's state. For example, if player A finishes her turn, CurrentGame would set the CurrentPlayer to Player B; however, the caller could use this method to set CurrentPlayer back to Player A before Player B's turn is over.

If the caller is responsible:

Your first option would be workable, although the caller might be overly dependent on using a property of CurrentGame to remember who was playing the last game.

If CurrentGame is responsible:

Consider a different approach, such as:

CurrentGame.SetPlayers(OrderedList<Player>)

The caller would pass in the list of players that is ordered according to the sequence of their turns. CurrentGame would just then set CurrentPlayer to the first player in the list when the game starts.

  • Your second paragraph is true but doesn't necessarily mean that anything is wrong. For example, many chess games will allow you to set up a board and then explicitly choose whose turn it is from a menu. It would be hard to encapsulate the menu choice into the game itself. – Aaronaught Oct 5 '13 at 17:55
0

Personally I'd go with the first option, because I find methods like the second option too specific. However if you had an array of players in your class, you could pass the array index instead.

Inside CurrentGame you would have something like the below for a four player game:

Player[] players = new Player[4];

Then to start the players:

CurrentGame.Start(0);
CurrentGame.Start(1);
CurrentGame.Start(2);
CurrentGame.Start(3);
  • 1
    But then I would need the player + the list to get the index. If somehow the same. Is it not so? – Lucas Oct 4 '13 at 22:02
0

First option is better as @brad already answered but i would like to suggest to not access CurrentPlayer explicitly but instead use a getter getCurrentPlayer(). IMHO you should have a setter too setCurrentPlayer. Maybe today you don´t have much computation to do and setCurrentPlayer is just an assignment as your example but if you already use setters / getters you have encapsulated all logic related to setCurrentPlayer inside a method.

Getters & Setters are a well known design pattern. You can find more about it here (http://en.wikipedia.org/wiki/Getter/setter) for example.

  • CurrentPlayer has a public getter and a private setter. It's written in C# so I don't need extra methods. – Lucas Oct 5 '13 at 11:13
  • @Lucas You did not mention before that it was C# neither there is a C# tag for this question. There is just a little sample of code too. By a design standpoint i think my suggestion is still valid as you said there are already getter / setter for CurrentPlayer. – Gustavo Coelho Oct 5 '13 at 19:07

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.