Issue #1: Added unit tests for CharacterRecognizer. #31

FridaTveit · 2016-12-19T19:15:03Z

Changed nested classes in CharacterRecognizer to be static to make them easier to test.

…sted classes in CharacterRecognizer to be static to make them easier to test.

coveralls · 2016-12-19T19:18:06Z

Coverage increased (+1.2%) to 13.138% when pulling 5b15d69 on FridaTveit:AddUnitTestsForCharacterRecognizer into 8f12182 on oskopek:master.

oskopek

Thanks for the pull request @FridaTveit! I've have a few nitpicks I left in the comments for you. The ones marked optional do not necessarily have to be addressed.

If you don't feel like fixing them at all I will merge in a few days anyway, as the test is great! Thank you for your time and effort :)

oskopek · 2016-12-20T10:06:50Z

src/test/java/net/sf/javaanpr/recognizer/CharacterRecognizerTest.java

+import java.util.Vector;
+
+import static org.junit.Assert.*;
+import static org.hamcrest.CoreMatchers.*;


I think it would be better if we used raw junit for assertions, if it's not too much of a pain. As far as I can tell, all the assertThat(..) statements in this test can be substituted with assertEquals.
If in the future we find out that we need Hamcrest's matchers, lets use them and add them to our dependencies.

oskopek · 2016-12-20T10:16:46Z

