Trunk 5452 - Adaption of equals for CohortMembership #2791

fruether · 2018-12-18T15:46:54Z

Description of what I changed

In this function I added the equal method to CohortMembership accordingly to the ticket description (https://issues.openmrs.org/browse/TRUNK-5452). In addition, I added several test cases to CohortMembershipTest to prove that the method behaves in the intended way. A related talk topic can be found here: Talk Thread

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-5452

Checklist: I completed these to help reviewers :)

My pull request only contains ONE single commit
(the number above, next to the 'Commits' tab is 1).

No? -> read here on how to squash multiple commits into one
[x ] My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
[ x] I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
[ x] I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
[ x] All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
[x ] My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

coveralls · 2018-12-18T16:06:43Z

Coverage increased (+0.02%) to 59.368% when pulling cc7e9d6 on fruether:TRUNK-5452 into eb6c31c on openmrs:master.

dkayiwa · 2018-12-18T20:42:43Z

Do you mind making the commit message better than simply "resolving ticket id"? In other words, from the commit message, i should tell what the commit is about, without having to first look at the ticket.

fruether · 2018-12-18T20:53:20Z

@dkayiwa I adapted the commit message. In the past I always did it like this. THat's why I did not do it in the first place.

dkayiwa · 2018-12-18T21:53:39Z

Can you have the commit message include the ticket id as advised at? https://wiki.openmrs.org/display/docs/Pull+Request+Tips

fruether · 2018-12-18T22:06:05Z

@dkayiwa added message and issue number

