Hw 1 solution #1233

ovodolazov · 2024-11-02T18:00:43Z

No description provided.

mateacademy-ai-mentor

Thank you for your hard work on this project! While your code is generally well-structured and adheres to many best practices, there are a few critical issues that need to be addressed before we can approve the pull request:

Exception Handling: Ensure that the OperationException and FileException classes are defined in your project. These custom exceptions are crucial for handling operation-related and file-related errors appropriately.
Null Checks: Add checks to handle cases where a fruit is not present in the fruitsStorage to prevent potential NullPointerException during supply, return, and purchase operations.
Operation Handler Map: Ensure that the operationHandlerMap includes a check for unregistered operation types to prevent NullPointerException.
Data Parsing: Add error handling for Integer.parseInt to manage potential NumberFormatException if the quantity is not a valid integer.
Data Integrity: Ensure that the input data format always includes a header to avoid missing data when processing reports.
Data Encapsulation: Consider returning an unmodifiable view or a copy of the fruitsStorage map to prevent unintended external modifications.

Addressing these issues will enhance the robustness and reliability of your code. Keep up the great work, and I'm confident you'll have these resolved in no time! 😊

✨ 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.

mateacademy-ai-mentor · 2024-11-02T18:04:04Z

src/main/java/core/basesyntax/db/Storage.java

