Add core services and model for fruit shop calculations #1239
Conversation
Implemented essential services along with the data model to manage fruit transactions and calculations. Introduced strategies for operations, CSV parsing, storage interaction, and report generation.
There was a problem hiding this comment.
Thank you for your hard work on this project! Your implementation is quite solid and meets many of the task requirements. However, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Resource Management: The
readLinesmethod inCsvReaderServiceImpldoes not close theFileReaderandBufferedReader, which can lead to resource leaks. Please use a try-with-resources statement to ensure these resources are closed automatically. -
Type Consistency: The
calculatemethod inFruitShopCalculationServiceImplshould return aShopStorage<T>instead ofFruitShopStorageto maintain consistency with the generic typeTin theShopCalculationServiceinterface. This change will enhance flexibility and align with the interface definition. -
Generic Usage: The
savemethod in theShopStorageinterface should use the generic typeTinstead ofFruitto ensure the interface can handle any typeT. Update the method signature tovoid save(T t, int value, OperationType operationType);. -
Spelling Error: Correct the typo in the method name
validateCalcualtiontovalidateCalculationto avoid potential confusion.
These changes are crucial for ensuring the code is robust, flexible, and maintainable. Once these issues are addressed, 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 fruitStorage.keySet(); | ||
| } | ||
|
|
||
| private int validateCalcualtion(int result) { |
There was a problem hiding this comment.
There is a typo in the method name validateCalcualtion. It should be validateCalculation to correctly reflect its purpose and avoid potential confusion.
| public interface ShopStorage<T> { | ||
| int getAmount(T t); | ||
|
|
||
| void save(Fruit fruit, int value, OperationType operationType); |
There was a problem hiding this comment.
The save method should use the generic type T instead of Fruit to maintain consistency with the rest of the interface. Change the method signature to void save(T t, int value, OperationType operationType); to ensure the interface can handle any type T.
| import java.util.List; | ||
|
|
||
| public interface ShopCalculationService<T> { | ||
| FruitShopStorage calculate(List<AbstractTransaction<T>> transactions); |
There was a problem hiding this comment.
The calculate method currently returns a FruitShopStorage, which is specific to Fruit. To maintain consistency with the generic type T, consider changing the return type to ShopStorage<T>. This will allow the interface to be more flexible and handle different types of storage.
| private Stream<String> readLines(String filePath) { | ||
| try { | ||
| FileReader fileReader = new FileReader(filePath); | ||
| return new BufferedReader(fileReader).lines(); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("Cannot read file: " + filePath, e); | ||
| } | ||
| } |
There was a problem hiding this comment.
The readLines method opens a FileReader and BufferedReader but does not close them, which can lead to resource leaks. Consider using a try-with-resources statement to ensure these resources are closed automatically.
| private void writeHeader(FileWriter writer) throws IOException { | ||
| writer.write(FIRST_LINE); | ||
| writer.write(System.lineSeparator()); | ||
| writer.flush(); |
There was a problem hiding this comment.
The flush() method call after writing the header is unnecessary when using a FileWriter inside a try-with-resources statement, as the writer will be flushed and closed automatically. Consider removing this call to simplify the code.
| fileWriter.write( | ||
| item.toString().toLowerCase() + "," + storage.getAmount(item)); | ||
| fileWriter.write(System.lineSeparator()); | ||
| fileWriter.flush(); |
There was a problem hiding this comment.
Similar to the previous comment, the flush() method call after writing each line is unnecessary when using a FileWriter inside a try-with-resources statement. The writer will be flushed and closed automatically. Consider removing this call to simplify the code.
| } | ||
|
|
||
| @Override | ||
| public FruitShopStorage calculate(List<AbstractTransaction<Fruit>> abstractTransactions) { |
There was a problem hiding this comment.
The calculate method currently returns a FruitShopStorage, which is specific to Fruit. To maintain consistency with the generic type T in the ShopCalculationService interface, consider changing the return type to ShopStorage<Fruit>. This will allow the implementation to be more flexible and align with the interface definition.
Renamed FruitShopStorage to ShopStorage to generalize the storage interface. Fixed typographical errors in method names and improved readability in several method calls across multiple classes.
There was a problem hiding this comment.
Great job on your progress! 🎉 While your implementation is mostly well-aligned with the task requirements, there are a few critical issues that need attention:
-
Type Consistency: The
calculatemethod inFruitShopCalculationServiceImplshould return aShopStorage<T>instead ofFruitShopStorageto maintain consistency with the generic typeTin theShopCalculationServiceinterface. This will enhance flexibility and align with the interface definition. -
Resource Management: Ensure that the
FileReaderis included in the try-with-resources statement to prevent potential resource leaks. This can be achieved by usingtry (FileReader fileReader = new FileReader(filePath); BufferedReader bufferedReader = new BufferedReader(fileReader)) { ... }. -
Generic Usage: The
savemethod in theShopStorageinterface should use the generic typeTinstead ofFruitto ensure the interface can handle any typeT. Update the method signature tovoid save(T t, int value, OperationType operationType);. -
Map Implementation: Since
Fruitis not an enum, usingEnumMapforfruitStorageis not suitable. Consider using aHashMapor another appropriateMapimplementation.
These changes are crucial for ensuring the code's flexibility and functionality. Please review the comments and address these issues in future iterations to enhance your code quality. Keep up the good work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| ShopCalculationService<Fruit> shopCalculationService = new FruitShopCalculationServiceImpl( | ||
| new FruitShopStorage()); | ||
| ShopStorage<Fruit> fruitStorage = shopCalculationService.calculate(fruitsOperations); |
There was a problem hiding this comment.
The calculate method in FruitShopCalculationServiceImpl should return a ShopStorage<T> instead of FruitShopStorage to maintain consistency with the generic type T in the ShopCalculationService interface. This change will enhance flexibility and align with the interface definition.
|
|
||
| public class FruitShopStorage implements ShopStorage<Fruit> { | ||
| private static final String STORAGE_VALUE_ERROR = "Storage value cannot be negative"; | ||
| private final Map<Fruit, Integer> fruitStorage = new EnumMap<>(Fruit.class); |
There was a problem hiding this comment.
The fruitStorage is currently using EnumMap, which is not appropriate since Fruit is not an enum. Consider using a HashMap or another suitable Map implementation.
| } | ||
|
|
||
| @Override | ||
| public void save(Fruit fruit, int value, OperationType operationType) { |
There was a problem hiding this comment.
The save method in the ShopStorage interface should use the generic type T instead of Fruit to ensure the interface can handle any type T. Update the method signature to void save(T t, int value, OperationType operationType);.
| FileReader fileReader = new FileReader(filePath); | ||
| try (BufferedReader bufferedReader = new BufferedReader(fileReader)) { |
There was a problem hiding this comment.
The FileReader should be included in the try-with-resources statement to ensure it is closed properly. This prevents potential resource leaks. Consider using try (FileReader fileReader = new FileReader(filePath); BufferedReader bufferedReader = new BufferedReader(fileReader)) { ... }.
| @Override | ||
| public FruitShopStorage calculate(List<AbstractTransaction<Fruit>> abstractTransactions) { | ||
| abstractTransactions.forEach(this::processFruitTransaction); | ||
| return fruitShopStorage; |
There was a problem hiding this comment.
The calculate method should return a ShopStorage<Fruit> instead of FruitShopStorage to maintain consistency with the generic type T in the ShopCalculationService interface. This change will enhance flexibility and align with the interface definition.
Implemented essential services along with the data model to manage fruit transactions and calculations. Introduced strategies for operations, CSV parsing, storage interaction, and report generation.