2
\$\begingroup\$

The following microservice is just handling loan application and response the result of the application is accepted or rejected. It is part of my assignment for an interview.

The project description is that; A user sends them Id, username, surname, and monthly salary via REST API. Assuming that you have got already credit scores in DB, you must return the loan application result based on that credit score. And then save the application on DB.

They give me the ruleset. And they said that please show what you have got like design patterns, test-driven development, clean code, REST API design, SOLID principles, OOP, etc. Also Docker, MongoDB

But they rejected me. Anyway, I appreciated if you judging my code, need to improve my skills.

I try to use the Strategy and Factory patterns to process the application.

Source code is huge so the project repo is HERE, please check this.

LoanController Class

@Slf4j
@RestController
@RequestMapping(value = "/api")
public class LoanController {

    private final LoanApplicantService loanApplicantService;

    public LoanController(LoanApplicantService loanApplicantService) {
        this.loanApplicantService = loanApplicantService;
    }

    @PostMapping("/loan")
    public ResponseEntity<Response> applyForLoan(@Valid @RequestBody LoanRequest request) {
        Response response;
        try{
            log.info("incoming request: {}", request.getId());
            response = Response.builder()
                    .success(Boolean.TRUE)
                    .data(loanApplicantService.process(request))
                    .build();

        }catch ( Exception e ) {
            log.error("Exception: {}", e.getCause());
            response = Response.builder()
                    .success(Boolean.FALSE)
                    .message(e.getMessage())
                    .build();

        }
        return new ResponseEntity<>(response, HttpStatus.OK);
    }

}

LoanApplicantService class

@Slf4j
@Service
public class LoanApplicantService implements ILoanApplicantService{

    private final LoanApplicantRepository loanApplicantRepository;

    private final LoanApplicantResultRepository resultRepository;

    private final LoanApplicantScoreService scoreService;

    private final SmsService smsService;

    public LoanApplicantService(LoanApplicantRepository loanApplicantRepository,
                                LoanApplicantScoreService scoreService,
                                LoanApplicantResultRepository resultRepository,
                                SmsService smsService){
        this.loanApplicantRepository = loanApplicantRepository;
        this.scoreService = scoreService;
        this.resultRepository = resultRepository;
        this.smsService = smsService;
    }

    public LoanResponse process(LoanRequest request) {
        // convert request to entity
        LoanApplicant applicant = LoanConverter.convertRequest(request).build();

        // save request
        save(applicant);

        // find score
        LoanApplicantScore applicantScore = scoreService.findApplicantScoreById(applicant.getId());

        // get strategy by score
        LoanStrategy strategy = LoanStrategyFactory.getStrategy(applicantScore.getScore(),
                applicant.getMonthlySalary());

        // execute the result of application
        LoanApplicantResult result = strategy.execute(applicantScore, applicant);

        // save result of application
        resultRepository.save(result);

        LoanResponse response =  LoanConverter.convertResult(result).build();

        // send sms
        smsService.sendSMS(applicant);

        return response;

    }

    public LoanApplicant save(LoanApplicant applicant) {
       return loanApplicantRepository.save(applicant);
    }

}

LoanStrategyFactory class

public class LoanStrategyFactory {

    private LoanStrategyFactory() {}

    public static LoanStrategy getStrategy(Integer score, BigInteger salary) {

        if (LoanRange.MEDIUM.contains(score) && isSalaryUnderBarrier(salary)) {

            return new MediumScoreStrategy();

        } else if(LoanRange.HIGH.contains(score)) {

            return new HighScoreStrategy();

        } else {

            return new LowScoreStrategy();
        }
    }

    private static boolean isSalaryUnderBarrier(BigInteger salary) {
        return LoanConstant.INCOME_BARRIER.compareTo(salary) > 0;
    }

}

LoanExceptionHandler class

@ControllerAdvice
public class LoanExceptionHandler extends ResponseEntityExceptionHandler {

    @Override
    protected ResponseEntity<Object> handleMethodArgumentNotValid(MethodArgumentNotValidException ex,
                                                                    HttpHeaders headers,
                                                                    HttpStatus status,
                                                                    WebRequest request) {
        Response response = Response.builder()
                .success(Boolean.FALSE)
                .message(ex.getMessage())
                .build();

        return new ResponseEntity<>(response, headers, status);
    }

    @Override
    protected ResponseEntity<Object> handleHttpMessageNotReadable(HttpMessageNotReadableException ex,
                                                                  HttpHeaders headers,
                                                                  HttpStatus status,
                                                                  WebRequest request) {
        Response response = Response.builder()
                .success(Boolean.FALSE)
                .message("Required request body is missing")
                .build();

        return new ResponseEntity<>(response, headers, status);
    }

}

LoanConverter class

public class LoanConverter {

    private LoanConverter() { }

    public static LoanApplicant.LoanApplicantBuilder convertRequest(LoanRequest loan) {
        return Optional.ofNullable(loan).map(d -> LoanApplicant.builder()
                .id(loan.getId())
                .name(loan.getName())
                .surname(loan.getSurname())
                .monthlySalary(new BigInteger(loan.getMonthlySalary()))
                .phoneNumber(loan.getPhoneNumber()))
                .orElseGet(LoanApplicant::builder);
    }

    public static LoanResponse.LoanResponseBuilder convertResult(LoanApplicantResult loan) {
        return Optional.ofNullable(loan).map(d -> LoanResponse.builder()
                .amount(loan.getAmount())
                .status(loan.getStatus()))
                .orElseGet(LoanResponse::builder);
    }

}

LoanRange class

import org.apache.commons.lang3.Range;

public class LoanRange {

    private LoanRange() {}

