covered solution with tests #817

dima-semeniuk · 2023-10-24T17:03:12Z

No description provided.

HovorukhaBohdan

Good job!

bhdnchui

Good job.
Let's add .DS_Store files to gitignore.

bhdnchui · 2023-10-26T07:00:48Z

src/test/java/core/basesyntax/dao/StorageDaoImplTest.java

+    static void beforeAll() {
+        storageDao = new StorageDaoImpl();
+        expectedMap = new HashMap<>();
+


Suggested change

bhdnchui · 2023-10-26T07:02:49Z

src/test/java/core/basesyntax/dao/StorageDaoImplTest.java

+        Map<Fruit, Integer> actualMap = storageDao.getALl();
+        assertEquals(expectedMap, actualMap);
+    }
+


If you have method isInStorage, test it too.

bhdnchui · 2023-10-26T07:07:16Z

src/test/java/core/basesyntax/service/handler/impl/BalanceOperationHandlerTest.java

+        transactionWithNegativeAmount.setOperation(Operation.BALANCE);
+        transactionWithNegativeAmount.setFruitName(FRUIT_NAME);
+        transactionWithNegativeAmount.setAmount(FRUIT_NEGATIVE_AMOUNT);
+


Suggested change

bhdnchui · 2023-10-26T07:07:25Z

src/test/java/core/basesyntax/service/handler/impl/BalanceOperationHandlerTest.java

+        Integer expectedFruitAmount = FRUIT_AMOUNT;
+        Integer actualFruitAmount = Storage.fruits.get(new Fruit(FRUIT_NAME));
+        assertEquals(expectedFruitAmount, actualFruitAmount);
+


Suggested change

bhdnchui · 2023-10-26T07:09:41Z

src/test/java/core/basesyntax/service/handler/impl/BalanceOperationHandlerTest.java

