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

Dustin Vidrine Senior Java Candidate Assessment (card 0013) #35

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Dstnvidrn
Copy link

No description provided.

Dstnvidrn and others added 4 commits November 15, 2022 13:04
Add Payment Endpoint (controller, service, and repository)
Created /payments/retrieve/{userId}. endpoint
-created paymentRepository
-created payService
-modified SQL file
@Dstnvidrn
Copy link
Author

I put the screen shots under ScreenShots folder at the root of the project

@Dstnvidrn
Copy link
Author

Screen Shot 2022-11-15 at 3 04 09 PM

@Dstnvidrn
Copy link
Author

Screen Shot 2022-11-15 at 2 58 32 PM

@Dstnvidrn
Copy link
Author

Screen Shot 2022-11-15 at 2 20 29 PM

@@ -3,10 +3,15 @@
import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;

import java.util.UUID;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import and whitespace.

public List<Payment> getUserPayments(@PathVariable String userId) {
userValidator.validateId(userId);
return paymentService.getUserPayments(userId);
}
Copy link
Member

@wheeleruniverse wheeleruniverse Nov 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Weird whitespace. Too much whitespace on the bottom.
  2. GetMapping should not end with a /
  3. Should not return DAO model. Should use a DTO model instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I've seen this DAO and DTO concept before but I don't quite understand why its used.
I'll need to do some research to see the advantage of this.
Thanks, Justin.
I really value this feedback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DAO is Data Access Object and mirrors your database exactly. The DTO is Data Transfer Object and is sent to the clients. The DAO may have fields or logic in your database that you don't want to send to a client. You can do a conversion between the 2 types to make your API more extensible and adjustable. If your API client is relying on your database matching their contract then you will put yourself in a bad situation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes perfect sense. Thanks for that , Justin.

Overall what would you say of the over all assessment. I see I have some rookie mistakes and slightly "sloppy" code in some places, but that can easily improved on.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Kruzik762 sorry for the delay in responding. Bravo tries to place people where it can when it can. It's not a pass/fail, it's more of a can we place you at x client and have their needs met? And of course do we want to work with this person :) . Overall, it's really a question for the recruitment team.

