add solution #1242
add solution #1242
Conversation
| Integer newQuantity = db.containsKey(fruit) | ||
| ? db.get(fruit) + quantity | ||
| : quantity; |
There was a problem hiding this comment.
map has native merge method to handle cases when key exists and doesn't exist
| @Override | ||
| public void remove(String fruit, int quantity) { | ||
| if (db.get(fruit) - quantity < 0) { | ||
| throw new RuntimeException("Not enough " + fruit + " in storage to remove"); |
There was a problem hiding this comment.
let's create a custom exception for such cases
| return operation; | ||
| } | ||
| } | ||
| return null; |
| import java.util.List; | ||
|
|
||
| public interface DataConverter { | ||
| List<FruitTransaction> convertStringsDataToFruitTransactions(List<String> stringsData); |
There was a problem hiding this comment.
let's make the method name concise and clear enough
| if (transaction.getQuantity() < 0) { | ||
| throw new RuntimeException("Quantity can't be less than 0"); | ||
| } |
There was a problem hiding this comment.
this isn't a good place for transaction validations. It's better to validate quantity before or during parsing phase
| if (repository.hasFruit(transaction.getFruit())) { | ||
| repository.remove(transaction.getFruit(), transaction.getQuantity()); | ||
| } else { | ||
| throw new RuntimeException("Can't return fruits that are/were not in storage"); |
There was a problem hiding this comment.
do not use the most general RuntimeException. Create your own one for such a case. Fix the same issue in all places
| private static final String PATH_TO_INPUT_FILE = "transactions.csv"; | ||
| private static final String PATH_TO_OUTPUT_FILE = "report.csv"; | ||
|
|
There was a problem hiding this comment.
these constants seem to be completely redundant
|
|
||
| @Override | ||
| public List<String> read(String filePath) { | ||
| File file = new File(filePath); |
| File file = new File(filePath); | ||
| file.delete(); |
|
|
||
| public class ReportGeneratorImpl implements ReportGenerator { | ||
| private static final String DATA_SPLITTER = ","; | ||
| private static final String REPORT_FIRST_LINE = "fruit,quantity\n"; |
There was a problem hiding this comment.
use System.lineSeparator(), fix in all places
| private static final Map<String, Integer> storage; | ||
|
|
||
| static { | ||
| storage = new HashMap<>(); | ||
| } | ||
|
|
||
| public static Map<String, Integer> getStorage() { | ||
| return storage; | ||
| } |
There was a problem hiding this comment.
| private static final Map<String, Integer> storage; | |
| static { | |
| storage = new HashMap<>(); | |
| } | |
| public static Map<String, Integer> getStorage() { | |
| return storage; | |
| } | |
| public static final Map<String, Integer> storage = new HashMap<>(); |
| import java.util.Map; | ||
|
|
||
| public class OperationStrategyImpl implements OperationStrategy { | ||
|
|
There was a problem hiding this comment.
remove empty line
|
|
||
| @Override | ||
| public List<String> read(String filePath) { | ||
| File file = new File(filePath); |
| StandardOpenOption.APPEND); | ||
|
|
||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
add informative message with parameters
| @Override | ||
| public List<String> read(String filePath) { | ||
| try { | ||
| return Files.readAllLines(new File(filePath).toPath()); |
There was a problem hiding this comment.
What is the reason to create a file, it is possible just read by path?
| if (arr.length != EXPECTED_ARRAY_LENGTH) { | ||
| throw new InvalidInputException("Invalid input"); | ||
| } | ||
| if (Integer.parseInt(arr[QUANTITY_INDEX]) < 0) { | ||
| throw new InvalidInputException("Quantity can't be less than 0"); | ||
| } |
There was a problem hiding this comment.
Better to move this logic to separate method to make code more clear
| int index = START_ROW_INDEX; | ||
| if (stringsData.get(index).equals(INPUT_POTENTIAL_FIRST_ROW)) { | ||
| index++; | ||
| } | ||
| for (; index < stringsData.size(); index++) { |
There was a problem hiding this comment.
Better to not overcomplicate, assume that input file is correct, and first row will be with index 0, so no reason to calculate index of header, just start iteration with index 1
| if (repository.hasFruit(transaction.getFruit())) { | ||
| repository.remove(transaction.getFruit(), transaction.getQuantity()); | ||
| } else { | ||
| throw new InvalidInputException("There are no " | ||
| + transaction.getFruit() | ||
| + " in storage"); | ||
| } |
There was a problem hiding this comment.
better avoid if-else construction
| if (repository.hasFruit(transaction.getFruit())) { | |
| repository.remove(transaction.getFruit(), transaction.getQuantity()); | |
| } else { | |
| throw new InvalidInputException("There are no " | |
| + transaction.getFruit() | |
| + " in storage"); | |
| } | |
| if (repository.hasFruit(transaction.getFruit())) { | |
| repository.remove(transaction.getFruit(), transaction.getQuantity()); | |
| return; | |
| } | |
| throw new InvalidInputException("There are no " | |
| + transaction.getFruit() | |
| + " in storage"); |
|
|
||
| @Override | ||
| public void handle(FruitTransaction transaction) { | ||
| if (fruitRepository.hasFruit(transaction.getFruit())) { |
No description provided.