+
+    @Test
+    void updateStorage_fruitAmountIsNegative_isNotOk() {
+        assertThrows(RuntimeException.class, () -> {


Check the message of exception or use other exception type.

bhdnchui · 2023-10-26T07:09:57Z

src/test/java/core/basesyntax/service/handler/impl/PurchaseOperationHandlerTest.java

+        transactionWithLessAmount.setOperation(Operation.PURCHASE);
+        transactionWithLessAmount.setFruitName(FRUIT_NAME);
+        transactionWithLessAmount.setAmount(FRUIT_LESS_AMOUNT);
+


Suggested change

bhdnchui · 2023-10-26T07:10:47Z

src/test/java/core/basesyntax/service/handler/impl/PurchaseOperationHandlerTest.java

+
+    @Test
+    void updateStorage_emptyStorage_isNotOk() {
+        assertThrows(RuntimeException.class, () -> {


Be sure that business exception was thrown.

bhdnchui · 2023-10-26T07:11:09Z

src/test/java/core/basesyntax/service/handler/impl/ReturnOperationHandlerTest.java

+        transactionWithNegativeAmount.setOperation(Operation.RETURN);
+        transactionWithNegativeAmount.setFruitName(FRUIT_NAME);
+        transactionWithNegativeAmount.setAmount(FRUIT_NEGATIVE_AMOUNT);
+


Suggested change

bhdnchui · 2023-10-26T07:11:19Z

src/test/java/core/basesyntax/service/handler/impl/ReturnOperationHandlerTest.java

+        Integer expectedFruitAmount = FRUIT_AMOUNT * NUMBER_OF_OPERATION;
+        Integer actualFruitAmount = Storage.fruits.get(new Fruit(FRUIT_NAME));
+        assertEquals(expectedFruitAmount, actualFruitAmount);
+


Suggested change

bhdnchui · 2023-10-26T07:11:37Z

src/test/java/core/basesyntax/service/handler/impl/SupplyOperationHandlerTest.java

+        transactionWithNegativeAmount.setOperation(Operation.SUPPLY);
+        transactionWithNegativeAmount.setFruitName(FRUIT_NAME);
+        transactionWithNegativeAmount.setAmount(FRUIT_NEGATIVE_AMOUNT);
+


Suggested change

mnyshenko

Critical points to consider

mnyshenko · 2023-10-26T14:54:52Z

src/test/java/core/basesyntax/service/impl/DataReaderFromCsvTest.java

+    }
+
+    @Test
+    public void readData_fileNotExistOrBadPath_notOk() {


Why did you choose to put public here?

mnyshenko · 2023-10-26T15:02:08Z

src/test/java/core/basesyntax/service/impl/ReportCreatorImplTest.java

+        Storage.fruits.put(BANANA, 100);
+        Storage.fruits.put(GRAPE, 40);
+        Storage.fruits.put(APPLE, 90);


seems to be magic numbers

mnyshenko · 2023-10-26T15:06:16Z

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

+
+    public static Operation getOperationOf(String letter) {
+        return Arrays.stream(Operation.values())
+                .filter(o -> o.getCodeOfOperation().equals(letter))


please choose a proper name for parameters in the lambdas; other developers will need to spend additional time figuring out what 'o' means

still exists.

mnyshenko · 2023-10-26T15:07:12Z

src/main/java/core/basesyntax/dao/StorageDaoImpl.java

+    }
+
+    public boolean isInStorage(String name) {
+        return (get(name) != null);


(get(name) != null)
these parentheses are redundant

still exists.

mnyshenko · 2023-10-26T15:08:03Z

src/main/java/core/basesyntax/dao/StorageDao.java

+
+    boolean isInStorage(String name);
+
+    Map<Fruit, Integer> getALl();


Minor: a little problem with the name

still exists.

mnyshenko · 2023-10-26T15:09:37Z

src/main/java/core/basesyntax/service/handler/impl/BalanceOperationHandler.java

+        if (!storageDao.isInStorage(transaction.getFruitName())) {
+            storageDao.add(new Fruit(transaction.getFruitName()), transaction.getAmount());


transaction.getFruitName() can be extracted to a separate variable to avoid code duplication

still exists.

mnyshenko · 2023-10-26T15:10:04Z

src/main/java/core/basesyntax/service/handler/impl/PurchaseOperationHandler.java

+        Integer fruitAmount = fruitAndAmount.getValue();
+        Integer fruitAmountFromTransaction = transaction.getAmount();
+        fruitAndAmount.setValue(fruitAmount - fruitAmountFromTransaction);
+


mnyshenko · 2023-10-26T15:10:36Z

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

+    @Override
+    public String createReport() {
+        return storageDao.getALl().entrySet().stream()
+                .map(e -> e.getKey().getName() + SEPARATOR + e.getValue())


parameter naming again

still exists.

mnyshenko · 2023-10-26T15:15:07Z

src/test/java/core/basesyntax/dao/StorageDaoImplTest.java

+    void storageDao_add_isOk() {
+        storageDao.add(fruit, AMOUNT);
+        Map<Fruit, Integer> actualMap = Storage.fruits;
+        assertEquals(expectedMap, actualMap);
+    }
+
+    @Test
+    void storageDao_getExistingFruit_isOk() {
+        Storage.fruits.put(fruit, AMOUNT);
+        Map.Entry<Fruit, Integer> actualEntry = storageDao.get(fruit.getName());
+        Fruit actualFruit = actualEntry.getKey();
+        Integer actualAmount = actualEntry.getValue();
+        assertEquals(fruit, actualFruit);
+        assertEquals(AMOUNT, actualAmount);
+    }
+
+    @Test
+    void storageDao_getNullIfNotExist_isOk() {
+        Storage.fruits.put(fruit, AMOUNT);
+        Map.Entry<Fruit, Integer> actualEntry = storageDao.get(NAME_OF_NOT_EXISTING_FRUIT);
+        assertNull(actualEntry);
+    }
+
+    @Test
+    void storageDao_getAll_isOk() {
+        Storage.fruits.put(fruit, AMOUNT);
+        Map<Fruit, Integer> actualMap = storageDao.getALl();
+        assertEquals(expectedMap, actualMap);
+    }
+
+    @Test
+    void storageDao_isInStorage_isOk() {
+        Storage.fruits.put(fruit, AMOUNT);
+        boolean actual = storageDao.isInStorage(fruit.getName());
+        assertTrue(actual);
+    }
+
+    @Test
+    void storageDao_isNotInStorage_isOk() {
+        boolean actual = storageDao.isInStorage(FRUIT_NAME);
+        assertFalse(actual);
+    }


Try to divide statements in tests by the following principle:
https://medium.com/@gitaeklee/given-when-then-junit-test-ba49564303e7

writing comments like //when //then is optional; you can divide everything by adding empty lines between statements

d1sam

There are other mentors' comments are still exist.

d1sam · 2023-10-27T09:05:42Z

src/main/java/core/basesyntax/dao/StorageDao.java

+
+    boolean isInStorage(String name);
+
+    Map<Fruit, Integer> getALl();


still exists.

d1sam · 2023-10-27T09:05:48Z

src/main/java/core/basesyntax/dao/StorageDaoImpl.java

+    }
+
+    public boolean isInStorage(String name) {
+        return (get(name) != null);


still exists.

d1sam · 2023-10-27T09:06:00Z

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

+
+    public static Operation getOperationOf(String letter) {
+        return Arrays.stream(Operation.values())
+                .filter(o -> o.getCodeOfOperation().equals(letter))


still exists.

d1sam · 2023-10-27T09:06:27Z

src/main/java/core/basesyntax/service/handler/impl/BalanceOperationHandler.java

+        if (!storageDao.isInStorage(transaction.getFruitName())) {
+            storageDao.add(new Fruit(transaction.getFruitName()), transaction.getAmount());


still exists.

d1sam · 2023-10-27T09:06:34Z

src/main/java/core/basesyntax/service/handler/impl/PurchaseOperationHandler.java

+        Integer fruitAmount = fruitAndAmount.getValue();
+        Integer fruitAmountFromTransaction = transaction.getAmount();
+        fruitAndAmount.setValue(fruitAmount - fruitAmountFromTransaction);
+


Suggested change

d1sam · 2023-10-27T09:08:32Z

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

+                    .skip(ROW_TO_SKIP)
+                    .toList();
+        } catch (IOException e) {
+            throw new RuntimeException("Can't read data from file: " + file);


make this message as constant.

d1sam · 2023-10-27T09:08:48Z

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

+    @Override
+    public String createReport() {
+        return storageDao.getALl().entrySet().stream()
+                .map(e -> e.getKey().getName() + SEPARATOR + e.getValue())


still exists.

d1sam · 2023-10-27T09:09:11Z

src/main/java/core/basesyntax/strategy/impl/OperationStrategyImpl.java

+    @Override
+    public OperationHandler getOperationHandler(Operation operation) {
+        if (operationHandlerMap == null) {
+            throw new RuntimeException("Map with handlers is NULL!!!");


make message as constant.

d1sam · 2023-10-27T09:09:30Z

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

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


make this message as constant.

d1sam · 2023-10-27T09:11:12Z

src/test/java/core/basesyntax/service/impl/DataReaderFromCsvTest.java

+    }
+
+    @Test
+    public void readData_fileNotExistOrBadPath_notOk() {


Suggested change

public void readData_fileNotExistOrBadPath_notOk() {

void readData_fileNotExistOrBadPath_notOk() {

kshuryhin

Good job

…/add_link_to_readme add link to readme file

covered solution with tests

cdcc324

HovorukhaBohdan approved these changes Oct 25, 2023

View reviewed changes

bhdnchui suggested changes Oct 26, 2023

View reviewed changes

dima-semeniuk added 2 commits October 26, 2023 15:48

made some style and logic changes

c34f14d

added DS_Store to gitignore

90b9866

dima-semeniuk requested a review from bhdnchui October 26, 2023 13:07

mnyshenko suggested changes Oct 26, 2023

View reviewed changes

devided statements in tests by the Given-When-Then principles

8fa3ad9

dima-semeniuk requested a review from mnyshenko October 26, 2023 15:55

d1sam suggested changes Oct 27, 2023

View reviewed changes

fixed previous maistakes

fe6051d

dima-semeniuk requested a review from d1sam October 27, 2023 09:58

kshuryhin approved these changes Oct 30, 2023

View reviewed changes

dlvsn pushed a commit to dlvsn/jv-fruit-shop-tests that referenced this pull request Sep 6, 2024

Merge pull request mate-academy#817 from mate-academy/feature/no-task…

9bc906b

…/add_link_to_readme add link to readme file


		boolean isInStorage(String name);

		Map<Fruit, Integer> getALl();

		if (!storageDao.isInStorage(transaction.getFruitName())) {
		storageDao.add(new Fruit(transaction.getFruitName()), transaction.getAmount());

	public void readData_fileNotExistOrBadPath_notOk() {
	void readData_fileNotExistOrBadPath_notOk() {

covered solution with tests #817

Are you sure you want to change the base?

covered solution with tests #817

Conversation

dima-semeniuk commented Oct 24, 2023

HovorukhaBohdan left a comment

Choose a reason for hiding this comment

bhdnchui 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

mnyshenko 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

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

d1sam 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

kshuryhin left a comment

Choose a reason for hiding this comment