hw done #1083

sergiikonov · 2024-12-10T16:47:32Z

No description provided.

liliia-ponomarenko

Good job! But firstly let’s fix some issues with the main solution ;)

liliia-ponomarenko · 2024-12-11T07:37:12Z

src/main/java/core/basesyntax/converter/DataConverterImpl.java

+    private FruitTransaction.Operation mapOperation(String code) {
+        return switch (code) {
+            case "b" -> FruitTransaction.Operation.BALANCE;
+            case "s" -> FruitTransaction.Operation.SUPPLY;
+            case "p" -> FruitTransaction.Operation.PURCHASE;
+            case "r" -> FruitTransaction.Operation.RETURN;
+            default -> throw new IllegalArgumentException("Unknown operation code: " + code);
+        };


don't use switch case here, it will be difficult for you to maintain it in the future. Instead, add the following method to the Operation class:

public static Operation getOperation(String code) { for (Operation value : Operation.values()) { if (value.getCode().equals(code)) { return value; } } throw new IllegalArgumentException(code + " operation doesn't exist."); }

liliia-ponomarenko · 2024-12-11T07:39:11Z

src/main/java/core/basesyntax/converter/DataConverterImpl.java

+    public List<FruitTransaction> convertToTransaction(List<String> data) {
+        List<FruitTransaction> convertedTransaction = new ArrayList<>();
+        for (int i = 1; i < data.size(); i++) { // Skip header
+            String[] parts = data.get(i).split(",");


make separator and other magic numbers constant field

liliia-ponomarenko · 2024-12-11T07:40:06Z

src/main/java/core/basesyntax/generator/ReportGeneratorImpl.java

+    @Override
+    public String getReport() {
+        if (Storage.fruits.isEmpty()) {
+            return "No records to report";


better throw exception

liliia-ponomarenko · 2024-12-11T07:41:01Z

src/main/java/core/basesyntax/generator/ReportGeneratorImpl.java

+        }
+        StringBuilder report = new StringBuilder();
+        for (Map.Entry<String, Integer> entry : Storage.fruits.entrySet()) {
+            report.append(entry.getKey()).append(",").append(entry.getValue()).append("\n");


Make separator constant field and use System.lineseparator() instead \n

liliia-ponomarenko · 2024-12-11T07:42:04Z

src/main/java/core/basesyntax/handler/OperationStrategyImpl.java

+    public OperationHandler getHandler(FruitTransaction.Operation operation) {
+        OperationHandler handler = operationHandlerMap.get(operation);
+        if (handler == null) {
+            throw new NullPointerException("No handler found for operation: " + operation);


better throw IllegalArgumentException or general RuntimeException

liliia-ponomarenko · 2024-12-11T07:42:39Z

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

+    }
+
+    public enum Operation {
+        BALANCE, SUPPLY, PURCHASE, RETURN


Suggested change

BALANCE, SUPPLY, PURCHASE, RETURN

BALANCE("b")...

use values

okuzan

Good job overall!

use src/test/resources for testing

okuzan · 2024-12-11T13:34:41Z

src/main/java/core/basesyntax/Main.java

+        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);


Use Map.of for brevity in such cases

okuzan · 2024-12-11T13:37:56Z

src/test/java/core/basesyntax/ConverterTest.java

+    }
+
+    @Test
+    void converter_testConverterToTransactions_ok() {


Suggested change

void converter_testConverterToTransactions_ok() {

void converter_validTransactions_ok() {

second part is about given conditions

okuzan · 2024-12-11T13:38:18Z

src/test/java/core/basesyntax/ConverterTest.java

+            FruitTransaction actual = actualTransaction.get(i);
+
+            Assertions.assertEquals(expected.getOperation(), actual.getOperation(),
+                    "Операції не збігаються");


don't use anything but english in code

okuzan · 2024-12-11T13:46:55Z

src/test/java/core/basesyntax/FileReaderTest.java

+        } catch (IOException e) {
+            Assertions.fail("Failed to create test directory");
+        } finally {
+            try {
+                Files.deleteIfExists(directory);
+            } catch (IOException e) {
+                Assertions.fail("Failed to delete test directory");
+            }
+        }
+    }


it's better to keep our tests clean if possible, move cleanup handling to afterEach if needed, or loop up how to create temp files (like Files.createTempFile)
If exception is thrown, test will fail anyway

JJJazl · 2024-12-11T16:49:54Z

src/test/java/core/basesyntax/ConverterTest.java

+                new FruitTransaction(FruitTransaction.Operation.SUPPLY, "apple", 50),
+                new FruitTransaction(FruitTransaction.Operation.PURCHASE, "banana", 10)
+        );
+        Assertions.assertEquals(expectedTransaction.size(), actualTransaction.size(),


Use static import for assertions

JJJazl · 2024-12-11T16:51:48Z

src/test/java/core/basesyntax/ConverterTest.java

+    }
+
+    @Test
+    void converter_testInvalidDataToConvert_notOk() {


Let's use the same naming convetion for all test method0s. Please, check everywhere

Suggested change

void converter_testInvalidDataToConvert_notOk() {

void convertToTransaction_invalidDataToConvert_notOk() {

JJJazl · 2024-12-11T16:51:59Z

src/test/java/core/basesyntax/ConverterTest.java

+    }
+
+    @Test
+    void converter_validTransactions_ok() {


Suggested change

void converter_validTransactions_ok() {

void convertToTransaction_validTransactions_ok() {

JJJazl · 2024-12-11T16:53:36Z

src/test/java/core/basesyntax/FileReaderTest.java

+    @Test
+    void readFromFile_validFile_ok() throws IOException {
+        String testFilePath = "src/main/resources/testFile.csv";
+        String fileContent = "type,fruit,quantity\nb,banana,20\ns,apple,50";


Let's use text block to make this line more readable

JJJazl · 2024-12-11T16:58:00Z

src/test/java/core/basesyntax/FileReaderTest.java

+        } catch (IOException e) {
+            Assertions.fail("Failed to create temporary directory", e);
+        } finally {
+            if (tempDirectory != null) {
+                try {
+                    Files.deleteIfExists(tempDirectory);
+                } catch (IOException e) {
+                    Assertions.fail("Failed to delete temporary directory", e);
+                }
+            }
+        }
+    }


Move it to the method with @AfterEach annotation, like you did in FileWriterTest

JJJazl · 2024-12-11T16:59:49Z

src/test/java/core/basesyntax/OperationStrategyTest.java

+        Assertions.assertThrows(IllegalArgumentException.class,
+                () -> operationStrategy.getHandler(null),
+                "IllegalArgumentException.");


Let's check error message content

JJJazl · 2024-12-11T17:00:17Z

src/test/java/core/basesyntax/OperationStrategyTest.java

+    @Test
+    void operationStrategy_returnsCorrectHandler_ok() {
+        OperationHandler handler = operationStrategy.getHandler(FruitTransaction.Operation.SUPPLY);
+
+        Assertions.assertTrue(handler instanceof SupplyOperation,
+                "Handler for SUPPLY should be of type SupplyOperation.");
+    }


Let's do the same for other handlers

JJJazl · 2024-12-11T17:00:38Z

src/test/java/core/basesyntax/ReportGeneratorTest.java

+
+    @Test
+    void reportGenerator_EmptyStorage_notOk() {
+        Assertions.assertThrows(RuntimeException.class, () -> reportGenerator.getReport());


JJJazl · 2024-12-11T17:01:12Z

src/test/java/core/basesyntax/ShopServiceTest.java

+                new FruitTransaction(
+                        FruitTransaction.Operation.PURCHASE, "banana", 20)
+        );
+        Assertions.assertThrows(IllegalArgumentException.class,


hw done

0be92be

liliia-ponomarenko suggested changes Dec 11, 2024

View reviewed changes

edited mistakes in main

fe131ea

sergiikonov requested a review from liliia-ponomarenko December 11, 2024 11:51

okuzan suggested changes Dec 11, 2024

View reviewed changes

fixed test problems

77fc789

sergiikonov requested a review from okuzan December 11, 2024 14:27

JJJazl suggested changes Dec 11, 2024

View reviewed changes

fixed all mistakes

0a65b46

sergiikonov requested a review from JJJazl December 12, 2024 07:24

JJJazl approved these changes Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hw done #1083

hw done #1083

sergiikonov commented Dec 10, 2024

liliia-ponomarenko left a comment

liliia-ponomarenko Dec 11, 2024

liliia-ponomarenko Dec 11, 2024

liliia-ponomarenko Dec 11, 2024

liliia-ponomarenko Dec 11, 2024

liliia-ponomarenko Dec 11, 2024

liliia-ponomarenko Dec 11, 2024

okuzan left a comment

okuzan Dec 11, 2024

okuzan Dec 11, 2024

okuzan Dec 11, 2024

okuzan Dec 11, 2024

JJJazl Dec 11, 2024

JJJazl Dec 11, 2024

JJJazl Dec 11, 2024

JJJazl Dec 11, 2024

JJJazl Dec 11, 2024

JJJazl Dec 11, 2024

JJJazl Dec 11, 2024

JJJazl Dec 11, 2024

JJJazl Dec 11, 2024

	void converter_testConverterToTransactions_ok() {
	void converter_validTransactions_ok() {

	void converter_testInvalidDataToConvert_notOk() {
	void convertToTransaction_invalidDataToConvert_notOk() {

	void converter_validTransactions_ok() {
	void convertToTransaction_validTransactions_ok() {

hw done #1083

Are you sure you want to change the base?

hw done #1083

Conversation

sergiikonov commented Dec 10, 2024

liliia-ponomarenko 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

okuzan 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