-
-
Notifications
You must be signed in to change notification settings - Fork 501
GP-155: add random-field-comparator exercise #135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GP-155: add random-field-comparator exercise #135
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Stanislav-Zabramnyi great job! 👏 It's your first exercise, right?
I've added a bunch of comments which are mostly related to the naming convention. You'll get used to it soon.
Action points for the next time:
- create a draft PR for the exercise (no solution yet, so I can see what the customers will see)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Stanislav-Zabramnyi let's skip the word should
in tests.
E.g. constructor throws exception if parameter is null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For those tests where we user @DisplayName
, lets' start with a capital letter.
A constructor throws exception if parameter is null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is redundant, since it's covered by the next test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DisplayName("Constructor throws an exception when the target type has no Comparable fields")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Stanislav-Zabramnyi let's follow the same naming convention. E.g. in other tests I usually go with something like this Method compare throws an exception when any param is null
I guess the pattern X does Y when Z
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DisplayName("Method compare returns 1 when the first field value is null")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛔️ My implementation didn't pass the tests not because it was incorrect, but because the name of the field was targetField
not fieldToCompare
.
What we want to do is either explicitly say in the test that which field name you expect. E.g. have a test
A class has a field called fieldToCompare of type Field
– but I don't think it's a good idea in this exercise.
Or just be more flexible and let people come up with any name for that field.
For instance, you can rely on type instead of the name
private Field getFieldToCompare() {
return Arrays.stream(randomFieldComparator.getClass().getDeclaredFields())
.filter(f -> f.getType().equals(Field.class))
.findAny()
.orElseThrow();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace this method with
Comparable.class.isAssignableFrom(type)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your filter logic will ignore all primitive fields 😅 which is not cool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you can to the Comparable, it would be better to force the type. So you know that value1
and value2
have the same type comparable type (currently you use Comparable as a raw type). I'm not sure if you know what I mean. I can show it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following logic is a natural null-safe compression. So I'd use Comparator.nullsLast(Comparator.naturalOrder())
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I thought about this way of implementation, but then I decided that it is not obvious and may increase the cognitive load of the person doing this exercise, since this logic with Comparator is not related to the topic of reflection. In short, case with 'if' more readable.
6739bb1
to
4b69006
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part looks good! 👍
Uh oh!
There was an error while loading. Please reload this page.
No description provided.