Code review -- can I simplify?

Good day,

I'm wondering if anyone could tell me if there was a way to shorten the code below. It seems to be a lot of "ceremony" to accomplish such a simple thing.

It's a 4-player game and before adding a new player to the size-4 array I want to confirm the color chosen by the new Player has not already been taken. It seems that the choice of using an enum is making things way more complicated than they should?

Thank you!

#[derive(Debug, Clone, PartialEq)]
enum Color {Red, Yellow, Green, Blue }

#[derive(Debug,Clone)]
enum Player {
  Player1 {name: String, color: Color},
  Player2 {name: String, color: Color},
  Player3 {name: String, color: Color},
  Player4 {name: String, color: Color},
}

// add new player p to array of Option<Players> if color chosen by the new player 
// has not already been taken
fn add_player(players: &mut [Option<Player>; 4], p: Player) {
  let desiredcolor: Color;

  // extract desired color from new Player p
  match p {
    Player::Player1 { name:_, color: c } => desiredcolor = c, 
    Player::Player2 { name:_, color: c } => desiredcolor = c, 
    Player::Player3 { name:_, color: c } => desiredcolor = c, 
    Player::Player4 { name:_, color: c } => desiredcolor = c, 
  } 
  
  // verify if color already taken by any other Player in the array
  if players.iter().any(|x|
    {
      // does the match need not to consume the Option?
      match x {
        &Some(Player::Player1 {name: _, color: ref c }) if desiredcolor == *c => true,
        &Some(Player::Player2 {name: _, color: ref c }) if desiredcolor == *c => true,
        &Some(Player::Player3 {name: _, color: ref c }) if desiredcolor == *c => true,
        &Some(Player::Player4 {name: _, color: ref c }) if desiredcolor == *c => true,
        _ => false,
      }
    }) 
  {
    println!("Error: Color taken already");
  } else {
    println!("Accepted: Color was free to take");
  }
}

fn main() {

  let mut players = [
    Some(Player::Player2
      {
        name: "John".to_string(),
        color: Color::Red
      }),
    Some(Player::Player4
      {
        name: "Bob".to_string(),
        color: Color::Blue
      }),
    None,  
    None,
  ];

  // success -- color did not exist
  let newplayer3 = Player::Player3 { name: "Alice".to_string(), color: Color::Green};
  add_player(&mut players, newplayer3);

  // error -- color already existed
  let newplayer1 = Player::Player1 { name: "Dave".to_string(), color: Color::Red};
  add_player(&mut players, newplayer1);
}

You could use struct Player and enum {P1(Player), P2(Player)}, and add Deref or get_player(&self) -> &Player to the enum.

Another option would be to just use Vec<Player> and instead of relying on the enum, just search the list of existing players for duplicates.

1 Like

Yeah, I think using an enum for Player wasn't a good choice. I've made a version I think is better structured here. Note that the add function does the same as yours, namely just print stuff. It should probably add the Player to Players, and return the index after insertion. Not sure how much you know about it, but finally the add function should return a Result, so you can transport any error that might have occured.

If you have more questions, just ask :slight_smile:

2 Likes

You just whipped that up really quickly. :slight_smile: I like your choice of a tuple struct for Player, it did not occur to me initially. Yes, I was aware of Result but I wanted to keep to the minimum. Thanks for your answer!

I will experiment in implementing the enum perhaps this will make things more concise in the end.

Experimenting is always good :slight_smile: Of course, just let me say, what's more concise/elegant/etc. ppp. depends vastly on what you're doing with this, I assume you'll implement more stuff than what you posted, and that might of course change things a bit. Good luck!

1 Like