Implement Report generator #1286
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! There are a few critical issues that need to be addressed before we can approve the pull request. Specifically, the PurchaseOperation class should include a check to prevent negative inventory values, and the file path for reading the CSV should be correctly set. Additionally, ensure that the getOperationByCode method handles all operation codes appropriately. Once these issues are resolved, your code will be in great shape! Keep up the great work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
| return; | ||
| } | ||
| String operationCode = parts[0]; | ||
| Operation operation = getOperationByCode(operationCode); |
There was a problem hiding this comment.
Ensure that the getOperationByCode method handles all possible operation codes defined in the FruitTransaction.Operation enum. Currently, it returns null if the code is not found, which is handled by skipping the transaction. Consider logging or handling unexpected operation codes more explicitly if needed.
| public class Main { | ||
| public static void main(String[] args) { | ||
| FileReader fileReader = new CsvFileReader(); | ||
| List<String> inputReport = fileReader.read(""); |
There was a problem hiding this comment.
The file path for reading the input CSV file is currently an empty string. Replace it with the actual file path to ensure the program reads the correct file.
| int quantity = transaction.getQuantity(); | ||
| Storage.fruitStorage.put( | ||
| fruit, | ||
| Storage.fruitStorage.getOrDefault(fruit, 0) - quantity |
There was a problem hiding this comment.
Consider adding a check to ensure that the resulting quantity in storage does not become negative. If the purchase quantity exceeds the available stock, you might want to throw an exception or handle it appropriately.
There was a problem hiding this comment.
Great job on implementing the solution! 🎉 Your code is well-structured and adheres to the task requirements for the most part. However, there are a few critical points to address:
-
Negative Inventory Check: Ensure that the
PurchaseOperationclass includes a check to prevent negative inventory values. This is crucial for maintaining accurate stock levels. -
CSV File Path: Make sure all file operations use relative paths and are placed in the
src/main/resourcesfolder for consistency and ease of access. -
Handling All Operation Codes: The
getOperationByCodemethod in theFruitTransactionclass should handle all operation codes appropriately, throwing anIllegalArgumentExceptionfor unknown codes.
These issues are important for the functionality of your code, so please review and address them in future iterations. Keep up the good work, and continue learning from these experiences to enhance your coding skills! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| List<FruitTransaction> transactions = new ArrayList<>(); | ||
| for (int i = 1; i < data.size(); i++) { | ||
| String line = data.get(i); | ||
| String[] parts = line.trim().split(","); |
There was a problem hiding this comment.
| String[] parts = line.trim().split(","); | |
| String[] parts = line.split(","); |
Remove trim everywhere, It’s invalid data. Move separator to the constant fields
| try { | ||
| operation = Operation.getOperationByCode(operationCode); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new RuntimeException(e.getMessage(), e); | ||
| } |
| if (fruit.isEmpty()) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
is it expected behavior? you should throw an exception if you have invalid data in the input file
| try { | ||
| quantity = Integer.parseInt(parts[2].trim()); | ||
| } catch (NumberFormatException e) { | ||
| continue; |
| StringBuilder reportBuilder = new StringBuilder("fruit,quantity\n"); | ||
| String reportBody = inventory.entrySet() | ||
| .stream() | ||
| .map(entry -> entry.getKey() + "," + entry.getValue()) | ||
| .collect(Collectors.joining("\n")); |
There was a problem hiding this comment.
move literals to the constant fields. Use System.lineseparator
|
|
||
| public class ReportGeneratorImpl implements ReportGenerator { | ||
| @Override | ||
| public String generateReport(Map<String, Integer> inventory) { |
There was a problem hiding this comment.
| public String generateReport(Map<String, Integer> inventory) { | |
| public String generateReport() { |
Don't pass storage as a parameter. You can call it directly
|
|
||
| @Override | ||
| public OperationHandler getOperationHandler(FruitTransaction.Operation operation) { | ||
| return operationHandlers.get(operation); |
There was a problem hiding this comment.
better add a null check for the handler. and throw an exception if the handler doesn’t exist
No description provided.