Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hw fruit shop #1231

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

RomanChygryn
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Hello! 😊 You've done a great job with the implementation, and your code largely meets the task requirements. However, there are a few critical issues that need to be addressed before we can approve the pull request:

  1. Exception Handling: When catching IOException, it's important to include the original exception in the RuntimeException to preserve the stack trace and provide more context for debugging. This will help in identifying the root cause of the issue more effectively.

  2. Operation Handler Null Check: The getOperationHandler method currently returns null if an operation is not found. This can lead to NullPointerException. Consider throwing an exception or providing a default handler to handle such cases.

  3. Encapsulation and Thread Safety: The storage field is public and static, which can lead to encapsulation issues and potential thread safety problems. Consider using private access with public getter methods or synchronized methods to manage access if thread safety is a concern.

  4. Efficiency Improvements: There are several places where efficiency can be improved:

    • Avoid calling storageDao.getQuantity(fruitName) multiple times if it involves a costly operation. Store the result in a variable and reuse it.
    • In the getAll method, consider returning Storage.storage.entrySet() directly if immutability is not a concern, or use Collections.unmodifiableSet() to return an unmodifiable view if needed.
  5. Error Messages and Logging: Improve error messages for better clarity and debugging:

    • Use correct punctuation in error messages, such as changing Cant write filetoCan't write file`.
    • Provide more specific feedback in exception messages, such as indicating the expected input format or which part is missing.
    • Consider logging errors or providing more detailed information about the transaction or parsing errors to aid in debugging.

By addressing these issues, your code will be more robust and maintainable. Keep up the great work, and I'm confident you'll nail these 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.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Great job on implementing the fruit transaction processing system! 🎉 Your code is well-structured and follows a logical sequence, making it easy to understand and maintain. While there are a few areas for improvement, such as optimizing method calls and ensuring thread safety, these do not detract from the overall functionality of your solution.

Here are some key points to consider for future enhancements:

  • File Paths: Ensure that input and output file paths are correctly placed in the src/main/resources folder.
  • Concurrency: Consider using ConcurrentHashMap for storage to handle potential concurrency issues in multi-threaded environments.
  • Efficiency: Optimize your code by reducing redundant method calls, such as using cached values where possible.
  • Exception Handling: Maintain consistency in exception handling to provide clear context and prevent unexpected crashes.
  • Documentation: Clearly document assumptions, such as the presence of headers in data files, to aid future maintenance.

These improvements will enhance the robustness and efficiency of your application. Keep up the great work, and continue learning from these experiences to refine your coding skills! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

Comment on lines +22 to +23
public Set<Map.Entry<String, Integer>> getAll() {
return Map.copyOf(Storage.storage).entrySet();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getAll method uses Map.copyOf(Storage.storage).entrySet(), which creates a copy of the storage map. This might be unnecessary if you only need to read the entries. Consider returning Storage.storage.entrySet() directly if immutability is not a concern, or use Collections.unmodifiableSet to return an unmodifiable view if needed.

import java.util.HashMap;

public class Storage {
public static final HashMap<String, Integer> storage = new HashMap<>();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a static HashMap for storage can lead to concurrency issues if accessed by multiple threads simultaneously. Consider using a thread-safe collection like ConcurrentHashMap if your application will be multi-threaded. Additionally, static fields can make unit testing more challenging, as they maintain state across tests.

Comment on lines +31 to +41

public static FruitTransaction.Operation getOperation(String letter) {
return Arrays.stream(FruitTransaction.Operation.values())
.filter(a -> a.getLetter().equals(letter))
.findFirst()
.orElseThrow(
() -> new IllegalArgumentException(
"No enum constant for letter: " + letter
)
);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getOperation method throws an IllegalArgumentException if no matching operation is found. This is a good practice for handling invalid inputs. Ensure that this exception is handled appropriately wherever this method is used to prevent unexpected crashes.

Comment on lines +22 to +27
storageDao.getAll().stream()
.map(Map.Entry::getKey)
.forEach(e -> stringBuilder.append(LINE_SEPARATOR)
.append(e)
.append(CSV_SEPARATOR)
.append(storageDao.getQuantity(e)));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the create method, you are calling storageDao.getQuantity(e) inside the forEach loop. This can be inefficient if getQuantity involves complex operations or if the storage is large. Since you already have the entries from getAll(), consider using Map.Entry::getValue to directly access the quantity, which would be more efficient.

public List<FruitTransaction> convertToTransaction(List<String> data) {
List<FruitTransaction> transactions = new ArrayList<>();

for (int i = 1; i < data.size(); i++) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loop starts from index 1, which assumes that the first line of the data is a header. Ensure that this assumption is documented or handled elsewhere if the input data format changes.


try {
FruitTransaction.Operation operation =
FruitTransaction.Operation.getOperation(parts[0].trim().toLowerCase());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getOperation method is called with parts[0].trim().toLowerCase(). Ensure that the operation letters in the input data are consistently formatted in lowercase to avoid unexpected errors.

Comment on lines +37 to +38
} catch (IllegalArgumentException e) {
throw new RuntimeException("Error parsing line: " + line, e);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching IllegalArgumentException and rethrowing it as a RuntimeException with a custom message is a good practice for providing more context. Ensure that this exception handling is consistent throughout the application.

Comment on lines +22 to +26
if (handler == null) {
throw new IllegalArgumentException(
"No handler found for operation: " + transaction.getOperation()
);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code checks if the handler is null and throws an IllegalArgumentException if no handler is found for a given operation. This is a good practice for handling unexpected cases. Ensure that the OperationStrategy is correctly set up to provide handlers for all expected operations to avoid this exception.

Comment on lines +14 to +19
Integer fruitQuantity = storageDao.getQuantity(fruitName);
if (fruitQuantity < quantity) {
throw new RuntimeException("Not enough " + fruitName + " in the storage");
}
Integer previousQuantity = storageDao.getQuantity(fruitName);
Integer newQuantity = previousQuantity - quantity;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are calling storageDao.getQuantity(fruitName) twice to get the current quantity. You can optimize this by using the fruitQuantity variable obtained in line 14 for both the check and the calculation of newQuantity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants