These are my code comments for the session 2 (2017-10-25).
Before, a quick recap on some important things.
Recap
Baby steps
We defined that we would test the class invariants: the conditions that should hold true during the entire class lifetime.
But we should test each condition separately. For example, the following exception does not help:
if (wins < 0 && losses < 0) {
throw new IllegalArgumentException("Number of wins and losses cannot be negative");
}As a client developer, I need to debug the code to know which value is wrong when I read this exception message. The right way is:
if (wins < 0) {
throw new IllegalArgumentException("Number of wins cannot be negative");
}
if (losses < 0) {
throw new IllegalArgumentException("Number of losses cannot be negative");
}Now, when I make a mistake, I know rapidly which value is wrong. So, let's check each condition independently and throw an exception for each violation.
After testing all conditions, we can refactor what can be refactored. Premature optimization is the root of all evil also when we write code. Most of the time we read code so, let's not be lazy to write code.
In the tests, it important to test for the exception message. Teams Frances Allen and Karen Jones tested for the exception message using different approaches (the Rules engine and the old school try/catch block, respectively).
Last but not least, the code coverage report is your friend to check if each condition is really tested!
Code coverage
Read the README file. The Grade task buildComplete will run the project's test suite and generate the code coverage report. Check it!
$ ./gradlew buildComplete
Java's code style
Let's check if we can make the IDEA's Reformat Code tool to work.
Anyways, it is important to be familiar with Java's code style. It is simple.
- We'll keep using soft tabs (i.e. spaces) for indentation. 4 spaces per level
- We'll also use the 120 as the line limit
Commit and exception messages
Commit and exception messages should follow the same rules we apply for any other text. For example, the following exception message has an error:
totalRolls must be Greater than zero
Greater should be written greater as it is not a proper name. totalRolls is right because it refers to a property name.
And as commit messages should explains what and why something was added/changed/removed, an exception message should also be clear. It's all about communication. When an exception happens, it should inform the developer what happened. Good example of exception message:
The sum of wins and losses must be greater than zero
The newspaper metaphor
From Uncle Bob's (Robert C. Martin) Clean Code book (p11):
Think of a well-written newspaper article. You read it vertically. At the top you expect a headline that will tell you what the story is about and allows you to decide whether it is something you want to read. The first paragraph gives you a synopsis of the whole story, hiding all the details while giving you the broad-brush concepts. As you continue downward, the details increase until you have all the dates, names, quotes, claims, and other minutia.
We would like a source file to be like a newspaper article. The name should be simple but explanatory. The name, by itself, should be sufficient to tell us whether we are in the right module or not. The topmost parts of the source file should provide the high-level concepts and algorithms. Detail should increase as we move downward, until at the end we find the lowest level functions and details in the source file.
A newspaper is composed of many articles; most are very small. Some are a bit larger. Very few contain as much text as a page can hold. This makes the newspaper usable. If the newspaper were just one long story containing a disorganized agglomeration of facts, dates, and names, then we simply would not read it.
This metaphor can be used for the test classes too. Think on the GameStatisticsTest class: the tests for the exceptional cases should come before the other test cases. The exceptions happens before the object can be created.
Team Frances Allen
Commit message
You improved a lot, well done!
But attention for the following:
265b5ac
On the commit body:
Remove unneded methods in GameStatistics class and change tests attributes as constants.
- Typo:
unneded -> unneeded
- Grammar:
attributes as constants -> attributes to constants
7912d3f
You can rewrite the message using the git rebase command. Check #2 for more details.
Code
- Fix the code style violations (IDEA has an option for that)
GameStatistics
Code style:
- There are unnecessary line breaks in the class
- There is an unused import statement:
IllegalFormatException
- There is an unneeded class comment
- Illegal space in the
if statements' parentheses
- Add the
public modifier to the class constructor
import java.util.IllegalFormatException;
/**
* Created by nayvir on 10/19/17.
*/
public class GameStatistics {
// ...
public final int averageRolls;
public final float probabilityWin;
GameStatistics (int wins, int losses, int totalRolls, int longestPlay, int numberOfWinsFirst, int numberOfLossesFirst, int averageRolls) {
if ( wins < 0 )
throw new IllegalArgumentException("Negative wins are not allowed");
if ( losses < 0 )
throw new IllegalArgumentException("Negative losses are not allowed");
if (wins + losses == 0) throw new IllegalArgumentException("The sum of wins and losses must be greater than zero");
if ( totalRolls < 1 )
throw new IllegalArgumentException("totalRolls must be Greater than zero");
this.wins = wins;
// ...
this.probabilityWin = wins / numberOfGames;
}
}Should be:
public class GameStatistics {
// ...
public final int averageRolls;
public final float probabilityWin;
public GameStatistics(int wins, int losses, int totalRolls, int longestPlay, int numberOfWinsFirst,
int numberOfLossesFirst, int averageRolls) {
if (wins < 0)
throw new IllegalArgumentException("Negative wins are not allowed");
if (losses < 0)
throw new IllegalArgumentException("Negative losses are not allowed");
if (wins + losses == 0)
throw new IllegalArgumentException("The sum of wins and losses must be greater than zero");
if (totalRolls < 1)
throw new IllegalArgumentException("totalRolls must be Greater than zero");
this.wins = wins;
// ...
this.probabilityWin = wins / numberOfGames;
}
}Exception message:
The exception message:
totalRolls must be Greater than zero
Should be:
totalRolls must be greater than zero
GameStatisticsTest
Code style:
- There is an unused import statement:
Rule
Initialization:
@Before
public void setup() {
gameStatistics = new GameStatistics(WINS, LOSSES, TOTAL_ROLLS, LONGEST_PLAY, NUMBERS_WINS_FIRST_ROLL, NUMBERS_LOSSES_FIRST_ROLL, AVERAGE_ROLL);
}As we discussed in #2, there is no need for this set up method. GameStatistics is a Value Object. So:
// ...
private static final int AVERAGE_ROLL = 15;
GameStatistics gameStatistics = new GameStatistics(WINS, LOSSES, TOTAL_ROLLS, LONGEST_PLAY, NUMBERS_WINS_FIRST_ROLL, NUMBERS_LOSSES_FIRST_ROLL, AVERAGE_ROLL);
Code explicity:
@Test
public void createGameStatisticsResultsProbabilityOfWin() {
assertEquals(gameStatistics.probabilityWin, (WINS / NUMBERS_GAMES), 0);
}I prefer to be clear on the values I expect. So:
assertThat(gameStatistics.probabilityWin, closeTo(0.4285, TOLERANCE));
Where TOLERANCE is a very small number:
private static final double TOLERANCE = 1e-6;
This way, the reader of the code knows in a first sight the expected value.
Properties names:
- Methods names: Check #2
NUMBERS_GAMES -> NUMBER_OF_GAMESNUMBERS_WINS_FIRST_ROLL -> NUMBERS_WINS_COMING_ROLL (it's important to name the properties and behaviors using the domain's language, which in this case is coming roll, not first roll)
Team Karen Jones
Commit message
cfad4bd
Commit message:
Validate that GameStatistics cannot be instantiated with negative Wins
Add test creatingGameStatisticsThrowsExceptionWhenWinsIsNegativeOnInstantiation
that validates that an IllegalParameter exception is thrown when instantiating
GameStatistics with wins Negative.
- The subject exceeds the 50 character limit
- Explaining that a test case was added in a commit message makes sense only when the commit is about adding a new test case
A better example:
Check GameStatistics wins invariant
There is no way that a game can have negative wins.
Code
- Fix the code style violations (IDEA has an option for that)
GameStatistics
Exception type:
import java.security.InvalidParameterException;
This is an exception intended to be used by the Java internals. The right exception to use is IllegalArgumentException, which is available for every Java class without needing an explicit import.
So, remove this import and replace the InvalidParameterException with IllegalArgumentException.
Properties names:
losesInComingOutRoll -> lossesInComingOutRollwinnerProbability -> winningProbabilitywinnerProbabilityInComingOutRoll -> winningProbabilityInComingOutRolllosesProbabilityInComingOutRoll -> losingProbabilityInComingOutRoll
GameStatisticsTest
Initialization:
int numLoses, numWins, longestPlayedGame, rollsMade, winsInComingOutRoll, losesInComingOutRoll;
GameStatistics stats;
private static final int NUM_LOSSES = 10;
private static final int NUM_WINS = 10;
private static final int LONGEST_PLAYED_GAME = 10;
private static final int ROLLS_MADE = 10;
private static final int WINS_IN_COMMING_OUT_ROLL = 10;
private static final int LOSSES_IN_COMMING_OUT_ROLL = 10;
numLoses = 10;
numWins = -10;
longestPlayedGame = 34;
rollsMade = 150;
winsInComingOutRoll = 4;
losesInComingOutRoll = 3;
@Before
public void createGameStatistics() {
stats = new GameStatistics(numWins, numLoses, longestPlayedGame, rollsMade, winsInComingOutRoll, losesInComingOutRoll);
}I think the refactoring to constants wasn't finished. Check the comment for the Team Frances Allen (there is no need for this @Before method).
Code explicity:
Check the comment for the Team Frances Allen.
For example:
@Test
public void creatingGameStatisticsResultsInWinnerProbability() {
double probability = ((double)numWins / (numWins + numLoses));
assertThat(stats.winnerProbability(), closeTo(probability, 0.00005));
}I would refactor to:
@Test
public void creatingGameStatisticsResultsInWinnerProbability() {
assertThat(stats.winnerProbability(), closeTo(0.5, TOLERANCE));
}This way I don't need to check the values for numWins and numLoses and calculate the probability in my head. TOLERANCE is a very small number:
private static final double TOLERANCE = 1e-6;
Properties names:
Let's not abbreviate:
NUM_WINS -> NUMBER_OF_WINSNUM_LOSSES -> NUMBER_OF_LOSSES
Team Radia Perlman
Commit message
Code
- Fix the code style violations (IDEA has an option for that)
GameStatistics
Code style:
Delete it!
Exceptions:
Check "Baby steps" on "Recap" above.
if (numWins == 0 && numLoses == 0) {
throw new IllegalArgumentException("Number of wins and loses cannot be zero.");
}
if (numWins < 0 || numLoses < 0 ) {
throw new IllegalArgumentException("Number of wins and loses cannot be negative.");
}Properties names:
Let's not abbreviate!
numWins -> numberOfWinsnumLoses -> numberOfLossesnumGames -> numberOfGameslengthLongestGame -> lengthOfLongestGamenumTotalRolls -> numberOfTotalRollsaverageNumRoll -> averageNumberOfRollsnumWinsOnComingOutRoll -> numberOfWinsOnComingOutRollnumLosesOnComingOutRoll -> numberOfLosesOnComingOutRoll
GameStatisticsTest
Code explicity:
The threshold value of the closeTo match can be extracted to a constant:
@Test
public void calculateWinningOnComingPutRollProbability() {
assertThat(gameStatistics.winningOnComingPutRollProbability, closeTo(0.16,0.01));
}I would refactor to:
assertThat(gameStatistics.winningOnComingPutRollProbability, closeTo(0.16, TOLERANCE));
Where TOLERANCE is a very small number:
private static final double TOLERANCE = 1e-6;
Initialization:
- Extract the 5 and 2 literals to constants (check #2 for "raw numbers")
private GameStatistics gameStatistics = new GameStatistics(GAMES_WON, GAMES_LOST, 5,TOTAL_ROLLS, 5, 2);
Properties names:
Let's not use idioms from other languages. "Raises error" is a Python idiom. Java "throws" exceptions.
raisesErrorWhenNumberOfGamesIsZero -> throwsExceptionWhenNumberOfGamesIsZeroraisesErrorWhenNunberOfWinsLessThenZero -> throwsExceptionWhenNunberOfWinsLessThenZeroraisesErrorWhenNunberOfLosesLessThenZero -> throwsExceptionWhenNunberOfLosesLessThenZero
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent