Skip to main content
Corrected static readonly properties cannot be fields
Source Link
Rick Davin
  • 6.4k
  • 1
  • 19
  • 31

I gave this a very quick review while still on my first cup of coffee. The answer is not very detailed, but then there were a lot of things I caught even with a quick scan.

The public fields at the top of the RockPaperScissor class should either be private fields or public properties. The one exception to public propertiesfields are constants or. Note a static readonly valuesvalue cannot be a constant, so if you have any of those they should be properties (be it public or private). If

If you use public properties, the naming should be Pascal-cased, so isPlaying should be IsPlaying (if you think it should be exposed publicly). Private fields or properties should begin with lowercase; public fields or properties should begin with uppercase.

isPlaying is initialized to true. It should be false and you should set it to true at the top of PlayGame. You may want to put a check at the top to make sure a game is not already in progress.

I'd like to see a enum for { Rock, Paper, Scissors } instead of literals "ROCK", "PAPER", and "SCISSORS". This requires better validation for the user input to map that input to the correct enum. I would also recommend using a switch instead of cascading if-else-if's.

ShowResult(int flagNum) could be improved. I don't like flagNum as an integer and would prefer to see an enum of GameResult = { InProgress, ComputerWins, PlayerWins, Draw }. There should be a class level public readonly property to hold the GameResult. And ShowResult should not take any parameters but instead read that class level public readonly property.

I gave this a very quick review while still on my first cup of coffee. The answer is not very detailed, but then there were a lot of things I caught even with a quick scan.

The public fields at the top of the RockPaperScissor class should either be private fields or public properties. The one exception to public properties are constants or static readonly values. If you use public properties, the naming should be Pascal-cased, so isPlaying should be IsPlaying (if you think it should be exposed publicly). Private fields or properties should begin with lowercase; public fields or properties should begin with uppercase.

isPlaying is initialized to true. It should be false and you should set it to true at the top of PlayGame. You may want to put a check at the top to make sure a game is not already in progress.

I'd like to see a enum for { Rock, Paper, Scissors } instead of literals "ROCK", "PAPER", and "SCISSORS". This requires better validation for the user input to map that input to the correct enum. I would also recommend using a switch instead of cascading if-else-if's.

ShowResult(int flagNum) could be improved. I don't like flagNum as an integer and would prefer to see an enum of GameResult = { InProgress, ComputerWins, PlayerWins, Draw }. There should be a class level public readonly property to hold the GameResult. And ShowResult should not take any parameters but instead read that class level public readonly property.

I gave this a very quick review while still on my first cup of coffee. The answer is not very detailed, but then there were a lot of things I caught even with a quick scan.

The public fields at the top of the RockPaperScissor class should either be private fields or public properties. The one exception to public fields are constants. Note a static readonly value cannot be a constant, so if you have any of those they should be properties (be it public or private).

If you use public properties, the naming should be Pascal-cased, so isPlaying should be IsPlaying (if you think it should be exposed publicly). Private fields or properties should begin with lowercase; public fields or properties should begin with uppercase.

isPlaying is initialized to true. It should be false and you should set it to true at the top of PlayGame. You may want to put a check at the top to make sure a game is not already in progress.

I'd like to see a enum for { Rock, Paper, Scissors } instead of literals "ROCK", "PAPER", and "SCISSORS". This requires better validation for the user input to map that input to the correct enum. I would also recommend using a switch instead of cascading if-else-if's.

ShowResult(int flagNum) could be improved. I don't like flagNum as an integer and would prefer to see an enum of GameResult = { InProgress, ComputerWins, PlayerWins, Draw }. There should be a class level public readonly property to hold the GameResult. And ShowResult should not take any parameters but instead read that class level public readonly property.

Source Link
Rick Davin
  • 6.4k
  • 1
  • 19
  • 31

I gave this a very quick review while still on my first cup of coffee. The answer is not very detailed, but then there were a lot of things I caught even with a quick scan.

The public fields at the top of the RockPaperScissor class should either be private fields or public properties. The one exception to public properties are constants or static readonly values. If you use public properties, the naming should be Pascal-cased, so isPlaying should be IsPlaying (if you think it should be exposed publicly). Private fields or properties should begin with lowercase; public fields or properties should begin with uppercase.

isPlaying is initialized to true. It should be false and you should set it to true at the top of PlayGame. You may want to put a check at the top to make sure a game is not already in progress.

I'd like to see a enum for { Rock, Paper, Scissors } instead of literals "ROCK", "PAPER", and "SCISSORS". This requires better validation for the user input to map that input to the correct enum. I would also recommend using a switch instead of cascading if-else-if's.

ShowResult(int flagNum) could be improved. I don't like flagNum as an integer and would prefer to see an enum of GameResult = { InProgress, ComputerWins, PlayerWins, Draw }. There should be a class level public readonly property to hold the GameResult. And ShowResult should not take any parameters but instead read that class level public readonly property.