src/main/java/net/sf/javaanpr/recognizer/KnnPatternClassificator.java

        for (int x = 0; x < this.learnVectors.size(); x++) {
            float fx = this.simplifiedEuclideanDistance(tested, this.learnVectors.elementAt(x));
-            recognized.addPattern(recognized.new RecognizedPattern(ALPHABET[x], fx));
+            recognized.addPattern(new RecognizedChar.RecognizedPattern(ALPHABET[x], fx));


Optional: The way the RecognizedPattern, RecognizedChar and PatternComparer classes are nested in CharacterRecognizer is a pain. It's good that you made them static, but I would suggest going even further and pulling them out into separate files.

oskopek · 2016-12-20T10:23:24Z

src/test/java/net/sf/javaanpr/recognizer/CharacterRecognizerTest.java

+
+public class CharacterRecognizerTest {
+
+    private CharacterRecognizer.RecognizedChar getRecognizedCharWithThreePatterns() {


Optional: This is an ok way to do create a RecognizedChar for testing purposed, but the way this is usually done in Junit tests is adding a setup method and then using the variable directly in the tests.

public class CharacterRecognizerTest { private RecognizedChar threePaternRecognizedChar; @Before private void setUp() { // ... contents of current getRecognizedCharWithThreePatterns() }

oskopek · 2016-12-20T10:42:57Z

src/test/java/net/sf/javaanpr/recognizer/CharacterRecognizerTest.java

+        recognizedChar.sort(0);
+        assertTrue(recognizedChar.isSorted());
+        Vector<CharacterRecognizer.RecognizedChar.RecognizedPattern> patterns = recognizedChar.getPatterns();
+        assertThat(patterns.get(0).getCost(), is(1.0f));


Be careful with this, as floats tend to be "not equal" at the weirdest times... See https://stackoverflow.com/questions/31776127/how-to-test-for-equality-of-float-double-in-junit for more information.

Thanks, changed it now! :)

…before setup method.

…edPattern. They are no longer nested inside CharacterRecognizer but have their own files.

…lasses in CharacterRecognizer have been extracted.

…ts as they were really testing RecognizedChar. CharacterRecognizer and RecognizedPattern don't really have anything to test.

coveralls · 2016-12-20T23:13:16Z

Coverage increased (+1.5%) to 13.469% when pulling 93ce0ae on FridaTveit:AddUnitTestsForCharacterRecognizer into 8f12182 on oskopek:master.

oskopek

I left a few more optional comments. Sorry I didn't find them before.

Feel free to ignore them, the PR is great as it is :) I'll merge it anyway if you don't get around to fixing them.

oskopek · 2016-12-21T18:58:06Z

src/main/java/net/sf/javaanpr/recognizer/PatternComparer.java

+
+import java.util.Comparator;
+
+public class PatternComparer implements Comparator<Object> {


Optional: Lets make it an implementation of Comparator<RecognizedPattern>, so that we can avoid the ugly casts on lines 30-31.

Optional: Rename the class to standard English, i.e. PatternComparator (also the test).

oskopek · 2016-12-21T18:58:56Z

src/main/java/net/sf/javaanpr/recognizer/PatternComparer.java

+public class PatternComparer implements Comparator<Object> {
+    private int direction;
+
+    public PatternComparer(int direction) {


Optional: It makes no sense to have a direction int, if it only ever has two values. boolean?

oskopek · 2016-12-21T19:02:27Z

src/main/java/net/sf/javaanpr/recognizer/PatternComparer.java

+    }
+
+    @Override
+    public int compare(Object o1, Object o2) {


Optional: Also, if you're up for it, the actual implementation of the compare method could be greatly simplified using a ternary operator + Integer.compareTo.

Too bad we're still on Java 6 and don't have the static Integer.compare (JDK7+). On a side note, what do you think about moving to Java 7/Java 8 with the project?

Done :) I think that sounds like a good idea!

oskopek · 2016-12-21T19:06:24Z

src/test/java/net/sf/javaanpr/recognizer/PatternComparerTest.java

+    public void testCompareAscendingFirstPatternSmallerReturnsMinusOne() {
+        RecognizedPattern recognizedPattern1 = new RecognizedPattern('A', 1.0f);
+        RecognizedPattern recognizedPattern2 = new RecognizedPattern('A', 2.0f);
+        assertEquals(new PatternComparer(0).compare(recognizedPattern1, recognizedPattern2), -1);


Optional: A more robust way of testing a comparator wold be to test for equality with 0, < 0 and > 0. See the definition of Comparable for more info.

…mparator<RecognizedPattern>. Changed PatternComparator to take a boolean shouldSortDescending instead of int direction.

…mparatorTest to check for < 0 and > 0 instead of = -1 and = 1.

coveralls · 2016-12-21T20:32:16Z

Coverage increased (+1.5%) to 13.462% when pulling dc3804a on FridaTveit:AddUnitTestsForCharacterRecognizer into 8f12182 on oskopek:master.

oskopek · 2016-12-21T20:38:44Z

Wonderful, thanks a lot @FridaTveit!

Issue oskopek#1: Added unit tests for CharacterRecognizer. Changed ne…

5b15d69

…sted classes in CharacterRecognizer to be static to make them easier to test.

oskopek approved these changes Dec 20, 2016

View reviewed changes

FridaTveit added 4 commits December 20, 2016 17:56

Changed CharacterRecognizerTest to only use junit assertions and use @…

842b063

…before setup method.

Extracted nested classes PatternComparer, RecognizedChar and Recogniz…

89ed072

…edPattern. They are no longer nested inside CharacterRecognizer but have their own files.

Updated CharacterRecognizerTest to reflect the fact that the nested c…

87b7575

…lasses in CharacterRecognizer have been extracted.

Added unit tests for PatternComparer. Renamed CharacterRecognizer tes…

93ce0ae

…ts as they were really testing RecognizedChar. CharacterRecognizer and RecognizedPattern don't really have anything to test.

FridaTveit mentioned this pull request Dec 20, 2016

Issue #1: Recognized plate add unit tests #33

Merged

oskopek approved these changes Dec 21, 2016

View reviewed changes

FridaTveit added 2 commits December 21, 2016 19:45

Renamed PatternComparer to PatternComparator and made it implement Co…

ce322b7

…mparator<RecognizedPattern>. Changed PatternComparator to take a boolean shouldSortDescending instead of int direction.

Simplified the compare method in PatternComparator. Changed PatternCo…

dc3804a

…mparatorTest to check for < 0 and > 0 instead of = -1 and = 1.

oskopek merged commit b80b810 into oskopek:master Dec 21, 2016

FridaTveit deleted the AddUnitTestsForCharacterRecognizer branch December 21, 2016 20:39


		public class CharacterRecognizerTest {

		private CharacterRecognizer.RecognizedChar getRecognizedCharWithThreePatterns() {


		import java.util.Comparator;

		public class PatternComparer implements Comparator<Object> {

Issue #1: Added unit tests for CharacterRecognizer. #31

Issue #1: Added unit tests for CharacterRecognizer. #31

Uh oh!

Conversation

FridaTveit commented Dec 19, 2016

Uh oh!

coveralls commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oskopek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oskopek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oskopek commented Dec 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Dec 19, 2016 •

edited

Loading

coveralls commented Dec 20, 2016 •

edited

Loading

coveralls commented Dec 21, 2016 •

edited

Loading