From e19abc6f5dbf790109f42ee2700f7867be11d619 Mon Sep 17 00:00:00 2001 From: noga Date: Sat, 17 Feb 2024 19:08:21 +0530 Subject: [PATCH 1/2] Updated with reviews and TODOs. --- .../assignment/constants/EmailConstants.java | 2 ++ .../assignment/constants/EndPoint.java | 1 + .../assignment/constants/ErrorCodeEnum.java | 4 ++-- .../assignment/controller/Controller.java | 14 ++++++------- .../assignment/entity/EmailDetails.java | 20 ++++++------------- .../assignment/entity/Employee.java | 2 +- .../exceptions/EmployeeException.java | 1 + .../assignment/pojo/EmployeeError.java | 2 ++ .../assignment/repository/EmployeeRepo.java | 4 ++++ .../assignment/response/EmailResponse.java | 2 ++ .../response/PostEmployeeResponse.java | 2 ++ .../assignment/service/EmployeeService.java | 15 ++++++++------ .../serviceImpl/EmailServiceImpl.java | 18 ++++++++--------- .../serviceImpl/EmployeeServiceImpl.java | 7 +++++-- 14 files changed, 53 insertions(+), 41 deletions(-) diff --git a/src/main/java/com/employee_wise/assignment/constants/EmailConstants.java b/src/main/java/com/employee_wise/assignment/constants/EmailConstants.java index 0b4b4c9..6b110f2 100644 --- a/src/main/java/com/employee_wise/assignment/constants/EmailConstants.java +++ b/src/main/java/com/employee_wise/assignment/constants/EmailConstants.java @@ -2,6 +2,8 @@ import com.employee_wise.assignment.entity.Employee; +// TODO this can be done via something called Server side templating engine +// public class EmailConstants { public static String getAdditionMessage(String manName, Employee emp) { diff --git a/src/main/java/com/employee_wise/assignment/constants/EndPoint.java b/src/main/java/com/employee_wise/assignment/constants/EndPoint.java index 118e1da..e3564c6 100644 --- a/src/main/java/com/employee_wise/assignment/constants/EndPoint.java +++ b/src/main/java/com/employee_wise/assignment/constants/EndPoint.java @@ -1,5 +1,6 @@ package com.employee_wise.assignment.constants; +// TODO this entire class is useless public class EndPoint { public static final String EMPLOYEE = "/employee"; diff --git a/src/main/java/com/employee_wise/assignment/constants/ErrorCodeEnum.java b/src/main/java/com/employee_wise/assignment/constants/ErrorCodeEnum.java index 5f44f45..0fecaa8 100644 --- a/src/main/java/com/employee_wise/assignment/constants/ErrorCodeEnum.java +++ b/src/main/java/com/employee_wise/assignment/constants/ErrorCodeEnum.java @@ -14,8 +14,8 @@ public enum ErrorCodeEnum { MAIL_NOT_SENT("10008","Failed Request, Mail could not be sent to the Reciepent"); - private String errorCode; - private String errorMessage; + private final String errorCode; + private final String errorMessage; private ErrorCodeEnum(String errorCode, String errorMessage) { this.errorCode=errorCode; diff --git a/src/main/java/com/employee_wise/assignment/controller/Controller.java b/src/main/java/com/employee_wise/assignment/controller/Controller.java index c8b3124..d9ebff0 100644 --- a/src/main/java/com/employee_wise/assignment/controller/Controller.java +++ b/src/main/java/com/employee_wise/assignment/controller/Controller.java @@ -37,14 +37,14 @@ public class Controller { public ResponseEntity addEmployee(@RequestBody Employee emp){ PostEmployeeResponse savedEmp = this.empSerImp.addEmployee(emp); - return new ResponseEntity(savedEmp,HttpStatus.CREATED); + return new ResponseEntity<>(savedEmp,HttpStatus.CREATED); } @GetMapping(EndPoint.GET_EMPLOYEE_BY_ID+ "/{id}") public ResponseEntity getEmployeeById(@PathVariable("id") String id){ Employee returnedEmp = this.empSerImp.getEmployeeById(id); - return new ResponseEntity(returnedEmp,HttpStatus.FOUND); + return new ResponseEntity<>(returnedEmp,HttpStatus.FOUND); } @GetMapping(EndPoint.GET_ALL_EMPLOYEE) @@ -55,13 +55,13 @@ public ResponseEntity> getAllEmployee( ){ List returnedEmp = this.empSerImp.getAllEmployee(pageNumber, pageSize, sortBy); - return new ResponseEntity>(returnedEmp,HttpStatus.FOUND); + return new ResponseEntity<>(returnedEmp,HttpStatus.FOUND); } @PutMapping(EndPoint.UPDATE_EMPLOYEE_BY_ID + "/{id}") public ResponseEntity updateEmployeeById(@PathVariable("id") String id, @RequestBody Employee emp){ PostEmployeeResponse updatedEmp = this.empSerImp.updateEmployeeById(id, emp); - return new ResponseEntity(updatedEmp,HttpStatus.OK); + return new ResponseEntity<>(updatedEmp,HttpStatus.OK); } @DeleteMapping(EndPoint.DELETE_EMPLOYEE_BY_ID+"/{id}") @@ -75,21 +75,21 @@ public ResponseEntity deleteEmployeeById(@PathVariable String id){ @GetMapping(EndPoint.GET_MANAGER_BY_NTH_LEVEL+ "/{id}" + "/{n}") public ResponseEntity getNthManager(@PathVariable("id") String id, @PathVariable("n") Integer n){ PostEmployeeResponse Emp = this.empSerImp.getNthManager(id, n); - return new ResponseEntity(Emp,HttpStatus.OK); + return new ResponseEntity<>(Emp,HttpStatus.OK); } @PostMapping(EndPoint.SEND_SIMPLE_MAIL) public ResponseEntity sendMail(@RequestBody EmailDetails details) { EmailResponse status = emailService.sendSimpleMail(details); - return new ResponseEntity(status,HttpStatus.OK); + return new ResponseEntity<>(status,HttpStatus.OK); } @PostMapping(EndPoint.SEND_MAIL_WITH_ATTACHMENT) public ResponseEntity sendMailWithAttachment(@RequestBody EmailDetails details) { EmailResponse status = emailService.sendMailWithAttachment(details); - return new ResponseEntity(status,HttpStatus.OK); + return new ResponseEntity<>(status,HttpStatus.OK); } } diff --git a/src/main/java/com/employee_wise/assignment/entity/EmailDetails.java b/src/main/java/com/employee_wise/assignment/entity/EmailDetails.java index 5d26939..a48213e 100644 --- a/src/main/java/com/employee_wise/assignment/entity/EmailDetails.java +++ b/src/main/java/com/employee_wise/assignment/entity/EmailDetails.java @@ -1,16 +1,8 @@ package com.employee_wise.assignment.entity; -import lombok.AllArgsConstructor; -import lombok.Data; -import lombok.NoArgsConstructor; - -@Data -@AllArgsConstructor -@NoArgsConstructor -public class EmailDetails { - - private String recipient; - private String msgBody; - private String subject; - private String attachment; -} +public record EmailDetails( + String recipient, + String msgBody, + String subject, + String attachment +){} diff --git a/src/main/java/com/employee_wise/assignment/entity/Employee.java b/src/main/java/com/employee_wise/assignment/entity/Employee.java index c33005c..ecc8183 100644 --- a/src/main/java/com/employee_wise/assignment/entity/Employee.java +++ b/src/main/java/com/employee_wise/assignment/entity/Employee.java @@ -12,7 +12,7 @@ import lombok.NoArgsConstructor; import lombok.Setter; - +// TODO migrate this to Record too - Java 17 Records @Data @NoArgsConstructor @AllArgsConstructor diff --git a/src/main/java/com/employee_wise/assignment/exceptions/EmployeeException.java b/src/main/java/com/employee_wise/assignment/exceptions/EmployeeException.java index aa300dd..8204ea0 100644 --- a/src/main/java/com/employee_wise/assignment/exceptions/EmployeeException.java +++ b/src/main/java/com/employee_wise/assignment/exceptions/EmployeeException.java @@ -6,6 +6,7 @@ import lombok.Getter; import lombok.Setter; +// TODO migrate to record @Getter @Setter @AllArgsConstructor diff --git a/src/main/java/com/employee_wise/assignment/pojo/EmployeeError.java b/src/main/java/com/employee_wise/assignment/pojo/EmployeeError.java index cb4a3f5..627d90e 100644 --- a/src/main/java/com/employee_wise/assignment/pojo/EmployeeError.java +++ b/src/main/java/com/employee_wise/assignment/pojo/EmployeeError.java @@ -5,6 +5,8 @@ import lombok.Data; import lombok.NoArgsConstructor; +// TODO migrate to record + @Data @Builder @AllArgsConstructor diff --git a/src/main/java/com/employee_wise/assignment/repository/EmployeeRepo.java b/src/main/java/com/employee_wise/assignment/repository/EmployeeRepo.java index 1613e72..d6645cd 100644 --- a/src/main/java/com/employee_wise/assignment/repository/EmployeeRepo.java +++ b/src/main/java/com/employee_wise/assignment/repository/EmployeeRepo.java @@ -7,6 +7,10 @@ import com.employee_wise.assignment.entity.Employee; +// TODO are we using MongoDB? If not, why we are using MongoRepo? +// import org.springframework.data.jpa.repository.JpaRepository; +// import org.springframework.stereotype.Repository; +// https://hevodata.com/learn/spring-boot-mysql/ @Repository public interface EmployeeRepo extends MongoRepository{ diff --git a/src/main/java/com/employee_wise/assignment/response/EmailResponse.java b/src/main/java/com/employee_wise/assignment/response/EmailResponse.java index e452b83..f39d61c 100644 --- a/src/main/java/com/employee_wise/assignment/response/EmailResponse.java +++ b/src/main/java/com/employee_wise/assignment/response/EmailResponse.java @@ -8,6 +8,8 @@ import lombok.NoArgsConstructor; import lombok.Setter; +// TODO migrate to Java 17 Record +// Also probably not needed @Data @Getter @Setter diff --git a/src/main/java/com/employee_wise/assignment/response/PostEmployeeResponse.java b/src/main/java/com/employee_wise/assignment/response/PostEmployeeResponse.java index 22ed690..c5e40a6 100644 --- a/src/main/java/com/employee_wise/assignment/response/PostEmployeeResponse.java +++ b/src/main/java/com/employee_wise/assignment/response/PostEmployeeResponse.java @@ -8,6 +8,8 @@ import lombok.NoArgsConstructor; import lombok.Setter; +// TODO migrate to Java 17 Record +// Not required, @Data @Getter @Setter diff --git a/src/main/java/com/employee_wise/assignment/service/EmployeeService.java b/src/main/java/com/employee_wise/assignment/service/EmployeeService.java index 6343217..3271eb4 100644 --- a/src/main/java/com/employee_wise/assignment/service/EmployeeService.java +++ b/src/main/java/com/employee_wise/assignment/service/EmployeeService.java @@ -10,10 +10,13 @@ @Service public interface EmployeeService { - public PostEmployeeResponse addEmployee(Employee emp); - public List getAllEmployee(Integer pageNumber, Integer pageSize, String sortBy); - public Employee getEmployeeById(String Id); - public PostEmployeeResponse updateEmployeeById(String id,Employee emp); - public Employee deleteEmployeeById(String id); - public PostEmployeeResponse getNthManager(String id, Integer n); + // TODO, refactor to return the new created employee id + PostEmployeeResponse addEmployee(Employee emp); + List getAllEmployee(Integer pageNumber, Integer pageSize, String sortBy); + Employee getEmployeeById(String Id); + PostEmployeeResponse updateEmployeeById(String id,Employee emp); + Employee deleteEmployeeById(String id); + + // TODO refactor to return the Employee, not this + PostEmployeeResponse getNthManager(String id, Integer n); } diff --git a/src/main/java/com/employee_wise/assignment/serviceImpl/EmailServiceImpl.java b/src/main/java/com/employee_wise/assignment/serviceImpl/EmailServiceImpl.java index 2ba9116..fc5da7c 100644 --- a/src/main/java/com/employee_wise/assignment/serviceImpl/EmailServiceImpl.java +++ b/src/main/java/com/employee_wise/assignment/serviceImpl/EmailServiceImpl.java @@ -34,12 +34,12 @@ public EmailResponse sendSimpleMail(EmailDetails details) SimpleMailMessage mailMessage = new SimpleMailMessage(); mailMessage.setFrom(sender); - mailMessage.setTo(details.getRecipient()); - mailMessage.setText(details.getMsgBody()); - mailMessage.setSubject(details.getSubject()); + mailMessage.setTo(details.recipient()); + mailMessage.setText(details.msgBody()); + mailMessage.setSubject(details.subject()); javaMailSender.send(mailMessage); - return new EmailResponse(details.getRecipient().toString(),"Email is Successfully sent to Recipent."); + return new EmailResponse(details.recipient(),"Email is Successfully sent to Recipent."); } catch (Exception e) { throw new EmployeeException(HttpStatus.INTERNAL_SERVER_ERROR, @@ -56,16 +56,16 @@ public EmailResponse sendMailWithAttachment(EmailDetails details) try { mimeMessageHelper = new MimeMessageHelper(mimeMessage, true); mimeMessageHelper.setFrom(sender); - mimeMessageHelper.setTo(details.getRecipient()); - mimeMessageHelper.setText(details.getMsgBody()); - mimeMessageHelper.setSubject(details.getSubject()); + mimeMessageHelper.setTo(details.recipient()); + mimeMessageHelper.setText(details.msgBody()); + mimeMessageHelper.setSubject(details.subject()); - FileSystemResource file = new FileSystemResource(new File(details.getAttachment())); + FileSystemResource file = new FileSystemResource(new File(details.attachment())); mimeMessageHelper.addAttachment(file.getFilename(), file); javaMailSender.send(mimeMessage); - return new EmailResponse(details.getRecipient().toString(),"Email with provided Attachment is Successfully sent to Recipent."); + return new EmailResponse(details.recipient(),"Email with provided Attachment is Successfully sent to Recipent."); } catch (MessagingException e) { throw new EmployeeException(HttpStatus.INTERNAL_SERVER_ERROR, diff --git a/src/main/java/com/employee_wise/assignment/serviceImpl/EmployeeServiceImpl.java b/src/main/java/com/employee_wise/assignment/serviceImpl/EmployeeServiceImpl.java index addee2a..1540bde 100644 --- a/src/main/java/com/employee_wise/assignment/serviceImpl/EmployeeServiceImpl.java +++ b/src/main/java/com/employee_wise/assignment/serviceImpl/EmployeeServiceImpl.java @@ -42,7 +42,7 @@ public class EmployeeServiceImpl implements EmployeeService{ @Override public PostEmployeeResponse addEmployee(Employee emp) { Employee saveEmp = this.repo.save(emp); - if(saveEmp == null) { + if(saveEmp == null) { // TODO refactor, this code is no go , will not get triggered - read the doc throw new EmployeeException(HttpStatus.INTERNAL_SERVER_ERROR, ErrorCodeEnum.EMPLOYEE_NOT_ADDED.getErrorCode(), ErrorCodeEnum.EMPLOYEE_NOT_ADDED.getErrorMessage()); @@ -109,6 +109,7 @@ public Employee deleteEmployeeById(String id) { @Override public PostEmployeeResponse updateEmployeeById(String id, Employee emp) { UUID uuid = UUID.fromString(id); + // TODO this is not how you do it List empl = this.repo.findById(uuid).stream().collect(Collectors.toList()); Employee employee = empl.get(0); if(employee == null) { @@ -150,7 +151,7 @@ public PostEmployeeResponse updateEmployeeById(String id, Employee emp) { emailServ.sendSimpleMail(email); return new PostEmployeeResponse(saveEmp.getId().toString(), "Employee is Updated"); } - + @Override public PostEmployeeResponse getNthManager(String id, Integer n) { UUID uuid = UUID.fromString(id); @@ -181,6 +182,8 @@ public PostEmployeeResponse getNthManager(String id, Integer n) { } } + // TODO this is a bad impl, create a write through cache here + // TODO Cache out the entire org in memory and return as fast as you can private Employee findNthManager(Employee employee, int n) { UUID managerId = employee.getReportsTo(); From cab636d0e15ef6d5774d0eb4044a675a93822b18 Mon Sep 17 00:00:00 2001 From: noga Date: Sat, 17 Feb 2024 20:13:35 +0530 Subject: [PATCH 2/2] Updated with sample how to reduce code. --- .../assignment/entity/EmailDetails.java | 4 +-- .../assignment/entity/Employee.java | 2 +- .../assignment/entity/NonNullCopier.java | 21 +++++++++++++ .../serviceImpl/EmployeeServiceImpl.java | 30 +++++-------------- 4 files changed, 32 insertions(+), 25 deletions(-) create mode 100644 src/main/java/com/employee_wise/assignment/entity/NonNullCopier.java diff --git a/src/main/java/com/employee_wise/assignment/entity/EmailDetails.java b/src/main/java/com/employee_wise/assignment/entity/EmailDetails.java index a48213e..d0ae91c 100644 --- a/src/main/java/com/employee_wise/assignment/entity/EmailDetails.java +++ b/src/main/java/com/employee_wise/assignment/entity/EmailDetails.java @@ -1,8 +1,8 @@ package com.employee_wise.assignment.entity; -public record EmailDetails( +public record EmailDetails ( String recipient, String msgBody, String subject, String attachment -){} +) implements NonNullCopier{} diff --git a/src/main/java/com/employee_wise/assignment/entity/Employee.java b/src/main/java/com/employee_wise/assignment/entity/Employee.java index ecc8183..a3780a2 100644 --- a/src/main/java/com/employee_wise/assignment/entity/Employee.java +++ b/src/main/java/com/employee_wise/assignment/entity/Employee.java @@ -19,7 +19,7 @@ @Getter @Setter @Document -public class Employee { +public class Employee implements NonNullCopier { @Id @Indexed(unique = true) diff --git a/src/main/java/com/employee_wise/assignment/entity/NonNullCopier.java b/src/main/java/com/employee_wise/assignment/entity/NonNullCopier.java new file mode 100644 index 0000000..e2b2b01 --- /dev/null +++ b/src/main/java/com/employee_wise/assignment/entity/NonNullCopier.java @@ -0,0 +1,21 @@ +package com.employee_wise.assignment.entity; + +import java.lang.reflect.Field; +import java.util.Arrays; + +public interface NonNullCopier { + + default void autoCopy(T src){ + if ( getClass() != src.getClass() ) return; + Field[] fields = getClass().getDeclaredFields(); + Arrays.stream(fields).forEach( f -> { + try { + Object val = f.get(src) ; + if ( val != null ){ + f.set(this, val); + } + } catch (IllegalAccessException ignored) { + } + }); + } +} diff --git a/src/main/java/com/employee_wise/assignment/serviceImpl/EmployeeServiceImpl.java b/src/main/java/com/employee_wise/assignment/serviceImpl/EmployeeServiceImpl.java index 1540bde..e53d29d 100644 --- a/src/main/java/com/employee_wise/assignment/serviceImpl/EmployeeServiceImpl.java +++ b/src/main/java/com/employee_wise/assignment/serviceImpl/EmployeeServiceImpl.java @@ -110,37 +110,23 @@ public Employee deleteEmployeeById(String id) { public PostEmployeeResponse updateEmployeeById(String id, Employee emp) { UUID uuid = UUID.fromString(id); // TODO this is not how you do it - List empl = this.repo.findById(uuid).stream().collect(Collectors.toList()); - Employee employee = empl.get(0); - if(employee == null) { + Optional opt = this.repo.findById(uuid); + if(opt.isEmpty()) { throw new EmployeeException(HttpStatus.INTERNAL_SERVER_ERROR, ErrorCodeEnum.EMPLOYEE_NOT_FOUND.getErrorCode(), ErrorCodeEnum.EMPLOYEE_NOT_FOUND.getErrorMessage()); } - else { - if(emp.getEmployeeName() != null) { - employee.setEmployeeName(emp.getEmployeeName()); - } - if(emp.getEmail() != null) { - employee.setEmail(emp.getEmail()); - } - if(emp.getPhoneNumber() != null) { - employee.setPhoneNumber(emp.getPhoneNumber()); - } - if(emp.getReportsTo() != null) { - employee.setReportsTo(emp.getReportsTo()); - } - if(emp.getProfileImage() != null) { - employee.setProfileImage(emp.getProfileImage()); - } - } + Employee employee = opt.get(); + // TODO this is another lotta code, no need, can do much better + employee.autoCopy(emp); Employee saveEmp = this.repo.save(employee); - if(saveEmp == null) { + if(saveEmp == null) { // TODO this never happens throw new EmployeeException(HttpStatus.INTERNAL_SERVER_ERROR, ErrorCodeEnum.EMPLOYEE_NOT_UPDATED.getErrorCode(), ErrorCodeEnum.EMPLOYEE_NOT_UPDATED.getErrorMessage()); } + // This never happens Employee mEmpl = getEmployeeById(saveEmp.getReportsTo().toString()); if(mEmpl == null) { throw new EmployeeException(HttpStatus.INTERNAL_SERVER_ERROR, @@ -190,7 +176,7 @@ private Employee findNthManager(Employee employee, int n) { while (managerId != null && n > BASE_LEVEL) { Optional optionalManager = this.repo.findById(managerId).stream().findFirst(); - if (!optionalManager.isPresent()) { + if (optionalManager.isEmpty()) { throw new EmployeeException(HttpStatus.INTERNAL_SERVER_ERROR, ErrorCodeEnum.MANAGER_NOT_FOUND.getErrorCode(), ErrorCodeEnum.MANAGER_NOT_FOUND.getErrorMessage());