Skip to content
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

TimeComparator and Comparator Merge? #5

Open
bertzzie opened this issue Feb 20, 2018 · 3 comments
Open

TimeComparator and Comparator Merge? #5

bertzzie opened this issue Feb 20, 2018 · 3 comments
Assignees
Labels
question Further information is requested

Comments

@bertzzie
Copy link

Both seems to have the same purpose, only with separate terms. I think we need to just merge it into one, but we might want to discuss this further before any changes.

Thoughs?

@bertzzie bertzzie added the question Further information is requested label Feb 20, 2018
@bunthatcodes
Copy link
Contributor

It did cross my mind to use the same Comparator for both number and date comparison but imo separating them into two comparators with different terms (i.e. GREATER_THAN and AFTER) provide more clarity for the context in which they are used for the users

@andirdju
Copy link
Member

can we link or show the related codes in the discussion, so it's more clear what we are talking about.

@bunthatcodes
Copy link
Contributor

bunthatcodes commented Feb 21, 2018

@bertzzie was saying that we have 2 comparators: Number Comparator and Date Comparator.
The former is used by Number Helper while the latter is used by the class itself.

Both have the same functionality:
Comparator.LT == DateHelper.TimeComparator.BEFORE
Comparator.LTE == DateHelper.TimeComparator.BEFORE_OR_EQUAL_TO
Comparator.EQ == DateHelper.TimeComparator.EQUAL_TO
Comparator.GT == DateHelper.TimeComparator.AFTER
Comparator.GTE == DateHelper.TimeComparator.AFTER_OR_EQUAL_TO

We can merge the two classes of course, but imo is(date1, BEFORE, date2) is easier to read and understand contextually than is(date1, LESS_THAN, date2) or is(date1, LT, date2).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants