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

This task was hard #1282

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

Conversation

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


Thank you for your hard work on this project! 😊 While you've made great progress, there are a few critical issues that need to be addressed before we can approve the pull request:

  1. Package Naming and Structure: There are multiple typos in the package names and import statements, such as core.basesyntax.tranasctions instead of core.basesyntax.transactions. These need to be corrected to ensure the code compiles and functions correctly.

  2. Data Conversion Logic: In DataConverterImpl, the logic for parsing the quantity is incorrect. The NUMBER constant is being used as a string instead of parsing the actual value from the input data. This needs to be fixed to ensure accurate data conversion.

  3. SOLID Principles: The DateFruits class is directly accessed by multiple classes, which violates the Dependency Inversion Principle. Consider refactoring to improve adherence to SOLID principles.

  4. Exception Handling: The exception handling in FileReaderImpl and FileWriterImpl could be more informative. Providing detailed error messages will help in debugging and maintaining the code.

  5. Purchase Operation Handling: The PurchaseOperation class should check if the resulting balance is positive before saving it in the storage. A RuntimeException should be thrown if the balance is negative.

Once these issues are resolved, your code will be in great shape! Keep up the good work, and don't hesitate to reach out if you need any help. 🚀

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

@@ -0,0 +1,9 @@
package core.basesyntax.DataService;

import core.basesyntax.tranasctions.FruitTransaction;

Choose a reason for hiding this comment

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

There is a typo in the import statement. It should be core.basesyntax.transactions.FruitTransaction instead of core.basesyntax.tranasctions.FruitTransaction. Please correct the package name to ensure the import works correctly.

@@ -0,0 +1,27 @@
package core.basesyntax.DataService;

import core.basesyntax.tranasctions.FruitTransaction;

Choose a reason for hiding this comment

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

There is a typo in the import statement. It should be core.basesyntax.transactions.FruitTransaction instead of core.basesyntax.tranasctions.FruitTransaction. Please correct the package name to ensure the import works correctly.

