Code Review Round

Hiring Managaer Round

Code review interview context

 1	You need to review the REST API written in Java. You need to perform the code review in 40 mins. For simplicity, the PR is shared in a Google doc.
 2	Consider that this PR is raised by a very junior developer. Hence, assume that this code has lots of issues and improvement areas. 
 3	Like any real PR, there are some minor and some major issues. Major issues will have more weightage in the scoring of the code review round. Please prioritise 
 4	You can do whatever you would normally do while performing code review like:
 1	Asking a question.
 2	Suggesting improvements / alternate ways of writing code.
 3	Share a link to specs, documentation, guideline, blog, video, etc.
 5	You can use the internet like you would do while performing a code review. However, please be mindful of the 40 mins time limit. We live in an age where most of the new code will soon be AI generated. That’s why we believe having engineers capable of performing great Code Reviews is even more important than before. This is the reason we discourage the use of AI tools during this task
 6	You can assume that the code compiles and runs, all of the usings are there, etc.
 7	Pro tip: Try to add comments as you go instead of adding all the comments towards the end.


*/



package vnd.credit.loans;

import java.util.Optional;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;



/**
Creating a new version of the borrowing functionality that registers the loan in a 3rd party Loan Management System (wrapped in feign client service). Since we get a lot of errors from the 3rd party I’ve added a functionality to let our staff move the money on behalf of the user if they give us a call that something’s not working properly!!!
*/
@RequestMapping("/v3/accounts/")
@RestController
public class LoanV3Controller {

   @Autowired
   public AccountService accountService;

   @Autowired
   public LoanManagementService loanManagementService;

   public static final Logger logger = LoggerFactory.getLogger(LoanV3Controller.class); 

   @PutMapping(value = "/new/{accountId}/v2/loans/borrow")
   public void borrowMoney(@PathVariable String accountId,
                                @RequestParam boolean isAdminAgent,
 				  @RequestParam double loanAmount,
                                @RequestParam String sourceAccountId) {
      
       if (!isAdminAgent) {
      // for admins we don’t need to check the ownership
     Account acc = accountService.getAccount(accountId);
     if (acc.getOwner() != AuthContext.getCurrentUserID()) {
         throw new InternalServerError();
     }
       }

       // make sure the user is allowed to borrow this amount
  if (loanAmount < loanManagementService.getCreditLimit(accountId)) {
      
       Account sourceAccount = accountService.getAccount(sourceAccountId);
       Account destinationAccount = accountService.getAccount(accountId);

       Optional.ofNullable(sourceAccount).orElseThrow();
       double balance = sourceAccount.getBalance();
      
       if (balance > loanAmount) {
      accountService.debit(sourceAccount, loanAmount);
      accountService.credit(destinationAccount, loanAmount);
      loanManagementService.registerLoan(new Random().nextInt(1000000), 
   loanAmount, AuthContext.getCurrentUserID());
       } else {
           throw new InternalServerError();
       }
}
   }

}

import org.springframework.beans.factory.annotation.Autowired;
// use constructor based injection

 public static final Logger logger = LoggerFactory.getLogger(LoanV3Controller.class); 
 // logger defined but not used

 @PutMapping(value = "/new/{accountId}/v2/loans/borrow")
  public void borrowMoney(@PathVariable String accountId,
                               @RequestParam boolean isAdminAgent,
				  @RequestParam double loanAmount,
                               @RequestParam String sourceAccountId) {

   // no idempotancy in api, make endpoint idompotent
   // no param validation
   // disperancy in url versioning

if (!isAdminAgent) 
// should not be passed as boolean but retived from current-user-id, make lead to security risk

throw new InternalServerError();
// generic error, should not be there should be meaningful with proper response code

Account destinationAccount = accountService.getAccount(accountId);

// duplicate code, reuse from above

if (loanAmount < loanManagementService.getCreditLimit(accountId)) {
      
       Account sourceAccount = accountService.getAccount(sourceAccountId);
       Account destinationAccount = accountService.getAccount(accountId);

       Optional.ofNullable(sourceAccount).orElseThrow();
       double balance = sourceAccount.getBalance();
      
       if (balance > loanAmount) {
      accountService.debit(sourceAccount, loanAmount);
      accountService.credit(destinationAccount, loanAmount);
      loanManagementService.registerLoan(new Random().nextInt(1000000), 
   loanAmount, AuthContext.getCurrentUserID());
       } else {
           throw new InternalServerError();
       }

// move this to service layer, should not reside at controller 
// if block should be made transaction
// avoid magic number, should be declare as constant
// avoid random use a derived value, maybe something like timestamp

loanManagementService.registerLoan(new Random().nextInt(1000000), 
  loanAmount, AuthContext.getCurrentUserID());