+    public Map<String, Integer> getFruitsStorage() {
+        return fruitsStorage;


Returning a direct reference to the fruitsStorage map can lead to unintended modifications from outside the class. Consider returning an unmodifiable view or a copy of the map to prevent external modifications.

mateacademy-ai-mentor · 2024-11-02T18:04:04Z

src/main/java/core/basesyntax/file/FileReaderImpl.java

+        try {
+            reportList = Files.readAllLines(Path.of(filePath));
+        } catch (IOException e) {
+            throw new FileException("Can't read data from file: " + filePath);


Ensure that the FileException class is defined in your project. If not, you need to create this custom exception class to handle file-related errors appropriately.

mateacademy-ai-mentor · 2024-11-02T18:04:04Z

src/main/java/core/basesyntax/file/FileWriterImpl.java

+        try {
+            Files.writeString(Path.of(filePath), report);
+        } catch (IOException e) {
+            throw new FileException("Can't write data to file: " + filePath);


Ensure that the FileException class is defined in your project. If not, you need to create this custom exception class to handle file-related errors appropriately.

mateacademy-ai-mentor · 2024-11-02T18:04:04Z

src/main/java/core/basesyntax/model/OperationType.java

+                return type;
+            }
+        }
+        throw new OperationException("Operation not found by code: " + codeOperation);


Ensure that the OperationException class is defined in your project. If not, you need to create this custom exception class to handle invalid operation codes appropriately.

mateacademy-ai-mentor · 2024-11-02T18:04:04Z

src/main/java/core/basesyntax/service/impl/DataConverterImpl.java

+    @Override
+    public List<ShopOperation> convertToOperation(List<String> reportList) {
+        List<ShopOperation> shopOperationList = new ArrayList<>();
+        for (String line : reportList.subList(LIST_START, reportList.size())) {


The method skips the first line of the reportList, assuming it is a header. Ensure that the input data format always includes a header to avoid missing data.

mateacademy-ai-mentor · 2024-11-02T18:04:04Z

src/main/java/core/basesyntax/service/impl/StrategyOperationServiceImpl.java

+    public OperationHandler get(OperationType operationType) {
+        return operationHandlerMap.get(operationType);


Consider adding a check to handle cases where operationType is not present in the operationHandlerMap. This will prevent potential NullPointerException if an unregistered operation type is requested.

mateacademy-ai-mentor · 2024-11-02T18:04:04Z

src/main/java/core/basesyntax/strategy/PurchaseOperation.java

+    @Override
+    public void handle(ShopOperation shopOperation) {
+        String fruit = shopOperation.getFruit();
+        int quantity = Storage.fruitsStorage.get(fruit);


Consider adding a check to handle cases where the fruit is not present in the fruitsStorage. This will prevent potential NullPointerException if a purchase operation is attempted for a fruit not in storage.

mateacademy-ai-mentor · 2024-11-02T18:04:04Z

src/main/java/core/basesyntax/strategy/PurchaseOperation.java

+            throw new OperationException("Operation is impossible, not enough quantity of: "
+                + fruit);


Ensure that the OperationException class is defined in your project. If not, you need to create this custom exception class to handle operation-related errors appropriately.

mateacademy-ai-mentor · 2024-11-02T18:04:05Z

src/main/java/core/basesyntax/strategy/ReturnOperation.java

+    @Override
+    public void handle(ShopOperation shopOperation) {
+        String fruit = shopOperation.getFruit();
+        int quantity = Storage.fruitsStorage.get(fruit);


Consider adding a check to handle cases where the fruit is not present in the fruitsStorage. This will prevent potential NullPointerException if a return operation is attempted for a fruit not in storage.

mateacademy-ai-mentor · 2024-11-02T18:04:05Z

src/main/java/core/basesyntax/strategy/SupplyOperation.java

+    @Override
+    public void handle(ShopOperation shopOperation) {
+        String fruit = shopOperation.getFruit();
+        int quantity = Storage.fruitsStorage.get(fruit);


Consider adding a check to handle cases where the fruit is not present in the fruitsStorage. This will prevent potential NullPointerException if a supply operation is attempted for a fruit not in storage.

added if block to method get() in StrategyOperationServiceImpl

mateacademy-ai-mentor

Your code demonstrates a solid understanding of the task requirements and adheres to many best practices. However, there are a few areas for improvement:

Dependency Inversion Principle: Consider using an interface for the operationHandlerMap to better adhere to this principle.
Exception Handling: Ensure that custom exceptions like FileException and OperationException are correctly defined and imported. Include original exceptions as causes for better context.
Null and Index Checks: Avoid catching NullPointerException for control flow. Validate inputs to prevent exceptions like IndexOutOfBoundsException and NumberFormatException.
Encapsulation: Avoid exposing internal storage maps directly. Provide methods to manipulate data safely.

These improvements will enhance the robustness and maintainability of your code. Keep up the good work and continue learning from these insights! 🚀

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

mateacademy-ai-mentor · 2024-11-02T19:04:02Z

src/main/java/core/basesyntax/db/Storage.java

+    public Map<String, Integer> getFruitsStorage() {
+        return fruitsStorage;


Exposing the internal storage map directly through getFruitsStorage can lead to unintended modifications from outside the class. Consider providing specific methods for adding, removing, or updating entries in the storage to maintain better control over the data.

mateacademy-ai-mentor · 2024-11-02T19:04:03Z

src/main/java/core/basesyntax/file/FileReaderImpl.java

@@ -0,0 +1,20 @@
+package core.basesyntax.file;
+
+import exception.FileException;


Ensure that the FileException class is correctly defined and imported. If it is not present in the project, you need to create it to handle file-related exceptions properly.

mateacademy-ai-mentor · 2024-11-02T19:04:03Z

src/main/java/core/basesyntax/file/FileReaderImpl.java

+        try {
+            reportList = Files.readAllLines(Path.of(filePath));
+        } catch (IOException e) {
+            throw new FileException("Can't read data from file: " + filePath);


Consider including the original exception e as a cause in the FileException to provide more context about the error. For example: throw new FileException("Can't read data from file: " + filePath, e);

mateacademy-ai-mentor · 2024-11-02T19:04:03Z

src/main/java/core/basesyntax/file/FileWriterImpl.java

@@ -0,0 +1,17 @@
+package core.basesyntax.file;
+
+import exception.FileException;


Ensure that the FileException class is correctly defined and imported. If it is not present in the project, you need to create it to handle file-related exceptions properly.

mateacademy-ai-mentor · 2024-11-02T19:04:03Z

src/main/java/core/basesyntax/file/FileWriterImpl.java

+        try {
+            Files.writeString(Path.of(filePath), report);
+        } catch (IOException e) {
+            throw new FileException("Can't write data to file: " + filePath);


Consider including the original exception e as a cause in the FileException to provide more context about the error. For example: throw new FileException("Can't write data to file: " + filePath, e);

mateacademy-ai-mentor · 2024-11-02T19:04:03Z

src/main/java/core/basesyntax/service/impl/StrategyOperationServiceImpl.java

+import java.util.Map;
+
+public class StrategyOperationServiceImpl implements StrategyOperationService {
+    private Map<OperationType, OperationHandler> operationHandlerMap;


Consider making operationHandlerMap final to indicate that it should not be reassigned after initialization. This can help prevent accidental modifications to the map reference.

mateacademy-ai-mentor · 2024-11-02T19:04:03Z

src/main/java/core/basesyntax/service/impl/StrategyOperationServiceImpl.java

+
+    @Override
+    public OperationHandler get(OperationType operationType) {
+        if (operationHandlerMap.get(operationType) != null) {


You can simplify the check by using operationHandlerMap.containsKey(operationType) to verify if the map contains the key before retrieving the handler.

mateacademy-ai-mentor · 2024-11-02T19:04:04Z

src/main/java/core/basesyntax/strategy/PurchaseOperation.java

+        } catch (NullPointerException e) {
+            throw new OperationException("Operation is not correct: " + shopOperation);
+        }


Catching NullPointerException is generally not a best practice for control flow. Instead, consider checking if the fruit exists in the storage before attempting to retrieve its quantity. This can prevent the exception from occurring in the first place.

mateacademy-ai-mentor · 2024-11-02T19:04:04Z

src/main/java/core/basesyntax/strategy/ReturnOperation.java

+        } catch (NullPointerException e) {
+            throw new OperationException("Operation is not correct: " + shopOperation);
+        }


Catching NullPointerException is generally not a best practice for control flow. Instead, consider checking if the fruit exists in the storage before attempting to retrieve its quantity. This can prevent the exception from occurring in the first place.

mateacademy-ai-mentor · 2024-11-02T19:04:04Z

src/main/java/core/basesyntax/strategy/SupplyOperation.java

+        } catch (NullPointerException e) {
+            throw new OperationException("Operation is not correct: " + shopOperation);
+        }


Catching NullPointerException is generally not a best practice for control flow. Instead, consider checking if the fruit exists in the storage before attempting to retrieve its quantity. This can prevent the exception from occurring in the first place.

liliia-ponomarenko

Good job! Let’s improve your solution ;)

liliia-ponomarenko · 2024-11-04T08:26:49Z

src/main/java/core/basesyntax/db/Storage.java

+public class Storage {
+    public static final Map<String, Integer> fruitsStorage = new HashMap<>();
+
+    public Map<String, Integer> getFruitsStorage() {


You can remove this method because your map is public ;)

liliia-ponomarenko · 2024-11-04T08:28:13Z

src/main/java/core/basesyntax/file/FileReaderImpl.java

+        try {
+            reportList = Files.readAllLines(Path.of(filePath));
+        } catch (IOException e) {
+            throw new FileException("Can't read data from file: " + filePath);


Suggested change

throw new FileException("Can't read data from file: " + filePath);

throw new FileException("Can't read data from file: " + filePath, e);

liliia-ponomarenko · 2024-11-04T08:28:29Z

src/main/java/core/basesyntax/file/FileWriterImpl.java

+        try {
+            Files.writeString(Path.of(filePath), report);
+        } catch (IOException e) {
+            throw new FileException("Can't write data to file: " + filePath);


liliia-ponomarenko · 2024-11-04T08:34:10Z

src/main/java/core/basesyntax/service/impl/StrategyOperationServiceImpl.java

+        if (operationHandlerMap.get(operationType) != null) {
+            return operationHandlerMap.get(operationType);
+        }
+        throw new OperationException("Operation type is not correct: " + operationType);


Suggested change

if (operationHandlerMap.get(operationType) != null) {

return operationHandlerMap.get(operationType);

}

throw new OperationException("Operation type is not correct: " + operationType);

OperationHandler handler = operationHandlerMap.get(operationType)

if (handlerMap != null) {

return handler;

}

throw new OperationException("Operation type is not correct: " + operationType);

Correct this method and get rid of the redundant try-catch blocks in your handlers

liliia-ponomarenko · 2024-11-04T08:35:25Z

src/main/java/core/basesyntax/strategy/PurchaseOperation.java

+        } catch (NullPointerException e) {
+            throw new OperationException("Operation is not correct: " + shopOperation);
+        }


remove try-catch block here

liliia-ponomarenko · 2024-11-04T08:35:30Z

src/main/java/core/basesyntax/strategy/PurchaseOperation.java

+        } catch (NullPointerException e) {
+            throw new OperationException("Operation is not correct: " + shopOperation);
+        }
+


Suggested change

liliia-ponomarenko · 2024-11-04T08:35:46Z

src/main/java/core/basesyntax/strategy/ReturnOperation.java

+        } catch (NullPointerException e) {
+            throw new OperationException("Operation is not correct: " + shopOperation);
+        }


liliia-ponomarenko · 2024-11-04T08:36:00Z

src/main/java/core/basesyntax/strategy/SupplyOperation.java

+        } catch (NullPointerException e) {
+            throw new OperationException("Operation is not correct: " + shopOperation);
+        }


…if the key is in the Storage to prevent NPE. Is this check necessary or unnecessary?

ovodolazov added 2 commits November 2, 2024 19:50

Fruit shop solution

11f9324

place files into src/main/resources

5fe8301

mateacademy-ai-mentor suggested changes Nov 2, 2024

View reviewed changes

ovodolazov added 2 commits November 2, 2024 20:55

added try/catch block to handle() methods,

74adf83

added if block to method get() in StrategyOperationServiceImpl

fixed checkstyle

a765ebb

ovodolazov requested a review from mateacademy-ai-mentor November 2, 2024 19:00

mateacademy-ai-mentor approved these changes Nov 2, 2024

View reviewed changes

liliia-ponomarenko suggested changes Nov 4, 2024

View reviewed changes

added fixes in exceptions. Remove try/catch blocks but added a check …

7265e20

…if the key is in the Storage to prevent NPE. Is this check necessary or unnecessary?

ovodolazov requested a review from liliia-ponomarenko November 4, 2024 10:34

Elena-Bruyako approved these changes Nov 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hw 1 solution #1233

Hw 1 solution #1233

ovodolazov commented Nov 2, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

mateacademy-ai-mentor Nov 2, 2024

liliia-ponomarenko left a comment

liliia-ponomarenko Nov 4, 2024

liliia-ponomarenko Nov 4, 2024

liliia-ponomarenko Nov 4, 2024

liliia-ponomarenko Nov 4, 2024

liliia-ponomarenko Nov 4, 2024

liliia-ponomarenko Nov 4, 2024

liliia-ponomarenko Nov 4, 2024

liliia-ponomarenko Nov 4, 2024

		public Map<String, Integer> getFruitsStorage() {
		return fruitsStorage;

		public OperationHandler get(OperationType operationType) {
		return operationHandlerMap.get(operationType);

		throw new OperationException("Operation is impossible, not enough quantity of: "
		+ fruit);

		@@ -0,0 +1,20 @@
		package core.basesyntax.file;

		import exception.FileException;

		@@ -0,0 +1,17 @@
		package core.basesyntax.file;

		import exception.FileException;

	throw new FileException("Can't read data from file: " + filePath);
	throw new FileException("Can't read data from file: " + filePath, e);

Hw 1 solution #1233

Are you sure you want to change the base?

Hw 1 solution #1233

Conversation

ovodolazov commented Nov 2, 2024

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liliia-ponomarenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment