Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't rotate the player assigned to the role 'random' when generating… #86

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tflaherty
Copy link
Contributor

… multiple matches

The current code rotates all players when generating multiple matches
for the same game. This change leaves the player associated with the
'random' role at the same index in the player list and rotates all the
other players. If there is no player associated with the 'random' role,
the code works as before.

… multiple matches

The current code rotates all players when generating multiple matches
for the same game. This change leaves the player associated with the
'random' role at the same index in the player list and rotates all the
other players. If there is no player associated with the 'random' role,
the code works as before.
@samschreiber
Copy link
Member

Sorry for the slow response here. Was travelling; now catching up on everything I missed. This is on my radar.

@samschreiber
Copy link
Member

Some initial thoughts:

  • the code that computes the names of the roles for a given game seems like something that could be factored out, maybe even put in Game.java? (Also, I wish there were a way to do this which didn't require initializing an entire ProverStateMachine ... that feels very heavyweight, but maybe it's necessary to do this)
  • I'm not sure this is the right level of abstraction to implement this -- in the future we may want the Game and StateMachine classes to understand the semantics of a "random" role, so that the applications built on top of that infrastructure (e.g. Server, Player, Kiosk, etc) aren't even aware that the "random" roles exist. This shouldn't deter us from starting by implementing this logic in Server; but eventually we may want to move it out into lower levels of the stack.

@tflaherty
Copy link
Contributor Author

Sam, I agree with you on both points. I will spend some time on these and send along my proposed solutions. Do you plan on checking this in in the current state? It might take me a week or two to get back to this.

@tflaherty
Copy link
Contributor Author

Sam, not a problem on the delay. I figured you might be on vacation or travelling.

@@ -244,10 +244,38 @@ public void actionPerformed(ActionEvent evt) {
thePlayers.add(playerSelector.getPlayerPresence(name));
}

StateMachine stateMachine = new ProverStateMachine();
stateMachine.initialize(theGame.getRules());
List<Role> roles = stateMachine.getRoles();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use Role#computeRoles for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants