Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
Stanislav-Zabramnyi merged 3 commits into main from GP-155-random-field-comparator-exercise
Aug 11, 2022

Conversation

Copy link
Contributor

@Stanislav-Zabramnyi Stanislav-Zabramnyi commented Aug 10, 2022
edited
Loading

No description provided.

tboychuk reacted with eyes emoji
Copy link
Contributor

@tboychuk tboychuk left a 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)


@Test
@Order(1)
@DisplayName("constructor should throw exception if parameter is null")
Copy link
Contributor

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

Copy link
Contributor

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


@Test
@Order(2)
@DisplayName("constructor should throw exception if provided type does not contain any fields")
Copy link
Contributor

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.

@Test
@Order(3)
@SneakyThrows
@DisplayName("target type must contain at least one field which implements Comparable interface")
Copy link
Contributor

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")


@Test
@Order(4)
@DisplayName("'compare' should throw exception if any parameter is null")
Copy link
Contributor

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


@Test
@Order(6)
@DisplayName("'compare' treats null value greater than a non-null value, so it should return 1 if field value of " +
Copy link
Contributor

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")


@SneakyThrows
private <T> void setFieldToCompare(String fieldName, Class<T> classType) {
Field fieldToCompare = randomFieldComparator.getClass().getDeclaredField("fieldToCompare");
Copy link
Contributor

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();
 }

.findAny().orElseThrow(() -> new IllegalArgumentException("There are no fields available to compare"));
}

private boolean isComparableType(Class<?> type) {
Copy link
Contributor

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)


private Field chooseFieldToCompare(Class<T> targetType) {
return Arrays.stream(targetType.getDeclaredFields())
.filter(f -> isComparableType(f.getType()))
Copy link
Contributor

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.

Objects.requireNonNull(o1);
Objects.requireNonNull(o2);
fieldToCompare.setAccessible(true);

Copy link
Contributor

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.


var field1 = (Comparable) fieldToCompare.get(o1);
var field2 = (Comparable) fieldToCompare.get(o2);

Copy link
Contributor

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?

Copy link
Contributor Author

@Stanislav-Zabramnyi Stanislav-Zabramnyi Aug 10, 2022
edited
Loading

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.

Copy link
Contributor

@tboychuk tboychuk left a 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! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tboychuk tboychuk tboychuk approved these changes

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /