trello ticket 13 - candidate-sample-payment-addition #32

ers020 · 2022-08-08T23:15:54Z

Notes on Changes:

pom.xml - I had been talking with Justin a while back, and he had mentioned the plugin pittest, so having tried it on my own, I decided to add it to this project to show the coverage of the Controller and Service I added (screen shots attached).

PaymentController - I added the requested GET method, and added a POST that consumes a Filter object. I figured it would be good for future code expansion possibilities. I understand this wasn't a requirement of the task, but wanted to expand it (screen shots for both attached).

ResourceMapper - I changed 1 line of code/logic, because when I went to test my initial code, I noticed it was returning the last 5 digits of the card number, instead of the last 4.

PageUtil - Made an overloaded method (no args) for the PageUtil to cut down on passing null values (more geared toward unit test coding).

data.sql - Added 6 entries for the payment table (attempted to mock the style of data being used in the program) and added 2 payment methods to the same User Id (grouped together at the top of the inserts) to show more than 1 return can happen.

… entries to show I added the same userId twice to return multiple values to show filter is working correctly.

ers020 · 2022-08-08T23:19:05Z

PaymentDto - Added the userId variable to show that the correct User information was being sent back in the response. Also included the single constructor arg to easily create the test object instead of having repeated code in multiple files.

tlynema-bravolt · 2022-08-09T17:43:14Z

src/main/java/com/bravo/user/model/dto/PaymentDto.java

+  public PaymentDto(final String id) {
+    this();
+    this.id = id;
+  }


Are there any other properties in here that should be required or added to the constructor. Setting final here when there is a setter from @DaTa annotation seems to only protect the value in certain situations. Is there a path to full immutability?

The constructor was mainly created to simplify code used in other places as a code cleanup effort; cleaner than using no args and setter. As far as making it more immutable, I can look into that.

I am pretty new with lombok (only heard about it a week ago), so I did some digging on the @builder. It looks like this would be a better alternative in the code in terms of creating objects with specific values, and won't clutter up the class I am trying to instantiate with values.

Though I did read a little more and saw @value, which does the same thing as @DaTa, but it makes the Class or Method fully immutable. I don't think this would be a good thing to do for the types of classes I am using (granted, @builder can make a referenced version I could use, but there would be a lot of recoding I would need to do at the moment), but the idea behind this is interesting, and something I wouldn't mind trying out in my free time.

If this isn't what you were looking for, let me know and I will do more research. I will also read up more on lombok and try some examples in my free time to see if it would be possible to update it to @value and be functional.

Exactly :) . @value or an Immutable @builder would be my preference here. If there are multiple levels to the object, builders can have objects that are made with builders as subclasses (though that can get ugly fast).

tlynema-bravolt · 2022-08-09T17:44:33Z

src/main/java/com/bravo/user/model/filter/PaymentFilter.java

+@AllArgsConstructor
+@NoArgsConstructor


With this being new, is there a reason for it having a builder a constructor and setters? Having three ways to set values seems overkill.

AllArgsConstructor was needed in order to create the object in the retrievePaymentByUserId on line 39 in the PaymentService class.

NoArgsConstructor - needed to stop an issue with serialization of the filter request object, it would return a Bad Request due to the application not being able to deserialize the object. Error from terminal: (although at least one Creator exists): cannot deserialize from Object value (no delegate- or property-based Creator).

I could get rid of the AllArgsConstructor by initializing an instance of the
object and then just using a setter, but the code does look cleaner this way.

Code generators like Lombok are a toss up. In this case, it's autogen needs to match the serialization. My preference has evolved to using immutable builders in this situation and configuring the serialization to use builders. The main idea here is to pick one way to set the values and stick to it. Someone copied a few pages from effective java here https://cs108.epfl.ch/archive/19/c/i/EffectiveJava_Item17.pdf. I highly recommend the book. It helped my view of Java coding immensely. The goal should be immutable value objects.

Thank you for the share, I will read this and take a look at getting the book (always room for learning more).

tlynema-bravolt · 2022-08-09T17:46:56Z

src/main/java/com/bravo/user/service/PaymentService.java

