Conversation
SURABHI2003
requested review from
NehaKariya0309,
Prathyusha5c0,
ShellyAgg and
VaniThapar
| public class Wallet { | ||
|
|
||
| private String walletid; | ||
| private String Walletid; |
There was a problem hiding this comment.
Better to keep same naming conventions for a single class. Either small(for now) or camelcase. Might create problems later.
| public Wallet getWalletInfo(@PathVariable String userId) throws ControllerException{ | ||
|
|
||
| @GetMapping("/get-wallet-info/{userId}") | ||
| public ResponseEntity<Wallet> getWalletInfo(@PathVariable String userId) throws ControllerException { |
There was a problem hiding this comment.
Can u write the function name getWalletInfoByUserId, like it would help us in future , because there are 2 apis for getting wallet info , one by user id and another by wallet id. It may create confusion later.
| Wallet currentWallet = walletService.getWalletById(walletId); | ||
|
|
||
| if (currentWallet == null) { | ||
| return null; |
There was a problem hiding this comment.
Will be better if u can log a statement here , "Wallet not found" , would help in debugging.
| } | ||
| } | ||
|
|
||
| @PostMapping("/{walletId}/addAmount") |
There was a problem hiding this comment.
I think api name should have function name followed by {walletId}.
| public Wallet getWalletById(String walletId) throws ServiceException{ | ||
| try { | ||
| Wallet wallet = walletRepository.getWalletById(walletId); | ||
| User user = walletRepository.getUserIdWithWallet(wallet); |
There was a problem hiding this comment.
Why are we setting user here?
There was a problem hiding this comment.
We have to set user explicitly because when I'm trying to get the wallet info through walletId, I'm getting null instead of user object.
| @@ -31,48 +31,80 @@ public class WalletController extends BaseController { | |||
| public List<Wallet> getAllWallets() throws ControllerException { | |||
| try { | |||
There was a problem hiding this comment.
Function doc is missing,
This holds true for all the methods.
|
|
||
| return walletList; | ||
| } catch (Exception e) { | ||
| log.error("Error encountered in getting the users"); |
There was a problem hiding this comment.
The error message should be
log.error("Error encountered in getting the users", e);
| try { | ||
| return walletService.getWalletInfo(userId); | ||
| if (userId == null || userId.isEmpty()) { | ||
| return ResponseEntity.badRequest().body(null); |
There was a problem hiding this comment.
Throw exception in case of userId is empty, With exception message.
| try { | ||
| return walletService.getWalletById(walletId); | ||
| if (walletId == null || walletId.isEmpty()) { | ||
| return null; |
There was a problem hiding this comment.
Throw exception here with message
| public ResponseEntity<String> addAmountToWallet(@PathVariable String walletId, @RequestParam Float amount) throws ControllerException { | ||
| try { | ||
| if (walletId == null || walletId.isEmpty()) { | ||
| return ResponseEntity.badRequest().body(null); |
There was a problem hiding this comment.
Its not a badRequest, Throw exception with error message
| return sqlQueryBuilder.toString(); | ||
| } | ||
|
|
||
| public static String getUserIdWithWalletQuery(@Param("ex") Wallet ex) { |
There was a problem hiding this comment.
Follow the proper naming convention.
what ex is denoting here?
There was a problem hiding this comment.
The Query here might return no values.
Please add the proper validation.
Or this can be handled at service layer as well.
| } else { | ||
| sqlQueryBuilder.append("NULL"); | ||
| } | ||
|
|
There was a problem hiding this comment.
The below code is not making sense,
Why are we comparing with null;
Instead throw exception when walletId is null
if (walletid == null) {
throw new IllegalArgumentException("walletid cannot be null");
}
There was a problem hiding this comment.
Add similar exception handling in all the other methods where params are necessary
| @NoArgsConstructor | ||
| @AllArgsConstructor | ||
| public class Wallet { | ||
|
|
There was a problem hiding this comment.
Can the wallet have more fields?
| Wallet getWalletById(String walletId) throws ServiceException; | ||
| Wallet getWalletByWalletId(String walletId) throws ServiceException; | ||
|
|
||
| void addAmountToWallet(String walletId, Float amount) throws ServiceException; |
There was a problem hiding this comment.
Can another method be added to deductFromWallet()
|
|
||
|
|
||
| @Override | ||
| public void generateWallet(String userId) throws ServiceException { |
There was a problem hiding this comment.
The method name should be generateWalletForUser
3 participants
Created Wallet feature. Implemented all the methods in all the corresponding layers.