Added CombSortTest.java #825
Conversation
copied over CombSort.java from master branch to Development branch. Added CombSortTest.java to Development branch
|
|
||
| class CombSortTest { | ||
|
|
||
| //One global combSort instance variable |
There was a problem hiding this comment.
The comment can be removed, not really needed.
| //One global combSort instance variable | ||
| CombSort combSort = new CombSort(); | ||
|
|
||
| //Integer Sort Test |
There was a problem hiding this comment.
Same goes here. The naming scheme of your methods should be testSomething. Like testIntegerSort and testStringSort. Methods in Java start with a lowercase letter. Make sure to correct that everywhere else too.
There was a problem hiding this comment.
how about combSortIntegerTest ?
There was a problem hiding this comment.
No, just change it to testStringSort and testIntegerSort. Your class is named CombSortTest already, no need to add it to the method names. Alongside that, all test methods from the other classes are named using the testXXX scheme. And to keep consistency it would be better if you could change them too :)
There was a problem hiding this comment.
CycleSortTest class does not follow the consistency you mentioned ?!
it follows what I suggested :)
class CycleSortTest {
@Test
void cycleSortIntegerTest() {
CycleSort cycleSort = new CycleSort();
There was a problem hiding this comment.
If you take a look at all the other tests, they follow the convention testXXX where XXX gives some details of what is being tested. In your case, it would be testStringSort or something similar.
Edit:
Well, looks like some tests need a rename then :) They shouldn't be named like that though. Even the JUnit site lists the testXXX naming scheme as a go-to. And in all Java based open source projects I worked on they mostly used that scheme.
Well, I am in no position to force you to use it, I'm no member of this organisation. At least make the method names lowercase please. Because THAT is Java convention.
There was a problem hiding this comment.
haha, yes now you see what I mean, and now I see what you mean, I just noticed in the /search folder, the tests follow the naming convention of testXXX.
I did the changes, did you want me to also remove all the comments like this one:
//Integer Sort Test
I'm almost done, and thank you for the review and the edit suggestions. Its always good to have another pair of eyes. I admit I totally overlooked the method names.
There was a problem hiding this comment.
Would be nice if you could remove any comments that don't really add value, yes. If they are needed to explain why a certain thing is done, let them in. But something like // Integer Sort test is not really needed.
No problem :)
There was a problem hiding this comment.
alright, done.
I made a commit to the same pull request.
Do you know when will pull requests be merged with the development branch ?
|
|
||
| //compare the two results, our CombSort vs. Java's sort | ||
| Assertions.assertArrayEquals(array1,array2); | ||
|
|
There was a problem hiding this comment.
Remove empty lines at the end of each method.
|
|
||
| //Integer Sort Test | ||
| @Test | ||
| void CombSortIntegerTest(){ |
|
|
||
| //String Sort Test | ||
| @Test | ||
| void CombSortStringTest(){ |
removed unnecessary comments removed blank lines at end of method modified method names
|
Hey please make PR to master branch. |
copied over CombSort.java from master branch to Development branch.
Added CombSortTest.java to Development branch