+            final PageRequest pageRequest,
+            final HttpServletResponse httpResponse
+    ){
+        LOGGER.info("Request to retrieve payment information being conducted... paymentFilter: {}", filter);


Good use of string formatting for the logger. This log message doesn't seem INFO to me. There are some reasons to log like this. Not sure they apply here. Would like to know what your thinking was.

The logger is kind of a habit I have from when I worked at Verizon (their in home standard). It was required in the service area anytime something came in or went out, in the hopes of trouble shooting problems that may arise either from front end or the database. I could change it to Trace or Debug though.

tlynema-bravolt · 2022-08-09T17:51:18Z

src/main/java/com/bravo/user/utility/PageUtil.java

+  public static PageRequest createPageRequest() {
+    return createPageRequest(null, null);
+  }


This looks like it is only used for test code. In general, having nulls can create additional checks that need to happen in code. Since this is only in test, could the other initializer be used with hardcoded values?

This was more of a code cleanup effort from having null values in tests to acquire default values from the PageUtil (page 1, size 20). Just a personal choice, but the original call could still be used, and this removed.

tlynema-bravolt · 2022-08-09T17:52:39Z

src/test/java/com/bravo/user/controller/PaymentControllerTest.java

+    }
+
+    @Test
+    public void getRetrieveWithUserId() throws Exception {


Is the throws needed here?

Yes, it is to cover the .perform, and result.andExpect calls as both throw exceptions. I could wrap these into a try/catch block, but this makes the code cleaner.

tlynema-bravolt · 2022-08-09T17:53:20Z

src/test/java/com/bravo/user/controller/PaymentControllerTest.java

+    private static final ObjectMapper MAPPER = new ObjectMapper();
+
+    @Autowired
+    private MockMvc mockMvc;


Happy to see mockmvc. That is often missed.

Thank you, it's been a little while since I have done JUnits and Mockito, so I am trying to do more learning and example coding in my free-time.

tlynema-bravolt · 2022-08-09T17:55:00Z

src/test/java/com/bravo/user/controller/PaymentControllerTest.java

+    public void postRetrieveWithFilter() throws Exception {
+        when(paymentService.retrieve(any(PaymentFilter.class), any(PageRequest.class), any(HttpServletResponse.class)))
+                .thenReturn(payments);


Which layers are being tested with this test?

Only the flow of the Controller, as the mock is designed to merely mock the results of calling the Service class.

tlynema-bravolt · 2022-08-09T17:56:13Z

src/test/java/com/bravo/user/service/PaymentServiceTest.java

+        final List<Integer> ids = IntStream
+                .range(1,6)
+                .boxed()
+                .collect(Collectors.toList());


Is this better than a simple List.of ?

I keep forgetting List.of is an item of Java9 and above. I am used to mainly working in Java8. I will look into it and see which performs better.

It looks like in this scenario, List.of(), would be better; given that the amount of data is smaller and has no reason to be changed (since it is immutable).

tlynema-bravolt · 2022-08-09T18:02:24Z

src/test/java/com/bravo/user/service/PaymentServiceTest.java

+    private Payment createPayment(String paymentId)
+    {
+        final Payment payment = new Payment();
+        payment.setId(paymentId);
+        return payment;
+    }


In my view, this is too much work to make a payment object. The constructor or builder can be better alternatives.

Agreed. This is an oversight on my part. I will look into improving this.

The @builder seems to be a good option for here, rather than what I did before with creating a single arg constructor, it's less messy than potentially having to add more constructors if I need to just set specific values.

ers020 · 2022-08-10T15:34:54Z

I will do the updates I have mentioned in my responses and push the code up by tomorrow. Thank you for the feedback so far.

ers020 · 2022-08-10T21:00:25Z

I pushed up fixes to use .builder() for some of the items, List.of() for another, and changing the the logger from INFO to DEBUG. I am going to work on immutable items for the PaymentFilter and see what I can do with the Payment DAO object.

… still be usable.

ers020 · 2022-08-10T21:27:58Z

I was able to remove the Constructor I added to the PaymentDTO, make the PaymentFilter more immutable and took care of the serialization issue by using the @Jacksonized annotation (which provides an auto-generated deserizable class). Will focus on Payment Dao object now.

… but tests correctly and runs live correctly as well.

ers020 · 2022-08-10T21:53:00Z

Was able to get rid of the 1 arg constructor I made to make a Payment Dao that I needed by adding @builder. Ran into an issue with the mapperFacade, so I added the @AllArgsConstructor so reflection would work properly (converting Dao to Dto object). I will look a bit more before the night is over to see what I can come up with.

ers020 added 4 commits August 6, 2022 19:38

Addition of the Payment Get functionality and Junit tests

5a94198

Junit Tests

43db612

Fixes to the application

1269862

Removed comments, will add them to PR request, and fixed data.sql new…

212c5f1

… entries to show I added the same userId twice to return multiple values to show filter is working correctly.

tlynema-bravolt reviewed Aug 9, 2022

View reviewed changes

ers020 added 2 commits August 10, 2022 15:49

Minor improvements/changes as pointed out to me by the code reviewer

e1e8463

changing the info loggers to debug

e55716f

made the PaymentFilter more immutable with @value and @Jacksonized to…

535cd72

… still be usable.

Able to remove the constructor I used before, had to add annotations,…

1e12677

… but tests correctly and runs live correctly as well.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trello ticket 13 - candidate-sample-payment-addition #32

trello ticket 13 - candidate-sample-payment-addition #32

ers020 commented Aug 8, 2022

ers020 commented Aug 8, 2022

tlynema-bravolt Aug 9, 2022

ers020 Aug 9, 2022

ers020 Aug 10, 2022

tlynema-bravolt Aug 10, 2022

tlynema-bravolt Aug 9, 2022

ers020 Aug 9, 2022

tlynema-bravolt Aug 10, 2022

ers020 Aug 10, 2022

tlynema-bravolt Aug 9, 2022

ers020 Aug 9, 2022

tlynema-bravolt Aug 9, 2022

ers020 Aug 9, 2022

tlynema-bravolt Aug 9, 2022

ers020 Aug 9, 2022 •

edited

Loading

tlynema-bravolt Aug 9, 2022

ers020 Aug 9, 2022

tlynema-bravolt Aug 9, 2022

ers020 Aug 9, 2022

tlynema-bravolt Aug 9, 2022

ers020 Aug 9, 2022

ers020 Aug 10, 2022

tlynema-bravolt Aug 9, 2022

ers020 Aug 9, 2022

ers020 Aug 10, 2022

ers020 commented Aug 10, 2022

ers020 commented Aug 10, 2022

ers020 commented Aug 10, 2022

ers020 commented Aug 10, 2022

		@AllArgsConstructor
		@NoArgsConstructor

trello ticket 13 - candidate-sample-payment-addition #32

Are you sure you want to change the base?

trello ticket 13 - candidate-sample-payment-addition #32

Conversation

ers020 commented Aug 8, 2022

ers020 commented Aug 8, 2022

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

ers020 Aug 9, 2022 • 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

ers020 commented Aug 10, 2022

ers020 commented Aug 10, 2022

ers020 commented Aug 10, 2022

ers020 commented Aug 10, 2022

ers020 Aug 9, 2022 •

edited

Loading