dkayiwa · 2018-12-19T19:39:21Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+	public void equal_shouldReturnTrueIfObjectReferenceAreTheSame() {
+		CohortMembership cohortMembership = new CohortMembership(12);
+
+		boolean result = cohortMembership.equals(cohortMembership);


I guess we could just assert without the temporary variable.

I thought it is more OpenMRS agree on style to first execute the function under test and then perform the assertion? Anyways I changed it

dkayiwa · 2018-12-19T19:39:51Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+		// A null argument to equal should not lead to an exception
+		CohortMembership cohortMembership = new CohortMembership(12);
+
+		boolean result = cohortMembership.equals(null);


I guess we could just assert without the temporary result variable.

dkayiwa · 2018-12-19T23:10:13Z

api/src/main/java/org/openmrs/CohortMembership.java

+
+	/**
+	 * Indicates if a given cohortMembership object is equal to this one
+	 * 


Did you just forget to add the @SInCE annotation?

Yes. I just added it

dkayiwa · 2018-12-19T23:25:02Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+		Date startDate1 = new Date(1545140935);
+		Date endDate1 = new Date(1577836800);
+
+		Date startDate2 = new Date(1545140935);


I guess we can avoid duplicating the date value.

Are you sure? Because the issue is that then we may do not see if equal() of Date works as aspected

Anyways I adapted it

i mean duplicating 1545140935

dkayiwa · 2018-12-19T23:29:17Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+		cohortMembershipTwo.setStartDate(startDate2);
+		cohortMembershipTwo.setEndDate(endDate2);
+
+		boolean result = cohortMembershipOne.equals(cohortMembershipTwo);


I guess we could just directly assert

I did so in the new commit

dkayiwa · 2018-12-19T23:30:47Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+		cohortMembershipTwo.setStartDate(startDate2);
+		cohortMembershipTwo.setEndDate(endDate2);
+
+		boolean result = cohortMembershipOne.equals(cohortMembershipTwo);


I guess we could just directly assert

I did so in the new commit

dkayiwa · 2018-12-20T11:17:22Z

api/src/main/java/org/openmrs/CohortMembership.java

+	public boolean equals(CohortMembership otherCohortMembership) {
+		if(this == otherCohortMembership) return true;
+
+		return otherCohortMembership != null && this.endDate.equals(otherCohortMembership.getEndDate()) && this.startDate.equals(otherCohortMembership.getStartDate()) 


What happens if any of these properties is null?

You are right! I did not took this into account. I changed the code and added two new tests. Are two tests alright or should I add further test cases dealing with null values?

Those tests would not hurt. 😊

All possible cases should be tested now!

dkayiwa · 2018-12-20T12:32:59Z

api/src/main/java/org/openmrs/CohortMembership.java

+		if(this == otherCohortMembership) return true;
+
+		return otherCohortMembership != null && 
+			((endDate != null ) ? this.endDate.equals(otherCohortMembership.getEndDate()) : otherCohortMembership.getEndDate() == null)


Any reason why you sometimes use endDate and others you use this.endDate?
At first appearance, it looked as if endDate is a local variable different from this.endDate

"this." is not necessary since only the class attributes exist with this name (no local variable).
At first I thought it would make it more obvious that "this" class member is checked for equality with the attribute. But since the statement is already quite long to avoid NullPointerExceptions I think the better way to go is to remove the "this" in this method.

This was done in the new commit

dkayiwa · 2018-12-20T14:57:17Z

@fruether is this of any help? http://www.javapractices.com/topic/TopicAction.do?Id=17

fruether · 2018-12-20T23:21:17Z

@dkayiwa

@fruether is this of any help? http://www.javapractices.com/topic/TopicAction.do?Id=17

I am not sure if I got correctly what the intension of this link is.
Do you want to point out that hashCode() has to be implemented as well? If so I did not do this so far since the issue just said equal and I am not sure what the best way would be to implement hashCode so that is will work

Regarding the comparrission guideline: "possibly-null object fields : use both == and equals" since -> I used equals() but not explicitly ==. But this should not be a problem since equals() should check if both references are equal and the null object case is also taken care of

dkayiwa · 2018-12-21T00:10:13Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+	}
+
+	@Test
+	public void equal_shouldReturnTrueIfBothPatientIdIsNull() {


Should return true or false?

dkayiwa · 2018-12-21T00:15:18Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+	}
+
+	@Test
+	public void equal_shouldReturnFalseIfEndDateIsNull() {


The method name seems to suggest that this would return true if the end date was not null

dkayiwa · 2018-12-21T00:16:46Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+	}
+
+	@Test
+	public void equal_shouldReturnFalseIfStartDateIsNull() {


The method name seems to suggest that this would return true if start date was not null

dkayiwa · 2018-12-22T18:24:12Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+	}
+
+	@Test
+	public void equal_shouldReturnTrueIfObjectsAreTheSameIncludingNullValues() {


What does "including null values" mean?

The thing is I want to distinguish the equal tests that contain Null values in their objects and that have all attributes set properly.

That is why I wanted to add a phrase to point out that the test actually tests if equal works when some attributes are null in both objects.

Which name do you think would make the most sense:

equal_shouldWorkWhenNullValuesExistInObject()

equal_shouldReturnTrueWhenBothPatientIdsAreNull()

The second one is more precise.

dkayiwa · 2018-12-23T11:38:16Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+	}
+
+	@Test
+	public void equal_shouldReturnTrueWhenBothPatientIdsAreNull() {


Does this mean it will return true if both patient ids are null even if the other fields are different?

@dkayiwa I corrected the name

dkayiwa · 2018-12-23T11:40:06Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+	}
+
+	@Test
+	public void equal_shouldReturnTrueIfAllDatesAreNull() {


Should return true if all dates are null even if the patients are different?

When you say "all dates" does it also include dateCreated, dateChanged and dateVoided?

@dkayiwa I corrected the name

dkayiwa · 2018-12-23T11:44:59Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+		CohortMembership cohortMembershipOne = new CohortMembership(13);
+		CohortMembership cohortMembershipTwo = new CohortMembership(12);
+
+		Date startDate1 = new Date(1577836801);


Should we duplicate 1577836801?

dkayiwa · 2018-12-23T11:46:59Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+	}
+
+	@Test
+	public void equal_shouldReturnFalseIfOnlyOneStartDateIsNull() {


Should return false when one start date is null, even if all the other fields are the same? Or did you want to mean something else?

equal_shouldReturnFalseIfOnlyStartDateOfOneObjectIsNull

dkayiwa · 2018-12-23T11:48:44Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+	}
+
+	@Test
+	public void equal_shouldReturnFalseIfOnlyOneEndDateIsNull() {


Should return false when end date is null, even if all the other fields are the same? Or did you want to mean something else?

Pointed out now that it is only False if end Date is null for one object:

equal_shouldReturnFalseIfOnlyEndDateOfOneObjectIsNull

dkayiwa · 2018-12-25T19:32:06Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+		CohortMembership cohortMembershipOne = new CohortMembership();
+		CohortMembership cohortMembershipTwo = new CohortMembership();
+
+		Date startDate1 = new Date(1545140935);


Do wee need to duplicate 1545140935?

I will make a variable out of it. And yeah I think it is helpful for testing purposes to duplicate it

how would duplicating it be helpful?

The alternative would be to say: startDate2 = startDate1 correct?
But if I am doing so the objects point to the same area and the reference is already equal. I want to test/make sure that the equal() function of the Date type is executed. Hence to verify, if both dates have the same content. That is why I need to duplicate the value to create a new object which is equal but not the same

Assigning that value to a constant always helps to avoid the duplication 😊

What do you mean? I can't follow your right now :(

If you wanted to change that value from 1545140935 to 1545140936, would also change it for the second date?

Yes. it should change the value of both startDate's. The one of object1 and object2. Because I want to test if it fails, when only the endDate is different in both objects

The fact that you have to change the same value in more than one place is a design smell.

I fixed it for patientId now as well. Now only values are hardcoded that are only used for the creation of one objects. Thanks for the clarification

dkayiwa · 2018-12-25T19:33:37Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+	}
+
+	@Test
+	public void equal_shouldReturnFalseIfOnlyEndDateOfOneObjectIsNull() {


end date of one object or two objects?

Secondly, would it return true of the patient ids were the same?

(1) Only EndDate of one object is null. That is why both objects are not equal since the EndDate is different
(2) I fixed it. It was actually a mistake of me

dkayiwa · 2018-12-25T19:36:05Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+	}
+
+	@Test
+	public void equal_shouldReturnFalseIfOnlyStartDateOfOneObjectIsNull() {


Only start date is null or even end date of one object?

dkayiwa · 2018-12-25T21:27:28Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+		CohortMembership cohortMembershipOne = new CohortMembership();
+		CohortMembership cohortMembershipTwo = new CohortMembership();
+
+		long timestampStart = 1545140935;


Did you get a chance to look at our naming convention for constants? https://wiki.openmrs.org/display/docs/Java+Conventions

Secondly, would you like this to be later on assigned to another value?

"Any constants should be written in uppercase letters." I made it upper case and new words seperated by _

dkayiwa · 2018-12-25T21:29:44Z

api/src/test/java/org/openmrs/CohortMembershipTest.java

+	public void equal_shouldReturnTrueIfPatientIdIsEqualAndStarDateAndEndDateAreBothNull() {
+
+		// Creating two logical identical CohortMemberships to test equal method
+		int patientId = 12;


Same questions as below

made it final

…mrs#2790) TRUNK-5452 - Adding equal method to CohoerMembership class (openmrs#2791) TRUNK-5266: Remove unecessary keywords (openmrs#2795) TRUNK-5467 Use diamond operator where possible (openmrs#2799) TRUNK-5469 Use simpler call to get a String's substring (openmrs#2801) TRUNK-5470 Simplify detection of global error (openmrs#2802)

fruether force-pushed the TRUNK-5452 branch from 6508a21 to 9ef3870 Compare December 18, 2018 15:47

fruether force-pushed the TRUNK-5452 branch from 9ef3870 to 6e1c0e9 Compare December 18, 2018 20:50

fruether force-pushed the TRUNK-5452 branch from 6e1c0e9 to 4c34638 Compare December 18, 2018 22:03

dkayiwa reviewed Dec 19, 2018

View reviewed changes

fruether force-pushed the TRUNK-5452 branch from 4c34638 to 429862a Compare December 19, 2018 19:45

dkayiwa reviewed Dec 19, 2018

View reviewed changes

fruether force-pushed the TRUNK-5452 branch from 429862a to 9fef42f Compare December 19, 2018 23:14

dkayiwa reviewed Dec 19, 2018

View reviewed changes

fruether force-pushed the TRUNK-5452 branch 2 times, most recently from 859a803 to 9293850 Compare December 20, 2018 09:16

dkayiwa reviewed Dec 20, 2018

View reviewed changes

fruether force-pushed the TRUNK-5452 branch 3 times, most recently from b569d7c to 3464633 Compare December 20, 2018 11:46

dkayiwa reviewed Dec 20, 2018

View reviewed changes

fruether force-pushed the TRUNK-5452 branch from 3464633 to 4980318 Compare December 20, 2018 12:35

dkayiwa reviewed Dec 21, 2018

View reviewed changes

fruether force-pushed the TRUNK-5452 branch from 4980318 to 8123b23 Compare December 21, 2018 22:33

dkayiwa reviewed Dec 22, 2018

View reviewed changes

fruether force-pushed the TRUNK-5452 branch from 8123b23 to 0539dd6 Compare December 23, 2018 11:35

dkayiwa reviewed Dec 23, 2018

View reviewed changes

openmrs deleted a comment Dec 23, 2018

dkayiwa reviewed Dec 23, 2018

View reviewed changes

fruether force-pushed the TRUNK-5452 branch 3 times, most recently from 767dcfe to 5cab65c Compare December 25, 2018 18:36

openmrs deleted a comment Dec 25, 2018

dkayiwa reviewed Dec 25, 2018

View reviewed changes

fruether force-pushed the TRUNK-5452 branch from 5cab65c to 2f915be Compare December 25, 2018 20:40

openmrs deleted a comment Dec 25, 2018

fruether force-pushed the TRUNK-5452 branch 2 times, most recently from 5ab97f6 to 7937ef6 Compare December 25, 2018 21:07

openmrs deleted a comment Dec 25, 2018

fruether force-pushed the TRUNK-5452 branch from 7937ef6 to 1d31d04 Compare December 25, 2018 21:23

dkayiwa reviewed Dec 25, 2018

View reviewed changes

fruether force-pushed the TRUNK-5452 branch from 1d31d04 to 9b6ccb7 Compare December 25, 2018 21:33

openmrs deleted a comment Dec 25, 2018

TRUNK-5452 - Adding equal method to CohoerMembership class

cc7e9d6

fruether force-pushed the TRUNK-5452 branch from 9b6ccb7 to cc7e9d6 Compare December 25, 2018 21:41

openmrs deleted a comment Dec 25, 2018

dkayiwa merged commit 60d92bd into openmrs:master Dec 26, 2018

Trunk 5452 - Adaption of equals for CohortMembership #2791

Trunk 5452 - Adaption of equals for CohortMembership #2791

Conversation

fruether commented Dec 18, 2018

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

coveralls commented Dec 18, 2018 • edited Loading

dkayiwa commented Dec 18, 2018

fruether commented Dec 18, 2018

dkayiwa commented Dec 18, 2018

fruether commented Dec 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa commented Dec 20, 2018

fruether commented Dec 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fruether Dec 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 18, 2018 •

edited

Loading

fruether commented Dec 20, 2018 •

edited

Loading

dkayiwa Dec 21, 2018 •

edited

Loading

fruether Dec 22, 2018 •

edited

Loading