-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Develop #1289
base: master
Are you sure you want to change the base?
Develop #1289
Conversation
There was a problem hiding this 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 a good start, there are several critical issues that need to be addressed to meet the core requirements of the task. 🚀
-
Uninitialized Variables and Imports: The
Main.java
file has variables likeshopService
andtransactions
that are used without being declared or initialized, which will cause compilation errors. Additionally, classes likeReportGeneratorImpl
andFileWriterImpl
are used without proper imports or declarations. -
Interface Implementations: Classes such as
FruitsDaoImpl
andInsertListImpl
are currently empty and do not implement their respective interfaces. Ensure these classes implement all required methods and provide necessary logic. -
Constructor and Initialization Issues: The
FruitServiceImpl
class has issues with the initialization offruitUseServiceMap
, and theFruit
class constructor only initializes theamount
field. These need to be addressed for proper functionality. -
Project Structure and Redundancies: There are redundant method declarations and typos in class names and parameter names that need correction for clarity and consistency.
-
Core Functionality: The code lacks implementations for reading from and writing to CSV files, which are essential for the task. Additionally, exception handling and null checks are necessary to prevent potential runtime errors.
Please address these issues to ensure the code meets the task requirements. Keep up the great effort, and don't hesitate to reach out if you need further guidance! 😊
✨ 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.
|
||
// 4. Process the incoming transactions with applicable OperationHandler implementations | ||
|
||
shopService.process(transactions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable shopService
is used here but is not declared or initialized anywhere in the code. You need to declare and initialize shopService
before using it.
|
||
// 4. Process the incoming transactions with applicable OperationHandler implementations | ||
|
||
shopService.process(transactions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable transactions
is used here but is not declared or initialized anywhere in the code. You need to declare and initialize transactions
before using it.
shopService.process(transactions); | ||
|
||
// 5.Generate report based on the current Storage state | ||
ReportGenerator reportGenerator = new ReportGeneratorImpl(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class ReportGeneratorImpl
is used here but is not imported or declared anywhere in the code. Ensure that ReportGeneratorImpl
is properly imported or declared.
String resultingReport = reportGenerator.getReport(); | ||
|
||
// 6. Write the received report into the destination file | ||
FileWriter fileWriter = new FileWriterImpl(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class FileWriterImpl
is used here but is not imported or declared anywhere in the code. Ensure that FileWriterImpl
is properly imported or declared.
import java.util.List; | ||
|
||
public interface FruitsDao { | ||
void setFruits(String fruitName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setFruits
method currently only accepts a String
parameter for the fruit name. Consider whether additional parameters, such as quantity or a fruit object, are needed to effectively set fruits in the database.
if (amount > fruitRepository.getFruit(fruitName).getAmount()) { | ||
throw new RuntimeException("As much product is not available"); | ||
} | ||
fruitRepository.getFruit(fruitName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method fruitRepository.getFruit(fruitName)
is called multiple times. Consider storing the result in a variable to avoid redundant method calls and improve performance.
import core.basesyntax.infratructure.persistence.FruitRepository; | ||
|
||
public interface ReturnFruitToBalance extends FruitUseService { | ||
void run(String fruitName, int amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The run
method is already defined in the FruitUseService
interface. Redefining it here without any changes or additional documentation is redundant. Consider removing this method declaration.
|
||
@Override | ||
public void run(String fruitName, int amount) { | ||
fruitRepository.getFruit(fruitName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method fruitRepository.getFruit(fruitName)
is called multiple times. Consider storing the result in a variable to avoid redundant method calls and improve performance.
package core.basesyntax.service.usecases; | ||
|
||
public interface SupplyFruitToBalance extends FruitUseService { | ||
void run(String fruitName, int amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The run
method is already defined in the FruitUseService
interface. Redefining it here without any changes or additional documentation is redundant. Consider removing this method declaration.
|
||
@Override | ||
public void run(String fruitName, int amount) { | ||
fruitRepository.getFruit(fruitName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method fruitRepository.getFruit(fruitName)
is called multiple times. Consider storing the result in a variable to avoid redundant method calls and improve performance.
There was a problem hiding this 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 core functionality of the code! 🎉 While there are several areas for improvement, such as enhancing exception messages for clarity and correcting typos in class and interface names, the decision to approve this pull request is to help you move forward with your learning journey.
Key Points to Address:
-
Exception Handling: Improve the specificity of exception messages. For example, include details like the fruit name or operation that failed to make debugging easier.
-
Typographical Errors: Correct typos in class and interface names, such as
PushareFruitImpl
toPurchaseFruitImpl
andFruitUseReposetory
toFruitUseRepository
, to maintain consistency and clarity. -
Method Redundancies: Avoid redefining methods in interfaces when they are already inherited, such as the
run
method in interfaces extendingFruitUseService
. -
Error Handling: Implement checks and error handling for potential exceptions like
NullPointerException
,NumberFormatException
, andArrayIndexOutOfBoundsException
. -
Resource Management: Use try-with-resources for file operations to ensure proper resource management and prevent leaks.
By addressing these issues, you can enhance the robustness and maintainability of your code. 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! ✨
try { | ||
allData = fruitsDao.getFruits(); | ||
} catch (IOException e) { | ||
throw new RuntimeException("Can't read from DB"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception handling here is too generic. Consider providing more detailed information in the exception message to aid in debugging.
fruitUseServiceMap | ||
.put(Operation.SUPPLY, new SupplyFruitToBalanceImpl(fruitRepository)); | ||
fruitUseServiceMap | ||
.put(Operation.PURCHASE, new PushareFruitImpl(fruitRepository)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a typo in the class name PushareFruitImpl
. It should likely be PurchaseFruitImpl
. Verify the class name and ensure it is correctly implemented.
try { | ||
fruitsDao.setFruits(fruitService.getFruitRepository().getFruitMap()); | ||
} catch (IOException e) { | ||
throw new RuntimeException("Can't write into DB"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the previous exception handling, provide more detailed information in the exception message to aid in debugging.
.map(fruit -> fruit.getName() + "," + fruit.getAmount()) | ||
.toList(); | ||
|
||
BufferedWriter br = new BufferedWriter(new FileWriter(FILE_NAME)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a try-with-resources statement for BufferedWriter
to ensure that the resource is closed automatically, even if an exception occurs. This will improve resource management and prevent potential resource leaks.
|
||
@Override | ||
public void writeInfo(Operation operation, String fruitName, int amount) throws IOException { | ||
BufferedWriter br = new BufferedWriter(new FileWriter(FILE_NAME, true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a try-with-resources statement for BufferedWriter
to ensure that the resource is closed automatically, even if an exception occurs. This will improve resource management and prevent potential resource leaks.
import core.basesyntax.service.Operation; | ||
import java.io.IOException; | ||
|
||
public class ReturnFruitToBalanceImpl extends FruitUseReposetory implements ReturnFruitToBalance { |
There was a problem hiding this comment.
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 superclass name FruitUseReposetory
. It should be FruitUseRepository
. Correct the spelling to maintain consistency and clarity.
try { | ||
super.getInsertListDao().writeInfo(Operation.RETURN, fruitName, amount); | ||
} catch (IOException e) { | ||
throw new RuntimeException("Can't insert to file"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception message 'Can't insert to file' could be more informative. Consider including details about the operation that failed.
package core.basesyntax.service.usecases; | ||
|
||
public interface SupplyFruitToBalance extends FruitUseService { | ||
void run(String fruitName, int amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redefining the run
method in this interface is unnecessary since it already inherits from FruitUseService
. Consider removing the method declaration here.
import core.basesyntax.service.Operation; | ||
import java.io.IOException; | ||
|
||
public class SupplyFruitToBalanceImpl extends FruitUseReposetory implements SupplyFruitToBalance { |
There was a problem hiding this comment.
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 superclass name FruitUseReposetory
. It should be FruitUseRepository
. Correct the spelling to maintain consistency and clarity.
try { | ||
super.getInsertListDao().writeInfo(Operation.SUPPLY, fruitName, amount); | ||
} catch (IOException e) { | ||
throw new RuntimeException("Can't insert to file"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception message 'Can't insert to file' could be more informative. Consider including details about the operation that failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s check task requirements! Compare the input and output file structures with your files. Let’s fix it! Check the example of the Main class, service names, and responsibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s improve your solution ;)
try { | ||
fileWriter.write(resultingReport); | ||
} catch (IOException e) { | ||
throw new RuntimeException("Can't write report into db"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catch exception in writer service. add file name parameter to the exception message
@Override | ||
public List<FruitTransaction> convertToTransaction(List<String> inputReport) { | ||
return inputReport.stream() | ||
.map(s -> s.split(",")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the separator to the constant fields
return inputReport.stream() | ||
.map(s -> s.split(",")) | ||
.map(s -> new FruitTransaction( | ||
FruitTransaction.getOperation(s[0]), s[1], Integer.parseInt(s[2]))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better validate data. check that quantity is number
} | ||
|
||
@Override | ||
public List<String> read() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pass file name as a method parameter. Add a parameter to the method message
@Override | ||
public void setFruits(Map<String, Fruit> fruitMap) throws IOException { | ||
List<String> data = fruitMap.entrySet().stream() | ||
.map(m -> m.getValue()) | ||
.map(fruit -> fruit.getName() + "," + fruit.getAmount()) | ||
.toList(); | ||
|
||
BufferedWriter br = new BufferedWriter(new FileWriter(FILE_NAME)); | ||
for (String str : data) { | ||
br.write(str + System.lineSeparator()); | ||
} | ||
br.close(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not reader logic
public class BalanceOperation implements OperationHandler { | ||
|
||
@Override | ||
public void run(FruitTransaction fruitTransaction, Map<String, Integer> fruitRepository) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same use Storage
throw new NoSuchElementException(); | ||
} | ||
fruitRepository.replace(fruitTransaction.getFruit(), | ||
fruitRepository.get(fruitTransaction.getFruit()) - fruitTransaction.getQuantity()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if fruit quantity isn’t enough?
@Override | ||
public void run(FruitTransaction fruitTransaction, Map<String, Integer> fruitRepository) { | ||
if (fruitRepository.get(fruitTransaction.getFruit()) == null) { | ||
throw new NoSuchElementException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add message
@Override | ||
public void run(FruitTransaction fruitTransaction, Map<String, Integer> fruitRepository) { | ||
if (fruitRepository.get(fruitTransaction.getFruit()) == null) { | ||
// throw new NoSuchElementException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove commented code and block without realization
public void run(FruitTransaction fruitTransaction, Map<String, Integer> fruitRepository) { | ||
if (fruitRepository.get(fruitTransaction.getFruit()) == null) { | ||
throw new NoSuchElementException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same comments
public static void main(String[] arg) { | ||
// 1. Read the data from the input CSV file | ||
FileReader fileReader = new FileReaderImpl(); | ||
List<String> inputReport = fileReader.read("src/main/java/operationslist.csv"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create constant for file path
|
||
public class Main { | ||
public static void main(String[] arg) { | ||
// 1. Read the data from the input CSV file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove comments
|
||
// 6. Write the received report into the destination file | ||
FileWriter fileWriter = new FileWriterImpl(); | ||
fileWriter.write(resultingReport, "src/main/java/database.csv"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
public List<FruitTransaction> convertToTransaction(List<String> inputReport) { | ||
return inputReport.stream() | ||
.map(s -> s.split(SEPARATOR)) | ||
.filter(s -> s.length == 3 && isNumeric(s[2])) // Перевіряємо довжину та чи є число |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove comments
public List<FruitTransaction> convertToTransaction(List<String> inputReport) { | ||
return inputReport.stream() | ||
.map(s -> s.split(SEPARATOR)) | ||
.filter(s -> s.length == 3 && isNumeric(s[2])) // Перевіряємо довжину та чи є число |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename s
create constants for all magic numbers
import java.util.Map; | ||
|
||
public class Main { | ||
private static final String OPERATION_LIST_FILE_PATH = "src/main/java/operationslist.csv"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
files should be stored in resources folder
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few comments
|
||
public class Main { | ||
private static final String OPERATION_LIST_FILE_PATH | ||
= "src/main/java/core/basesyntax/resources/operationslist.csv"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resources folder should be placed in src/main/resources
Integer.parseInt(str); | ||
return true; | ||
} catch (NumberFormatException e) { | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's throw an exception with message in this case
src/main/java/core/basesyntax/infrastructure/DataConverterImpl.java
Outdated
Show resolved
Hide resolved
if (Storage.STORAGE.get(fruitTransaction.getFruit()) == null) { | ||
throw new NoSuchElementException("Can't find fruit: " + fruitTransaction.getFruit()); | ||
} |
There was a problem hiding this comment.
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 the default or static method in OperationHandler to avoid code duplication
|
||
private boolean isNumeric(String str) { | ||
try { | ||
Integer.parseInt(str); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if quantity is negative?)
Let's rewrite this method, change return type to int and use it in map(), instead of filter()
Storage.STORAGE.replace(fruitTransaction.getFruit(), | ||
Storage.STORAGE.get(fruitTransaction.getFruit()) + fruitTransaction.getQuantity()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use put()
instead of replace()
. See everywhere
….java Co-authored-by: Safarov Renat <[email protected]>
Co-authored-by: Safarov Renat <[email protected]>
Co-authored-by: Safarov Renat <[email protected]>
No description provided.