savvato-software / tribe-app-backend

3 stars 20 forks source link

CosignServiceImplTest.java listBuilderCosignForUser() method needs cleanup #193

Closed haxwell closed 4 months ago

haxwell commented 6 months ago

CosignServiceImplTest.java, the method listBuilderCosignForUser()..

  1. Long userIdReceiving = userPerPhraseCount + 1L; -- Why is the userIdReceiving being based on a count? Just set it to something, don't base it on this variable. Someone coming behind (like me) looking at this won't understand why it's being done.

  2. for(int j=1; j<=userPerPhraseCount; j++)
    cosign.setUserIdIssuing((long)j);
    cosign.setUserIdReceiving(userIdReceiving);
    • Why are we repeatedly setting UserIdIssuing, but userIdReceiving we set just once? Both should be set just once.
mrsbluerose commented 6 months ago

@haxwell The helper method I built is a little unclear and admittedly outdated since we are trying to use testing constants now. Here's what I did:

I could take this ticket and either get rid of the helper method and write out the longer code, or I could rework the helper method with constants and make comments to better explain the logic. What are your thoughts?

haxwell commented 6 months ago

Yes, I remember that.. Okay, I think reworking the helper method will be enough.

mrsbluerose commented 5 months ago

I refactored the test in pull request #209 , along with the other refactoring of tests to use AbstractTestConstants.