[jweeyh] iP #162

jweeyh · 2023-01-24T04:34:13Z

No description provided.

# Conflicts: # src/main/java/Duke.java # src/main/java/DukeList.java

…ubstring

dfordarius

Overall code quality is pretty good! Good OOP with some mixor fixes to be done! Keep up the good work 😄

dfordarius · 2023-01-30T16:34:13Z

src/main/java/duke/Duke.java

+    private Storage storage;
+    private DukeList dukeList;
+    private Ui ui;


Perhaps these 3 variables can be final?

dfordarius · 2023-01-30T16:37:56Z

src/main/java/duke/DukeList.java

+        }
+        System.out.println("Sure, Imma add that real quick");
+        System.out.println(task);
+        list.add(task);
+        System.out.println("Now you've got " + list.size() + pluralTask(list.size()));
+        System.out.println(new TextBorder(""));
+    }


Maybe you can move all the prints into the Ui class? After all you have a Ui class and the Ui class should deal with interactions with the user

dfordarius · 2023-01-30T16:40:21Z

src/main/java/command/MarkCommand.java

+public class MarkCommand extends Command{
+
+    private String task;
+    private boolean marker;


Maybe change it to a more boolean sounding name like "isMarked" ?

dfordarius · 2023-01-30T16:44:12Z

src/main/java/duke/Parser.java

+            if (inputText.equals("bye")) {
+                return new ExitCommand(this.dukelist, this.storage);
+            } else if (first.equals("list")){
+                return new ListCommand(this.dukelist);
+            } else if (first.equals("mark")) {
+                return new MarkCommand(array[1], true, dukelist);
+            } else if (first.equals("unmark")) {
+                return new MarkCommand(array[1], false, dukelist);
+            } else if (first.equals("delete")){
+                return new DeleteCommand(array[1], dukelist);
+            } else if (first.equals("find")) {
+                return new FindCommand(array[1], dukelist);
+            } else {
+                if (array.length != 2) {
+                    if (first.equals("todo") || first.equals("deadline") || first.equals("event")) {


Maybe u can consider using switch for this part so it looks cleaner and improve readability?

dfordarius · 2023-01-30T16:59:50Z

src/main/java/duke/DukeList.java

+    public void add(String type, String s) {
+        Task task = new Task();
+        if (type.equals("todo")) {
+            task = new Todo(s);
+


Maybe this method could be named to be more descriptive like "addTask"

jweeyh · 2023-02-02T03:19:56Z

Hey Darius, thanks so much for the review!

…

On Tue, Jan 31, 2023 at 1:00 AM Darius Ng ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/duke/DukeList.java <#162 (comment)> : > + public void add(String type, String s) { + Task task = new Task(); + if (type.equals("todo")) { + task = new Todo(s); + Maybe this method could be named to be more descriptive like "addTask" — Reply to this email directly, view it on GitHub <#162 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AU4UEONHH52UFRYH6G4NC6TWU7XRXANCNFSM6AAAAAAUEUCHMY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

bryansendeavour

Code looks good, some minor suggestions made feel free to disregard if found unnecessary. Cheers!

bryansendeavour · 2023-02-03T14:04:12Z

src/main/java/command/MarkCommand.java

+public class MarkCommand extends Command{
+
+    private String task;
+    private boolean marker;


Maybe isMarked could be used for your boolean variable?

bryansendeavour · 2023-02-03T14:05:26Z

src/main/java/duke/DukeList.java

+import task.*;
+import exception.*;


Would it be better to import all the classes instead?

bryansendeavour · 2023-02-03T14:07:03Z

src/main/java/duke/DukeList.java

+    private String pluralTask (int size) {
+        if (size == 1) {
+            return " task.";
+        } else {
+            return " tasks.";
+        }
+    }


Would it be better to put this in your Ui class?

bryansendeavour · 2023-02-03T14:08:29Z

src/main/java/duke/Parser.java

@@ -0,0 +1,66 @@
+package duke;
+
+import command.*;


Maybe to conform to conventions you would like to import each class separately?

bryansendeavour · 2023-02-03T14:09:56Z

src/main/java/duke/TextBorder.java

+
+public class TextBorder {
+    String text;
+    String border = "//  /  /  //  /  /  //  /  /  //  /  /  //  /  /  //  /  /  //  /";


Maybe this can be static final?

# Conflicts: # src/main/java/command/ExitCommand.java # src/main/java/command/FindCommand.java # src/main/java/command/ListCommand.java # src/main/java/command/MarkCommand.java # src/main/java/duke/DukeList.java

Add Assert to SaveList in Storage

DerenC

A brief review of code quality, mainly on these problems
a. weak SLAP
b. arrow-head style code
c. too-long methods
d. too-deep nesting

DerenC · 2023-02-09T04:30:51Z

src/main/java/duke/Storage.java

+    public DukeList retrieveList (Ui ui) {
+        try {
+            if ((new File(path)).exists()) {
+                FileInputStream fis = new FileInputStream(path);
+                ObjectInputStream ois = new ObjectInputStream(fis);
+                try {
+                    DukeList savedDukeList = (DukeList) ois.readObject();
+                    savedDukeList.setUi(ui);
+                    return savedDukeList;
+
+                } catch (IOException e) {
+                    ui.addStatement("Creating new save");
+                }
+            }
+
+        } catch (FileNotFoundException e) {
+            ui.addStatement("Hey sorry man, something's off bout you're save data.");
+            ui.addStatement(e.toString());
+        } catch (IOException e) {
+            ui.addStatement("Yo something's up with your I/O, gotta get it checked before doing this.");
+            ui.addStatement(e.toString());
+            System.out.println(e);
+        } catch (ClassNotFoundException e) {
+            ui.addStatement("Hey I can't find your object class.");
+            ui.addStatement(e.toString());
+        }
+        return new DukeList(ui);
+    }


Maybe can make this double nested try-catch block into a single level try-catch

DerenC · 2023-02-09T04:34:45Z

src/main/java/gui/Gui.java

+    public void start(Stage primaryStage) throws Exception {
+        scrollPane = new ScrollPane();
+        dialogContainer = new VBox();
+        scrollPane.setContent(dialogContainer);
+
+        userInput = new TextField();
+        sendButton = new Button("Send");
+
+        AnchorPane mainLayout = new AnchorPane();
+        mainLayout.getChildren().addAll(scrollPane, userInput,sendButton);
+
+        scene = new Scene(mainLayout);
+        primaryStage.setScene(scene);
+        primaryStage.show();
+
+        //2
+        primaryStage.setTitle("Duke");
+        primaryStage.setResizable(false);
+        primaryStage.setMinHeight(600.0);
+        primaryStage.setMinWidth(400.0);
+
+        mainLayout.setPrefSize(400.0, 600.0);
+
+        scrollPane.setPrefSize(385, 535);
+        scrollPane.setHbarPolicy(ScrollPane.ScrollBarPolicy.NEVER);
+        scrollPane.setVbarPolicy(ScrollPane.ScrollBarPolicy.ALWAYS);
+
+        scrollPane.setVvalue(1.0);
+        scrollPane.setFitToWidth(true);
+
+        dialogContainer.setPrefHeight(Region.USE_COMPUTED_SIZE);
+
+        userInput.setPrefWidth(325.0);
+
+        sendButton.setPrefWidth(55.0);
+
+        AnchorPane.setTopAnchor(scrollPane, 1.0);
+
+        AnchorPane.setBottomAnchor(sendButton, 1.0);
+        AnchorPane.setRightAnchor(sendButton, 1.0);
+
+        AnchorPane.setLeftAnchor(userInput , 1.0);
+        AnchorPane.setBottomAnchor(userInput, 1.0);
+
+        sendButton.setOnMouseClicked((event) -> {
+            handleUserInput();
+        });
+
+        userInput.setOnAction((event) -> {
+            handleUserInput();
+        });
+
+        dialogContainer.getChildren().addAll(
+                new DialogBox(new Label("Waddup the name's Duncan. Sorry but Duke couldn't make it, had a pretty bad stomach-ache."), new ImageView(duncan)),
+                new DialogBox(new Label("So what do you need bro?"), new ImageView(duncan))
+        );
+    }


While the use of SLAP is quite good here, the method seems a bit too long

DerenC · 2023-02-09T04:38:53Z

src/main/java/duke/Parser.java

+            } else {
+                if (array.length != 2) {
+                    if (first.equals("todo") || first.equals("deadline") || first.equals("event")) {
+                        throw new EmptyDescriptionException("Sorry, your " + first + " task description is missing.");
+
+                    } else {
+                        throw new UnknownInputException("Hmm, I'm not sure what you're saying man.");
+
+                    }
+                }
+                return new AddCommand(array[0], array[1], dukelist);
+
+            }
+        } catch (ArrayIndexOutOfBoundsException e) {


This part of the code seems a bit too deeply nested

There was text present that contained the default String drawing of Duke, which was relevant only in CLI, but has since become redundant with the new GUI. This currently unused text is thus deleted in accordance with the coding standard to remove unused and outdated code. Let's try to find other instances in the code which could be simplified and refactored.

At the start of the run function, it is important that the statements in the Ui are cleared so that previous commands would not be executed unintentionally. An assert is thus placed after ui.clearStatements This is done to confirm that the Ui statements have been cleared

Add assert to the run function in Duke

# Conflicts: # src/main/java/duke/Parser.java

Branch Coding Standards

As a matter of housekeeping and storage, there might be tasks that need to be removed from the current list, but may be retrieved in the future An archive function that creates and stores another DukeList is implemented As an extension, the archive function can help to store redundant tasks in the main list into a separate list for future retrieval

damithc and others added 19 commits July 31, 2022 17:20

Add Gradle support

556af3f

Level 1

f8ea90d

created list function

c623edd

Added markings

85620ea

Created todo, deadline, event

5111353

Added exceptions and delete

39cfca1

Add save function to local data

5526229

Change date from String type to LocalDate type

ee4cfdb

Merge branch 'branch-Level-8'

6ee3c9e

# Conflicts: # src/main/java/Duke.java # src/main/java/DukeList.java

Create more classes to abstract out common objects

2da0ff6

Merge branch 'add-gradle-support'

4ac62ee

Add Gradle

c10ec6d

Add JUnit tests for Parser and Task

a06dae7

Create Jar file

bcad45f

Create find function that checks if stored tasks have a given input s…

4746649

…ubstring

Add JavaDocs documentation for commands, tasks and parser

ad4e08b

Check that code complies to coding standard

a35dadc

Merge branch 'branch-A-CodingStandard'

7ab24f9

Check that code complies to coding standard

aa5c4f6

dfordarius reviewed Jan 30, 2023

View reviewed changes

Update README.md

efa145e

bryansendeavour reviewed Feb 3, 2023

View reviewed changes

jweeyh and others added 6 commits February 6, 2023 20:46

Add Gui

ea29af0

Merge branch 'branch-A-CodingStandard'

698beeb

# Conflicts: # src/main/java/command/ExitCommand.java # src/main/java/command/FindCommand.java # src/main/java/command/ListCommand.java # src/main/java/command/MarkCommand.java # src/main/java/duke/DukeList.java

Merge remote-tracking branch 'origin/master'

47ed8b3

Add Gui

f002416

Add Assert to SaveList in Storage

15666f3

Merge pull request #1 from jweeyh/branch-A-Assertions

3139c8d

Add Assert to SaveList in Storage

DerenC reviewed Feb 9, 2023

View reviewed changes

jweeyh and others added 14 commits February 10, 2023 14:29

Change Parser to fit the coding standards

344e92a

Merge pull request #4 from jweeyh/branch-A-Assertions

93732ab

Add assert to the run function in Duke

Merge branch 'master' into branch-A-CodingStandard

f0cb705

# Conflicts: # src/main/java/duke/Parser.java

Merge pull request #3 from jweeyh/branch-A-CodingStandard

cb8d29e

Branch Coding Standards

Build shadowJar of project

b5b1ec0

Add Ui.png to docs

1523fa6

Update README.md

07d8cef

Rename DukeException to DuncanException

c7df3dd

Merge remote-tracking branch 'origin/master'

b43a885

Update README.md

bdc243d

Add error message when input is too short

c402e67

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jweeyh] iP #162

[jweeyh] iP #162

jweeyh commented Jan 24, 2023

dfordarius left a comment

dfordarius Jan 30, 2023

dfordarius Jan 30, 2023

dfordarius Jan 30, 2023

dfordarius Jan 30, 2023

dfordarius Jan 30, 2023

jweeyh commented Feb 2, 2023 via email

bryansendeavour left a comment

bryansendeavour Feb 3, 2023

bryansendeavour Feb 3, 2023

bryansendeavour Feb 3, 2023

bryansendeavour Feb 3, 2023

bryansendeavour Feb 3, 2023

DerenC left a comment

DerenC Feb 9, 2023

DerenC Feb 9, 2023

DerenC Feb 9, 2023

[jweeyh] iP #162

Are you sure you want to change the base?

[jweeyh] iP #162

Conversation

jweeyh commented Jan 24, 2023

dfordarius 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

jweeyh commented Feb 2, 2023 via email

bryansendeavour 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

DerenC 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