    public static final Range<Integer> LOW = Range.between(0, 499);
    public static final Range<Integer> MEDIUM = Range.between(499, 999);
    public static final Range<Integer> HIGH = Range.between(999, Integer.MAX_VALUE);

}

LoanControllerTest class

@WebMvcTest
public class LoanControllerTest {

    private static final String API = "/api/loan";

    MockMvc mockMvc;

    private final ObjectMapper objectMapper = new ObjectMapper();

    @Autowired
    LoanController loanController;

    @MockBean
    LoanApplicantService loanApplicantService;

    @BeforeEach
    public void setup() {
        this.mockMvc = MockMvcBuilders
                .standaloneSetup(loanController)
                .setControllerAdvice(new LoanExceptionHandler())
                .build();
    }

    @Test
    void whenRequestLoan_thenReturn200() throws Exception {
        LoanRequest request = LoanRequest.builder()
                .id("12312312312")
                .name("fuat")
                .surname("kara")
                .monthlySalary("1000")
                .phoneNumber("5312341234").build();

        performRequest(request, status().isOk());
    }

    @Test
    void whenInvalidRequestLoan_thenReturn400() throws Exception {
        LoanRequest request = LoanRequest.builder()
                .id("123123123123")
                .name("fuat")
                .surname("kara")
                .monthlySalary("1000")
                .phoneNumber("5312341234").build();

        performRequest(request, status().isBadRequest());
    }

    @Test
    void whenNullRequestLoan_thenReturn400() throws Exception {
        mockMvc.perform(post(API))
                .andExpect(status().isBadRequest());
    }

    private ResultActions performRequest(Object request, ResultMatcher matcher) throws Exception {
        return performRequest(request, post(API), matcher);
    }

    private ResultActions performRequest(Object request, MockHttpServletRequestBuilder builder, ResultMatcher matcher) throws Exception {
        return mockMvc.perform(
                builder
                        .content(objectMapper.writeValueAsString(request))
                        .contentType(MediaType.APPLICATION_JSON))
                .andDo(print())
                .andExpect(matcher);
    }
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Do you have a description of the assignment? If yes please add it to the question. \$\endgroup\$ – Marc Sep 11 '20 at 1:00
  • \$\begingroup\$ Yes, I will add the description and delete these classes here. \$\endgroup\$ – fuat Sep 11 '20 at 7:22
  • 2
    \$\begingroup\$ The code for review needs to be present in the question, see codereview.stackexchange.com/help/on-topic \$\endgroup\$ – mtj Sep 11 '20 at 7:49
2
\$\begingroup\$

Method applyForLoan.

  1. Either success/failed op, will return an 200 OK http response?! Maybe not a good idea, and you may respond something like code 500(internal server error) for failed op maybe
  2. Duplicated statements, such as instancing the response var,success state, and build()ing it.

I suggest something like following

//not sure if builder is tent, or where is it
Response.Builder _resp_builder = Response.builder();
HttpStatus _status;
try{
    log.info("incoming request: {}", request.getId());
    LoanResponse _load_resp = loanApplicantService.process(request);
    _resp_builder.data(_load_resp);
    _status = HttpStatus.OK;

}catch ( Exception e ) {
    log.error("Exception: {}", e.getCause());
    _resp_builder.message(e.getMessage());
    _status = HttpStatus.InternalServerError;//?
}
return new ResponseEntity(_resp_builder.success(_status==HttpStatus.OK).build(), _status);

About method process in type LoanApplicantService.

I'm not sure if transactions should be there for those persistency statements, or not. Anyway, the process looks a little scary, since no any return type of each DOA is checked.

For example, what if request entity cannot be saved? or findApplicantScoreById returns null? No check for results, not a transactions to rollback by any failure, I think not a good move.

Having some logging could be good.

Personally I'm not fan of those two POJO/type mapping/converting stuffs, and sometimes this is possible to let a POJO play act of two scheme (e.g. as both input arg, and entity).

I would split this process method into smaller units, where related statements(e.g. execute, and save result taks) get placed into dedicated types/units.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your answer, For 0. it is an approach for error handling. You can read some here. You return 200 with Success is False instead of return 500. So your suggestion is not useful. And for rollback scenarios, I use MongoDB. Maybe, you can check the whole project repo from Github. \$\endgroup\$ – fuat Sep 11 '20 at 7:21
2
\$\begingroup\$

First of all, it starts out with an inconsistent coding style: if you have a blank before an opening brace, have it everywhere. If you handed this in to me when applying for a job I'd think you don't take me serious. Apply a code formatter!

Then, where are tests? If TDD is explicitly asked for, not having a single test is a bad joke. (OK, I've seen that there are some in the repository, but not included in the question.)

Method applyForLoan: forward declaration of the response variable. Don't do this. As 911992 already mentioned, returning HTTP status 200 in the exception case. Not making use of standard solutions like WebApplicationExeption. Furthermore "hiding" the business code in response building, which makes it hard to spot. "Info" logging for clear debug information. Catch-all block for undeclared exceptions.

I'd have expected something like:

try {
    LoanResponse loanResponse = loanApplicantService.process(request);
    log.debug("incoming request: {} ==> {}", request.getId(), loanResponse);
    
    return loanResponse; // you should know enough about handing objects as outputs, if you can handle them as inputs
} catch (LoanServiceException e) { // provided somthing like this exists
    throw new WebApplicationExeption(e, Response.Status.INTERNAL_SERVER_ERROR);
}

The process method in LoanApplicantService is a jumble of responsibilites from database management to sending sms-messages. Separate concerns. Why is there a save method which delegates to the real save-method. Is there any error handling?

\$\endgroup\$
1
  • \$\begingroup\$ Rest of the code in my Github. Please check the repo, I have %90 test coverage. \$\endgroup\$ – fuat Sep 11 '20 at 7:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.