reed all data from an input CSV file, created a report, and wrote it … #1623
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this task! Your implementation of the getStatistic method in WorkWithFile.java is mostly correct 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:
-
Method Length and Responsibility: The
getStatisticmethod is currently handling multiple responsibilities, which makes it quite lengthy and harder to maintain. It's important to refactor this method into smaller, more focused methods likereadData,calculateStatistics, andwriteReport. This will improve the readability and maintainability of your code. -
Exception Handling: The current approach to exception handling could be improved. Instead of just printing error messages, it's recommended to re-throw a
RuntimeExceptionwith a meaningful message. This will provide more context when an error occurs and help with debugging. -
Use of Constants: The strings
"supply"and"buy"are used directly in the code. Declaring these as constants will enhance the readability and maintainability of your code. -
String Concatenation: While your code does not currently use string concatenation inside loops, it's a good practice to use
StringBuilderfor concatenating strings, even outside of loops, to optimize memory usage.
Please address these issues to ensure your code meets the core requirements and maintains a high standard of quality. Once these changes are made, we can proceed with approving your pull request. Keep up the great work, and don't hesitate to reach out if you have any questions! 😊
✨ 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.
| public void getStatistic(String fromFileName, String toFileName) { | ||
| int totalSupply = 0; | ||
| int totalBuy = 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()); | ||
|
|
||
| // Calculating total supply and buy | ||
| if ("supply".equalsIgnoreCase(operation)) { | ||
| totalSupply += amount; | ||
| } else if ("buy".equalsIgnoreCase(operation)) { | ||
| totalBuy += amount; | ||
| } | ||
|
|
||
| } | ||
|
|
||
| } catch (IOException e) { | ||
| System.err.println("Error reading the file: " + e.getMessage()); | ||
| } catch (NumberFormatException e) { | ||
| System.err.println("Error parsing the amount: " + e.getMessage()); | ||
| } | ||
|
|
||
| int result = totalSupply - totalBuy; | ||
|
|
||
| try (BufferedWriter writer = new BufferedWriter(new FileWriter(toFileName))) { | ||
| writer.write("supply," + totalSupply + System.lineSeparator()); | ||
| writer.write("buy," + totalBuy + System.lineSeparator()); | ||
| writer.write("result," + result + System.lineSeparator()); | ||
| } catch (IOException e) { | ||
| System.err.println("Error writing to the file: " + e.getMessage()); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
The getStatistic method is too large and performs multiple tasks. According to the checklist, methods should be short and responsible for one task. Consider dividing this method into smaller methods, such as readData, calculateStatistics, and writeReport.
| try (BufferedReader reader = new BufferedReader(new FileReader(fromFileName))) { | ||
| String line; | ||
| while ((line = reader.readLine()) != null) { | ||
| String[] parts = line.split(","); |
There was a problem hiding this comment.
Consider using StringBuilder instead of String for concatenating strings in loops to optimize memory usage, as suggested in the checklist.
| if ("supply".equalsIgnoreCase(operation)) { | ||
| totalSupply += amount; | ||
| } else if ("buy".equalsIgnoreCase(operation)) { |
There was a problem hiding this comment.
The strings "supply" and "buy" should be declared as constants to improve code readability and maintainability, as recommended in the checklist.
| } catch (IOException e) { | ||
| System.err.println("Error reading the file: " + e.getMessage()); | ||
| } catch (NumberFormatException e) { | ||
| System.err.println("Error parsing the amount: " + e.getMessage()); |
There was a problem hiding this comment.
The exception handling could be improved. Instead of just printing the error message, re-throw a RuntimeException with a meaningful message, as suggested in the checklist.
| } catch (IOException e) { | ||
| System.err.println("Error writing to the file: " + e.getMessage()); |
There was a problem hiding this comment.
Similar to the previous comment, improve exception handling by re-throwing a RuntimeException with a meaningful message.
There was a problem hiding this comment.
Review Summary
Congratulations on your implementation of the WorkWithFile class! 🎉 Your refactoring of the getStatistic method into smaller, focused methods (readData, writeReport, and buildReport) is a great step towards improving code readability and maintainability. The use of constants for "supply" and "buy" enhances clarity, and your implementation of exception handling by re-throwing a RuntimeException with meaningful messages is commendable.
You've effectively used StringBuilder for efficient string concatenation and ensured proper resource management with try-with-resources for file handling. These are all excellent practices that align with the task requirements.
While your code meets the task requirements, remember to review the comments provided to further refine your code. Addressing any issues mentioned will help you avoid similar problems in the future and enhance your coding skills. Keep up the great work and continue learning from your experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
…to newFile