Removal of magic numbers in CollectionTest #2091
Conversation
Sync Forked Repo
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). For more information, open the CLA check for this pull request. |
|
Can you explain what this PR is about? The description is not very helpful. |
| assertEquals(17, target.pop().intValue()); | ||
| assertEquals(13, target.pop().intValue()); | ||
| assertEquals(11, target.pop().intValue()); | ||
| for (int i = 2; i > 0; i--){ |
There was a problem hiding this comment.
Shouldn't this be >= 0, otherwise it misses the element at value[0]. Maybe would also be good to to replace the 2 with value.length - 1.
I wanted to remove magic numbers of this class. Magic numbers are not explicit to the project and subsequently one can forget what they correspond to. In my opinion, deleting them will allow future contributors to work more efficiently on the project. |
Lhandi29
changed the title
|
While I agree that magic numbers are often a hindrance, here I don't think there's much advantage to the rewrite. It's pretty obvious that the 3 in One change that I think would be worthwhile would be to use Truth for the assertions. Since instead of checking the size and removing the elements one by one. This doesn't test that the elements would be retrieved in the expected order from the queue, but they must be if I'm a bit surprised to see that Truth is not yet a dependency of the main Please do sign the CLA if you do decide to go ahead with this. |
in CollectionTest.testCollectionOfStringsDeserialization and CollectionTest.testRawCollectionDeserializationNotAlllowed
in CollectionTest.testRawCollectionOfIntegersSerialization
in CollectionTest.testTopLevelCollectionOfIntegersSerialization
in CollectionTest.testTopLevelCollectionOfIntegersDeserialization
in CollectionTest.testTopLevelListOfIntegerCollectionsDeserialization
in CollectionTest.testLinkedListSerialization
in CollectionTest.testLinkedListDeserialization
in CollectionTest.testQueueSerialization
in CollectionTest.testQueueDeserialization
in CollectionTest.testPriorityQueue
in CollectionTest.testVector
in CollectionTest.testStack
in CollectionTest.testNullsInListSerialization
in CollectionTest.testNullsInListDeserialization
in CollectionTest.testCollectionOfObjectSerialization
in CollectionTest.testCollectionOfObjectWithNullSerialization
in CollectionTest.testCollectionOfStringsSerialization
in CollectionTest.testCollectionOfBagOfPrimitivesSerialization
in CollectionTest.testRawCollectionSerialization
in CollectionTest.testRawCollectionDeserializationNotAlllowed
in CollectionTest.testRawCollectionOfBagOfPrimitivesNotAllowed
in CollectionTest.testWildcardPrimitiveCollectionSerilaization
in CollectionTest.testWildcardPrimitiveCollectionDeserilaization
in CollectionTest.testFieldIsArrayList
in ObjectWithWildcardCollection.testSetSerialization
in ObjectWithWildcardCollection.testSetDeserialization
in SmallClass.testIssue1107
| @@ -41,6 +45,10 @@ | |||
| </configuration> | |||
| </execution> | |||
| </executions> | |||
| <configuration> | |||
There was a problem hiding this comment.
Is this required for your change to work?
Sync Forked Repo