Implemented the FruitShop using Solid principles #959
Conversation
| public class ReportCreatorImpl implements ReportCreator { | ||
| private static final String SEPARATOR = ","; | ||
| private static final String REPORT_HEADER = "fruit,quantity"; | ||
| private final StorageDao storageDao = new StorageDaoImpl(); |
There was a problem hiding this comment.
Consider injecting storage dao implementation through the constructor.
| return storageDao.getALl().entrySet().stream() | ||
| .map(e -> e.getKey().getName() + SEPARATOR + e.getValue()) | ||
| .collect(Collectors.joining(System.lineSeparator(), | ||
| REPORT_HEADER + System.lineSeparator(), "")); |
There was a problem hiding this comment.
Just for the consistency, make "" a constant as well
| private DataReader dataReader; | ||
| private DataProcessing dataProcessing; | ||
| private ReportCreator reportCreator; | ||
| private Writer writer; |
| dataReader = new DataReaderFromCsv(); | ||
| dataProcessing = new DataProcessingImpl(new OperationStrategyImpl(map)); | ||
| reportCreator = new ReportCreatorImpl(); | ||
| writer = new CsvWriter(); |
There was a problem hiding this comment.
I would say it is preferable to pass the initialisation objects to the constructor, like
public FruitServiceImpl(DataReader dataReader,...)
| private static final int OPERATION_POSITION = 0; | ||
| private static final int FRUIT_NAME_POSITION = 1; | ||
| private static final int AMOUNT_OF_FRUIT_POSITION = 2; | ||
| private OperationStrategy operationStrategy; |
|
|
||
| @Override | ||
| public void updateStorage(FruitTransaction transaction) { | ||
| if (transaction.getAmount() < MIN_AMOUNT) { |
There was a problem hiding this comment.
It looks like you are doing this check in several places. Probably we can extract it to some helper utility method?
|
|
||
| public class SupplyOperationHandler implements OperationHandler { | ||
| private static final int MIN_AMOUNT = 0; | ||
| private StorageDao storageDao = new StorageDaoImpl(); |
There was a problem hiding this comment.
May be final and consider initialising through the constructor as well
| } | ||
| } | ||
|
|
||
| private boolean validAmountOfFruit(FruitTransaction transaction) { |
There was a problem hiding this comment.
Probably there is no need in boolean return value. We can simply make it void and just call it in updateStorage
| private String fruitName; | ||
| private int amount; | ||
|
|
||
| public Operation getOperationByLetter(String letter) { |
There was a problem hiding this comment.
Just wondering if it is correct to make Operation in the same file as FruitTransaction? I see it is used not only inside enclosing class, so probable we can extract Operation to the separate file?
| import java.util.List; | ||
|
|
||
| public class DataReaderFromCsv implements DataReader { | ||
| private static final int rowToSkip = 1; |
There was a problem hiding this comment.
Pay more attention to the constants format in Java
| import core.basesyntax.model.FruitTransaction; | ||
|
|
||
| public interface OperationHandler { | ||
| int MIN_AMOUNT = 0; |
There was a problem hiding this comment.
Probably we can make it protected or private here?
No description provided.