.forEach(strings -> {
FruitTransaction fruitTransaction = new FruitTransaction();
fruitTransaction.setFruit(strings[FRUIT_TYPE]);
fruitTransaction.setQuantity(Integer.parseInt(String.valueOf(NUMBER)));

Choose a reason for hiding this comment

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

The setQuantity method is incorrectly using String.valueOf(NUMBER). This should be Integer.parseInt(strings[NUMBER]) to correctly parse the quantity from the data.

@@ -0,0 +1,10 @@
package core.basesyntax.DataService;

import core.basesyntax.tranasctions.FruitTransaction;

Choose a reason for hiding this comment

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

There is a typo in the import statement. It should be core.basesyntax.transactions.FruitTransaction instead of core.basesyntax.tranasctions.FruitTransaction. Please correct the package name to ensure the import works correctly.

package core.basesyntax.DataService;

import core.basesyntax.tranasctions.FruitTransaction;
import core.basesyntax.tranasctions.OperationStrategy;

Choose a reason for hiding this comment

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

There is a typo in the import statement. It should be core.basesyntax.transactions.OperationStrategy instead of core.basesyntax.tranasctions.OperationStrategy. Please correct the package name to ensure the import works correctly.

@@ -0,0 +1,5 @@
package core.basesyntax.tranasctions;

Choose a reason for hiding this comment

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

There is a typo in the package declaration. It should be package core.basesyntax.transactions; instead of package core.basesyntax.tranasctions;. Please correct the package name to ensure consistency and proper functionality.

@@ -0,0 +1,15 @@
package core.basesyntax.tranasctions;

Choose a reason for hiding this comment

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

There is a typo in the package declaration. It should be package core.basesyntax.transactions; instead of package core.basesyntax.tranasctions;. Please correct the package name to ensure consistency and proper functionality.

@@ -0,0 +1,12 @@
package core.basesyntax.tranasctions;

Choose a reason for hiding this comment

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

There is a typo in the package declaration. It should be package core.basesyntax.transactions; instead of package core.basesyntax.tranasctions;. Please correct the package name to ensure consistency and proper functionality.

@@ -0,0 +1,12 @@
package core.basesyntax.tranasctions;

Choose a reason for hiding this comment

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

There is a typo in the package declaration. It should be package core.basesyntax.transactions; instead of package core.basesyntax.tranasctions;. Please correct the package name to ensure consistency and proper functionality.

@@ -0,0 +1,12 @@
package core.basesyntax.tranasctions;

Choose a reason for hiding this comment

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

There is a typo in the package declaration. It should be package core.basesyntax.transactions; instead of package core.basesyntax.tranasctions;. Please correct the package name to ensure consistency and proper functionality.

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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

Something went wrong while processing the review.
[CODE: 3]

If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.

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.

Thank you for your submission! 🎉 While your code has been approved to help you move forward with your learning, there are several important issues that need to be addressed:

  1. Package Naming: The package name fileservise is misspelled and should be corrected to fileservice to adhere to naming conventions.

  2. SOLID Principles: Direct access to DateFruits violates the Dependency Inversion Principle. Consider using an abstraction or interface for accessing the storage to improve adherence to SOLID principles.

  3. Exception Handling: In FileReaderImpl and FileWriterImpl, exceptions are rethrown as RuntimeException without informative messages. Enhance exception handling to provide more context.

  4. Purchase Operation: The PurchaseOperation class lacks a check to ensure the resulting balance is not negative. Implement a check and throw a RuntimeException if the balance is negative.

  5. Operation Handling: Consider adding a check in OperationStrategyImpl to handle cases where the operation is not found in the map, preventing potential NullPointerExceptions.

  6. Business Logic in Main Method: The HelloWorld.java file contains business logic in the main method. Refactor this logic into separate methods or classes for better readability and maintainability.

Please review these comments and make the necessary improvements 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! ✨

import core.basesyntax.dataservice.DataConverterImpl;
import core.basesyntax.dataservice.ShopService;
import core.basesyntax.dataservice.ShopServiceImpl;
import core.basesyntax.fileservise.FileReader;

Choose a reason for hiding this comment

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

The package name fileservise is misspelled. It should be fileservice to match the correct naming convention.

import core.basesyntax.dataservice.ShopService;
import core.basesyntax.dataservice.ShopServiceImpl;
import core.basesyntax.fileservise.FileReader;
import core.basesyntax.fileservise.FileReaderImpl;

Choose a reason for hiding this comment

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

The package name fileservise is misspelled. It should be fileservice to match the correct naming convention.

import core.basesyntax.dataservice.ShopServiceImpl;
import core.basesyntax.fileservise.FileReader;
import core.basesyntax.fileservise.FileReaderImpl;
import core.basesyntax.fileservise.FileWriter;

Choose a reason for hiding this comment

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

The package name fileservise is misspelled. It should be fileservice to match the correct naming convention.

import core.basesyntax.fileservise.FileReader;
import core.basesyntax.fileservise.FileReaderImpl;
import core.basesyntax.fileservise.FileWriter;
import core.basesyntax.fileservise.FileWriterImpl;

Choose a reason for hiding this comment

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

The package name fileservise is misspelled. It should be fileservice to match the correct naming convention.

Comment on lines 32 to 55
public static void main(String[] args) {

FileReader fileReader = new FileReaderImpl();
List<String> inputReport = fileReader.readFile("reportToRead.csv");

DataConverter dataConverter = new DataConverterImpl();
List<FruitTransaction> transactions = dataConverter.convertToTransaction(inputReport);

Map<FruitTransaction.Operation, OperationHandler> operationHandlers = new HashMap<>();
operationHandlers.put(FruitTransaction.Operation.BALANCE, new BalanceOperation());
operationHandlers.put(FruitTransaction.Operation.PURCHASE, new PurchaseOperation());
operationHandlers.put(FruitTransaction.Operation.RETURN, new ReturnOperation());
operationHandlers.put(FruitTransaction.Operation.SUPPLY, new SupplyOperation());
OperationStrategy operationStrategy = new OperationStrategyImpl(operationHandlers);

ShopService shopService = new ShopServiceImpl(operationStrategy);
shopService.process(transactions);

ReportGenerator reportGenerator = new ReportGeneratorImpl();
String resultingReport = reportGenerator.getReport();

FileWriter fileWriter = new FileWriterImpl();
fileWriter.writeFile(resultingReport, "finalReport.csv");
}

Choose a reason for hiding this comment

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

The main method contains business logic, which is not recommended. Consider refactoring this logic into separate methods or classes to improve code readability and maintainability.

@@ -0,0 +1,13 @@
package core.basesyntax.reportservice;

import core.basesyntax.storage.DateFruits;

Choose a reason for hiding this comment

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

Direct access to DateFruits may violate the Dependency Inversion Principle. Consider using an abstraction or interface to access the storage, which would improve adherence to SOLID principles.

return fruitsStorage.get(name) == null ? 0 : fruitsStorage.get(name);
}

public static Map<String, Integer> getAll() {

Choose a reason for hiding this comment

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

The method getAll() returns a copy of the storage map, which is good for encapsulation. However, consider using an interface or abstraction for accessing the storage to adhere to the Dependency Inversion Principle.

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

import core.basesyntax.storage.DateFruits;

Choose a reason for hiding this comment

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

Direct access to DateFruits may violate the Dependency Inversion Principle. Consider using an abstraction or interface to access the storage, which would improve adherence to SOLID principles.


@Override
public OperationHandler getOperation(FruitTransaction.Operation operation) {
return operationHandlerMap.get(operation);

Choose a reason for hiding this comment

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

Consider adding a check to handle cases where the operation is not found in the operationHandlerMap. This could prevent potential NullPointerExceptions and improve robustness.

@Override
public void resultOfOperation(String fruitName, int amount) {
int currentAmount = DateFruits.get(fruitName);
int newAmount = currentAmount - amount;

Choose a reason for hiding this comment

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

Before saving the new amount, ensure that newAmount is not negative. If it is, throw a RuntimeException to prevent the storage from having a negative balance, as per the task requirements.

Copy link

@liliia-ponomarenko liliia-ponomarenko left a comment

Choose a reason for hiding this comment

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

Firstly let’s fix the build ;) If you have any issues let's discuss them in the chat on the platform

Comment on lines 37 to 49
DataConverter dataConverter = new DataConverterImpl();
List<FruitTransaction> transactions = dataConverter.convertToTransaction(inputReport);

Map<FruitTransaction.Operation, OperationHandler> operationHandlers = new HashMap<>();
operationHandlers.put(FruitTransaction.Operation.BALANCE, new BalanceOperation());
operationHandlers.put(FruitTransaction.Operation.PURCHASE, new PurchaseOperation());
operationHandlers.put(FruitTransaction.Operation.RETURN, new ReturnOperation());
operationHandlers.put(FruitTransaction.Operation.SUPPLY, new SupplyOperation());
OperationStrategy operationStrategy = new OperationStrategyImpl(operationHandlers);

ShopService shopService = new ShopServiceImpl(operationStrategy);
shopService.process(transactions);

Choose a reason for hiding this comment

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

Suggested change
DataConverter dataConverter = new DataConverterImpl();
List<FruitTransaction> transactions = dataConverter.convertToTransaction(inputReport);
Map<FruitTransaction.Operation, OperationHandler> operationHandlers = new HashMap<>();
operationHandlers.put(FruitTransaction.Operation.BALANCE, new BalanceOperation());
operationHandlers.put(FruitTransaction.Operation.PURCHASE, new PurchaseOperation());
operationHandlers.put(FruitTransaction.Operation.RETURN, new ReturnOperation());
operationHandlers.put(FruitTransaction.Operation.SUPPLY, new SupplyOperation());
OperationStrategy operationStrategy = new OperationStrategyImpl(operationHandlers);
ShopService shopService = new ShopServiceImpl(operationStrategy);
shopService.process(transactions);
Map<FruitTransaction.Operation, OperationHandler> operationHandlers = new HashMap<>();
operationHandlers.put(FruitTransaction.Operation.BALANCE, new BalanceOperation());
operationHandlers.put(FruitTransaction.Operation.PURCHASE, new PurchaseOperation());
operationHandlers.put(FruitTransaction.Operation.RETURN, new ReturnOperation());
operationHandlers.put(FruitTransaction.Operation.SUPPLY, new SupplyOperation());
OperationStrategy operationStrategy = new OperationStrategyImpl(operationHandlers);
DataConverter dataConverter = new DataConverterImpl();
List<FruitTransaction> transactions = dataConverter.convertToTransaction(inputReport);
ShopService shopService = new ShopServiceImpl(operationStrategy);
shopService.process(transactions);

Or move operationHandlers initialization to the private method

Copy link

@liliia-ponomarenko liliia-ponomarenko left a comment

Choose a reason for hiding this comment

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

Good job! Let’s improve your solution ;)

finalReport.csv Outdated
Comment on lines 1 to 3
fruit,quantity
apple,90
banana,152

Choose a reason for hiding this comment

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

move all files to the resources directory

Choose a reason for hiding this comment

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

fix comment

pom.xml Outdated
Comment on lines 21 to 26
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter</artifactId>
<version>RELEASE</version>
<scope>test</scope>
</dependency>

Choose a reason for hiding this comment

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

you don’t need this dependency for this HW

Comment on lines 29 to 32
// HINT: In the `public static void main(String[] args)`
// it is better to create instances of your classes,
// and call their methods, but do not write any business
// logic in the `main` method!

Choose a reason for hiding this comment

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

Suggested change
// HINT: In the `public static void main(String[] args)`
// it is better to create instances of your classes,
// and call their methods, but do not write any business
// logic in the `main` method!

remove redundant brackets

// logic in the `main` method!
public static void main(String[] args) {
FileReader fileReader = new FileReaderImpl();
List<String> inputReport = fileReader.readFile("reportToRead.csv");

Choose a reason for hiding this comment

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

move the file path to the constant fields

String resultingReport = reportGenerator.getReport();

FileWriter fileWriter = new FileWriterImpl();
fileWriter.writeFile(resultingReport, "finalReport.csv");

Choose a reason for hiding this comment

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

the same

} catch (IOException e) {
throw new RuntimeException(e);
}

Choose a reason for hiding this comment

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

Suggested change

Comment on lines 9 to 11
return "fruit,quantity" + "\n" + DateFruits.getAll().entrySet().stream()
.map(element -> element.getKey() + "," + element.getValue())
.collect(Collectors.joining("\n"));

Choose a reason for hiding this comment

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

Create constants for literals. Use System.lineseparator()
What is bad with this type of concatenation?

Comment on lines 8 to 15
if (amount > 0) {
int currentAmount = DateFruits.get(fruitName);
int newAmount = currentAmount + amount;
DateFruits.save(fruitName, newAmount);

} else {
throw new RuntimeException(" The balance can`t be less or equals zero");
}

Choose a reason for hiding this comment

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

better avoid if-else construction

Suggested change
if (amount > 0) {
int currentAmount = DateFruits.get(fruitName);
int newAmount = currentAmount + amount;
DateFruits.save(fruitName, newAmount);
} else {
throw new RuntimeException(" The balance can`t be less or equals zero");
}
if (amount <= 0) {
throw new RuntimeException(" The balance can`t be less or equals zero");
}
int currentAmount = DateFruits.get(fruitName);
int newAmount = currentAmount + amount;
DateFruits.save(fruitName, newAmount);

fix it for other handlers

private String fruit;
private int quantity;

// getters, setters, ...

Choose a reason for hiding this comment

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

Suggested change
// getters, setters, ...

Comment on lines 15 to 19
if (operationHandlerMap.get(operation) != null) {
return operationHandlerMap.get(operation);
} else {
throw new RuntimeException("Operation can`t be null");
}

Choose a reason for hiding this comment

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

the same. provide a parameter in the exception message

finalReport.csv Outdated
Comment on lines 1 to 3
fruit,quantity
apple,90
banana,152

Choose a reason for hiding this comment

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

fix comment

reportToRead.csv Outdated
@@ -0,0 +1,8 @@
b,banana,20

Choose a reason for hiding this comment

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

move ro resources folder

= new BufferedWriter(new java.io.FileWriter(nameOfNewFile))) {
bufferedWriter.write(report);
} catch (IOException e) {
throw new RuntimeException(e);

Choose a reason for hiding this comment

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

fix comment


@Override
public String getReport() {
return FRUIT + "," + QUANTITY + System.lineSeparator()

Choose a reason for hiding this comment

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

it's better to use StringBuilder

Copy link

@JJJazl JJJazl left a comment

Choose a reason for hiding this comment

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

Left some comments, but I think you can move on. Please, fix them all before your next task.

import java.util.HashMap;
import java.util.Map;

public class DateFruits {
Copy link

Choose a reason for hiding this comment

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

Suggested change
public class DateFruits {
public class Storage {

@Override
public void resultOfOperation(String fruitName, int amount) {
if (amount <= 0) {
throw new RuntimeException(" The balance can`t be less or equals zero");
Copy link

Choose a reason for hiding this comment

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

Suggested change
throw new RuntimeException(" The balance can`t be less or equals zero");
throw new IllegalArgumentException(" The balance can`t be less or equals zero");

.forEach(strings -> {
FruitTransaction fruitTransaction = new FruitTransaction();
fruitTransaction.setFruit(strings[FRUIT_TYPE]);
fruitTransaction.setQuantity(Integer.parseInt(strings[NUMBER]));
Copy link

Choose a reason for hiding this comment

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

What happens if quantity in not a number? Also we want to check if it's not negative before processing any transaction. It's called fail fast principle. It's better to use for loop and wrap this line in try catch block to throw an IllegalArgumentException

return Arrays.stream(Operation.values())
.filter(e -> e.getCode().equals(code))
.findFirst()
.orElseThrow(() -> new RuntimeException(("Operation with code [%s] "
Copy link

Choose a reason for hiding this comment

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

Suggested change
.orElseThrow(() -> new RuntimeException(("Operation with code [%s] "
.orElseThrow(() -> new IllegalArgumentException(("Operation with code [%s] "

@Override
public OperationHandler getOperation(FruitTransaction.Operation operation) {
if (operationHandlerMap.get(operation) == null) {
throw new RuntimeException("Operation can`t be null");
Copy link

Choose a reason for hiding this comment

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

Suggested change
throw new RuntimeException("Operation can`t be null");
throw new IllegalArgumentException("Operation can`t be null");


public class SupplyOperation implements OperationHandler {
@Override
public void resultOfOperation(String fruitName, int amount) {
Copy link

Choose a reason for hiding this comment

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

What happens if amount is negative?

Comment on lines +8 to +10
if (amount <= 0) {
throw new RuntimeException(" The balance can`t be less or equals zero");
}
Copy link

Choose a reason for hiding this comment

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

Let's move it to separate static or default method in OperationHandler and reuse where we need


public class ReturnOperation implements OperationHandler {
@Override
public void resultOfOperation(String fruitName, int amount) {
Copy link

Choose a reason for hiding this comment

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

What happens if amount is negative?

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.

5 participants