@@ -15,6 +13,7 @@ public class Payment {

@Id
@Column(name = "id")
@GeneratedValue(strategy = GenerationType.IDENTITY) // Auto increments ID upon creation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The id is a string so how would the auto increment work?
  2. The id is being set explicitly in the constructor. Does this @GeneratedValue overwrite that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right.
Being that ID is a string, this isn't needed. I think I added it out of habit.

@@ -37,4 +36,52 @@ public Payment(){
this.id = UUID.randomUUID().toString();
this.updated = LocalDateTime.now();
}

public String getId() {
Copy link
Member

@wheeleruniverse wheeleruniverse Nov 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getters and Setters are provided by Lombok @Data annotation already on this model.
https://projectlombok.org/features/Data

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Okay. I haven't used Lombok before and wasn't sure how that worked. I added Getters/setters incase.

import lombok.Data;

@Entity
@Data
Copy link
Member

@wheeleruniverse wheeleruniverse Nov 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getters and Setters are provided by Lombok @Data annotation. Why did you remove it? https://projectlombok.org/features/Data

import lombok.Data;

@Entity
@Data
Copy link
Member

@wheeleruniverse wheeleruniverse Nov 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getters and Setters are provided by Lombok @Data annotation. Why did you remove it? https://projectlombok.org/features/Data

private PaymentRepository paymentRepository;

@Autowired
private UserService userService;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

userService is unused here.

@Repository
public interface PaymentRepository extends JpaRepository<Payment,String> {

// Finds All Payments linked to the User ID provided
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this comment adds value?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really a lot. More so to briefly describe what the method will do.
I could have gone into more detail of how it works based on the method signature and name, being that the way this works is somewhat abstract.


LOGGER.info("found user '{}'", id);
System.out.println(user.toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't commit System.out or System.err. Use a Logger instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Noted

@@ -9,7 +9,7 @@ spring:
password: ${DB_PASSWORD:password}
username: ${DB_USERNAME:sa}
url: ${DB_URL:jdbc:h2:mem:testdb}
h2.console.enabled: false
h2.console.enabled: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The console should only be enabled locally.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. I enabled to see the database tables and forgot to disable it.
Noted


@Test
void getUserPayments() {
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty test.

@@ -25,14 +26,15 @@ create table payment (
);

create table profile (
id varchar(60) primary key,
profile_id varchar(60) primary key,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Profile id was changed from id and the others were not. Naming is an interesting beast. In this case, with the others having only id as the field, changing only this one may cause confusion. https://thecoderoad.blog/2020/03/29/clean-code-naming-conventions/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I should have changed to other to match (userId, addressId, etc) to follow convention.

Thanks for the link. I'll read up on it

Comment on lines 87 to +90
('dbbfcff8-bdc3-41ff-9589-e4a2d0899a49', 'Edwin', null, 'Foster', '3354354465'),
('dc0b6cb6-633d-41ae-a2a4-4a87d20474a1', 'Reid', 'Craig', 'Stevens', '3562052323'),
('dc59ce6d-9f94-4e9a-a74c-524754a8c14e', 'Honey', null, 'Hall', '5625725736'),
('dd164d46-e41c-4ee6-8efb-6261709e7df4', 'John', null, 'Rogers', '5474116241'),
('dd9864a2-964c-4392-a911-9bc112e44336', 'William', null, 'Sullivan', '5150641251'),
('ddc164ca-d765-458f-8fd8-fcc0017e2b26', 'Kirsten', null, 'Cooper', '2770336024'),
('ddc2ae1e-e019-4569-b32e-cadee6a259d8', 'Luke', null, 'Evans', '2501553452'),
('ddcdac39-8b72-4253-a058-9493db239b6d', 'Adele', 'Montgomery', 'Tucker', '3365515354'),
('ddd6afa8-9864-4e57-a633-5c62950a0e11', 'Madaline', null, 'Elliott', '6462316347'),
('de26af1c-dacb-41c0-b8ab-c303a2ca93a8', 'Paige', 'Grant', 'Anderson', '2644315374'),
('de647c3c-ca05-4663-acf6-119876f45d3b', 'Annabella', 'Maddie', 'Crawford', '1510053531'),
('dea65993-e55e-4998-a3e2-554590be9363', 'Luke', null, 'Johnson', '2552342361'),
('ded62381-f1d2-44d2-85bb-98c64e2d70e1', 'Gianna', 'Fiona', 'Edwards', '3374116463'),
('deee9071-de71-4e35-9377-5f01f9ac03df', 'Roman', null, 'Elliott', '3341552626'),
('df8798fd-7e86-450a-b262-072ce701f85d', 'Paul', null, 'Warren', '1561612334'),
('dfc8df54-8f09-4e71-ade0-bc81b4717d25', 'Hailey', null, 'Perry', '3662044233'),
('e122b10e-786d-4f96-8600-d51d9fbc9d7f', 'Ashton', null, 'Dixon', '4052451171'),
('e1d51055-b695-48da-a401-926b887fa93f', 'Mary', 'Higgins', 'Ross', '1404365616'),
('e21b7b81-563b-4ce6-a42e-7ec5f688de0c', 'Charlotte', 'Abigail', 'Carter', '3750752435'),
('e243bd35-b892-4551-a924-597499631c9c', 'Annabella', 'Kate', 'Hamilton', '4676242460'),
('e32a9640-23c0-4ab1-a7c9-75dc49982a39', 'Brianna', 'Jared', 'Cooper', '2604241176'),
('e353c247-498d-4ee5-a626-465f79ca7593', 'Sarah', null, 'Douglas', '2546615422'),
('e39fc8d1-9591-472e-9d9f-0d2b44ed8303', 'Kevin', null, 'West', '6126166417'),
('e489c813-54b4-4a5f-8376-b268c8d589f4', 'Antony', null, 'Allen', '4132635373'),
('e4d2a62f-6d89-4a62-8bca-5c208ada9291', 'Victoria', 'Lloyd', 'Morrison', '3540273757'),
('e50b8bed-3dba-4a29-a66c-bbfb4331095d', 'Adelaide', 'Harris', 'Andrews', '6116654217'),
('e55cc624-f9ec-42e2-8141-444845226799', 'Arnold', 'Howard', 'Morrison', '5335210244'),
('e6590c86-1b8e-43f0-840f-5ea887e5f193', 'Naomi', null, 'Hall', '1152351465'),
('e66fec98-b0e7-4adf-ac0a-6c1d80bda163', 'Robert', 'Morgan', 'Higgins', '2207472261'),
('e68f6ddf-490a-4b32-9f92-66d692cbd056', 'Luke', 'John', 'Hunt', '3316256332'),
('e69ebd28-2f76-4204-9d45-cdd79b9e6876', 'Emma', null, 'Harper', '1406261675'),
('e6dfb176-76b5-4a43-97c3-3858366a4ab2', 'Charlotte', null, 'Owens', '6725336511'),
('e77a5d44-6ad9-4995-ba37-62ac199213f3', 'Michelle', null, 'Perry', '6522536435'),
('e7c83435-574a-4755-aebf-c38a8a5f13c1', 'Stella', 'Harold', 'Crawford', '4552544571'),
('e7ef12d6-b157-4356-858c-4ea7bbf10d6d', 'Dominik', 'Andrews', 'Robinson', '7141154142'),
('e81a764f-0fd9-4dac-a9d5-be9d0092bc34', 'Derek', null, 'Rogers', '4614110664'),
('e8913a42-fdd7-4598-9d0f-d585b5b6f0b3', 'Sarah', 'Kellan', 'Tucker', '2150724667'),
('e8b0bec3-3880-4f5e-bcbd-33a5f0ba4798', 'Violet', 'Brown', 'Morrison', '5471262332'),
('e92ce9d2-b0a0-4beb-83ee-63b4038fb70b', 'Adison', null, 'Robinson', '2114032416'),
('e9c6e61c-bd35-4a91-ac30-389f3f7c88ce', 'Frederick', 'James', 'Dixon', '4166117431'),
('ea41dabc-c47f-4d41-bd5f-244bbe843d14', 'Evelyn', null, 'Evans', '2304616011'),
('ea5421f2-02c3-425c-acce-da3ae3161e92', 'Penelope', null, 'Brooks', '2743571475'),
('ea82c0cb-2ecb-47fc-9acc-64e740b832f7', 'Ashton', 'Eleanor', 'Williams', '4562736434'),
('ebc324ba-176b-4595-89ba-346eaa0eca46', 'Kelsey', null, 'Phillips', '2721441705'),
('ec5c324f-7fca-4372-9ec8-b409b9565bee', 'Derek', null, 'Thomas', '2611164363'),
('ec8b29df-21ac-4034-8dbe-9ec595859866', 'Kate', 'Aida', 'Kelly', '4077725036'),
('ecb92ad6-9c03-409e-8f19-164786368010', 'Annabella', 'Johnson', 'Martin', '5242507152'),
('ed387742-378e-4cee-a0b1-0b490ada583c', 'Paul', null, 'Evans', '4264653314'),
('ed5f511d-b9f3-44bc-bbb3-6855ec1c2970', 'George', 'Edward', 'Johnson', '4712624304'),
('ed8fc6eb-b1d7-4827-9bbd-6c68b2a78808', 'Isabella', null, 'Harper', '4252373645'),
('edb40e70-ed73-47bb-9933-ad4f925eb214', 'Edith', 'Lucy', 'Watson', '5266134012'),
('eee5228e-48da-4a69-8d4f-7cda70774057', 'Melanie', 'Jacob', 'Edwards', '5046167444'),
('ef04a930-cae1-4235-aa2b-f45e2c2e5df3', 'Chloe', 'Douglas', 'Henderson', '5632012347'),
('efc0d7a9-334f-4925-b366-46def741560b', 'Emily', 'Brooks', 'Andrews', '1517614262'),
('efd74d2a-3d56-44f7-8b75-08bcc6096c64', 'Lucy', 'Eddy', 'Henderson', '4526573013'),
('f0118893-1883-4698-b184-a0ec06534bfd', 'Sofia', null, 'Johnston', '4231675452'),
('f06300c6-b366-4bb5-b6fc-8dfd00ae6672', 'Alfred', null, 'Perkins', '3512522142'),
('f06b6168-5cc2-4302-a50c-3a61334767d2', 'Vanessa', 'Max', 'Edwards', '3173425362'),
('f0965938-e910-4b30-9181-24c813a26e4d', 'Frederick', null, 'Stewart', '6471173475'),
('f1468399-6d75-47fc-ad14-e931c044f664', 'Sawyer', null, 'Sullivan', '5031132322'),
('f1491491-24d6-4395-9376-69e4476bdf9d', 'James', 'Casey', 'Dixon', '5761652604'),
('f1757f2b-1ba1-448d-8b96-56ef4dff627e', 'Ted', null, 'Harrison', '7637557563'),
('f1865e34-08ef-40c0-b805-97bad85385af', 'Melanie', 'Perry', 'Watson', '5136533166'),
('f245644b-a551-4b64-aa1f-a1111412c517', 'Jacob', 'Gibson', 'Brooks', '2334011104'),
('f329392a-21d5-4581-8ac1-70548d96979b', 'Sophia', 'Hamilton', 'Taylor', '5450731655'),
('f334199f-5f22-4ce5-b980-d2df4a1085c0', 'Kellan', 'Preston', 'Andrews', '2461744165'),
('f375aa7f-e081-49d0-870f-eebcd2b9d399', 'Walter', 'Martin', 'Murray', '6160371033'),
('f3c5dc29-1089-4887-b73c-0ef0c057f24f', 'Derek', null, 'Scott', '4714751567'),
('f46900f0-8331-4653-a209-9618d68f059c', 'Garry', 'Arthur', 'Mason', '5560531623'),
('f46ee9b9-2b26-45b6-9494-dfeb21d7324f', 'Jared', null, 'Andrews', '1736221512'),
('f479cdaf-2163-4ef3-aa01-e5235f16a462', 'Deanna', null, 'Brown', '2644013341'),
('f4823101-8425-4b5d-9024-575ea4e8e703', 'Abraham', null, 'Higgins', '5732643020'),
('f4cc8426-2ed3-4c16-98ce-96605f5324eb', 'Frederick', null, 'Davis', '3344434621'),
('f5164fd3-b259-4d36-a7cb-5d37d5d3ab07', 'Kellan', null, 'Casey', '5044561015'),
('f52f40de-6cc3-45d9-b4dd-8d743391ba8e', 'Frederick', null, 'Kelley', '7441421171'),
('f579b2a5-6d58-4a4d-b536-5e8f835e6fc7', 'Sofia', null, 'Farrell', '5642647625'),
('f62bcea8-61c1-4f8d-ab9e-e5ac28a65013', 'Chelsea', null, 'Foster', '6221501645'),
('f69538af-5a9b-4130-b708-667cd588035b', 'Alberta', null, 'Howard', '5524655527'),
('f6cd5842-da01-4e59-b676-5ebc7e4a2693', 'Lydia', 'Rebecca', 'Harris', '5134251606'),
('f7025046-5199-45f6-97bb-92a48ec0eb82', 'James', 'Reed', 'Morrison', '3775512223'),
('f7f4a515-7f64-4718-8bb1-0e2f7b04465b', 'Fenton', 'Ashton', 'Wright', '1324044616'),
('f8160123-1766-41c7-8056-2acbfd3c4f2a', 'Victor', null, 'Taylor', '3426104323'),
('f8730cc6-a89b-40ca-94b2-ff4f7b3e0044', 'Catherine', null, 'Smith', '7653356573'),
('f8bbbc7a-867b-4e8d-a2a5-8b7c07a4589c', 'Roland', null, 'Ross', '4655666462'),
('f8f76b77-7007-47a5-9c50-5dfc26f59eb5', 'Roland', 'Alen', 'Murray', '5245223310'),
('f9096b14-daab-430f-835b-2f0d651e3559', 'Arianna', 'Belinda', 'West', '4323425442'),
('f9190ad7-930a-47d3-a6ce-84376082ff0b', 'Sam', null, 'Thompson', '1431023164'),
('f97177e1-5fe1-4327-9978-9e9d48b23338', 'Carlos', null, 'Allen', '3111463363'),
('fa98771c-4b54-4552-9c51-2d26ec988fa8', 'Alen', null, 'Williams', '5356263502'),
('fae16dca-950f-441d-bc35-415d92f2e939', 'April', null, 'Turner', '1134214353'),
('fb4d9aba-3c4b-45f2-8f94-8ad144c7dff7', 'Frederick', null, 'Fowler', '2273766242'),
('fb89e37f-96b8-4d9a-b4e1-b51c592a6c1c', 'Hailey', null, 'Edwards', '2652441101'),
('fc0e0a12-6679-4b44-8586-9d8eee063629', 'Alexia', 'Cadie', 'Carter', '3152614653'),
('fcee9b60-6a3f-4fae-b7b3-27bc70c68b6f', 'Daisy', 'Adele', 'Williams', '2264357140'),
('fd10b53c-a428-49a1-9628-f96f5dcaaed6', 'Carl', null, 'Thomas', '5455464561'),
('fd152dd9-6fae-4cb2-a041-02162f200678', 'Brooke', 'Aiden', 'Watson', '2535073126'),
('fd6d21f6-f1c2-473d-8ed7-f3f9c7550cc9', 'Elian', 'Cherry', 'Gibson', '3226211765'),
('fd9e22ef-20f1-4e1b-9f9e-3139fd13fa85', 'Ashton', 'Carina', 'Foster', '4653236364');
('0fa26435-0d04-4afe-b7aa-9a86d71b33a4', 'Clark', null, 'West', '5221356551');
--('107f2920-9b79-4112-802c-47bc145acd56', 'Carlos', 'Lyndon', 'Perkins', '1136532503'),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like that you scrapped some of these as they didn't add a lot of value for the tests. Commenting them is confuses me, is there a reason for the comment over the delete?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted because I felt that having so many wasn't all that helpful.
I could of commented some out as well, but it still makes the file and navigating the file a bit cumbersome.

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

Successfully merging this pull request may close these issues.

3 participants