implemented getStatistic() method #1626
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this Pull Request! 😊 While your implementation of the getStatistic method in the WorkWithFile class is mostly correct and meets the core functionality requirements, there are a few critical issues that need to be addressed before we can approve it:
-
Method Length and Single Responsibility Principle: The
getStatisticmethod is currently too long and handles multiple responsibilities. It's important to break it down into smaller methods, each responsible for a single task, to improve readability and maintainability. -
Exception Handling: Using
e.printStackTrace()is not ideal for production code. Instead, consider re-throwing aRuntimeExceptionwith a meaningful message or using a logging framework to handle exceptions more gracefully. -
Use of Constants: Hardcoded strings like
"supply","buy", and"result"should be declared as constants. This will enhance the readability and maintainability of your code.
These changes are crucial for ensuring that your code adheres to best practices and is easier to maintain in the future. Keep up the great work, and I look forward to seeing your improvements! 🚀
✨ 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.
| } catch (IOException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Instead of using e.printStackTrace(), consider throwing a runtime exception or logging the error using a logging framework. This will provide more control over error handling and is generally more suitable for production environments.
There was a problem hiding this comment.
Great job on your progress! 🎉 Your implementation of the WorkWithFile class is mostly correct, and you've made significant improvements to the code. Here are some key points to consider for future enhancements:
-
Method Length and Single Responsibility Principle: The
getStatisticmethod is currently handling multiple tasks, making it lengthy and complex. Breaking it down into smaller, more focused methods will improve both readability and maintainability. -
Exception Handling: While using
RuntimeExceptionis a step forward, consider adopting a more specific exception type or a logging framework for more robust error handling in production code. -
Use of Constants: The hardcoded strings
"supply","buy", and"result"should be declared as constants. This change will enhance the readability and maintainability of your code.
You've addressed some of these issues in your recent updates, which is commendable. Keep up the good work and continue refining your code. Remember, addressing these critical issues will help you avoid similar problems in the future and enhance your coding skills. Keep learning and improving! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| public void getStatistic(String fromFileName, String toFileName) { | ||
| int supply = 0; | ||
| int buy = 0; | ||
|
|
||
| try (BufferedReader reader = new BufferedReader(new FileReader(fromFileName))) { | ||
| String line; | ||
| while ((line = reader.readLine()) != null) { | ||
| String[] parts = line.split(","); | ||
| String operation = parts[0].trim(); | ||
| int amount = Integer.parseInt(parts[1].trim()); | ||
|
|
||
| if ("supply".equalsIgnoreCase(operation)) { | ||
| supply += amount; | ||
| } else if ("buy".equalsIgnoreCase(operation)) { | ||
| buy += amount; | ||
| } | ||
| } | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| writeToFile(supply, buy, toFileName); | ||
| } |
There was a problem hiding this comment.
The getStatistic method is too long and handles multiple responsibilities. Consider breaking it down into smaller methods, each responsible for a single task, to improve readability and maintainability.
| } catch (IOException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Using RuntimeException for exception handling is not ideal. Consider using a more specific exception type or a logging framework for better error handling.
| if ("supply".equalsIgnoreCase(operation)) { | ||
| supply += amount; | ||
| } else if ("buy".equalsIgnoreCase(operation)) { |
There was a problem hiding this comment.
The strings "supply" and "buy" are hardcoded. Consider declaring them as constants to enhance readability and maintainability.
| try (BufferedWriter writer = new BufferedWriter(new FileWriter(toFileName))) { | ||
| writer.write("supply," + supply + "\n"); | ||
| writer.write("buy," + buy + "\n"); | ||
| writer.write("result," + result + "\n"); |
There was a problem hiding this comment.
The string "result" is hardcoded. Consider declaring it as a constant to enhance readability and maintainability.
No description provided.