Sharing iP code quality feedback [for @kohkaixun] - Round 2

[nus-se-script]

@kohkaixun We did an automated analysis of your code to detect potential areas to improve the code quality. We are sharing the results below, so that you can avoid similar problems in your tP code (which will be graded more strictly for code quality).

IMPORTANT: Note that the script looked for just a few easy-to-detect problems only, and at-most three example are given i.e., there can be other areas/places to improve.

Aspect: Tab Usage

No easy-to-detect issues 👍

Aspect: Naming boolean variables/methods

No easy-to-detect issues 👍

Aspect: Brace Style

No easy-to-detect issues 👍

Aspect: Package Name Style

No easy-to-detect issues 👍

Aspect: Class Name Style

No easy-to-detect issues 👍

Aspect: Dead Code

No easy-to-detect issues 👍

Aspect: Method Length

Example from src/main/java/duke/Duke.java lines 67-132:

    public void start(Stage stage) {
        //Step 1. Setting up required components

        //The container for the content of the chat to scroll.
        this.scrollPane = new ScrollPane();
        this.dialogContainer = new VBox();
        this.scrollPane.setContent(this.dialogContainer);

        this.userInput = new TextField();
        this.sendButton = new Button("Send");

        AnchorPane mainLayout = new AnchorPane();
        mainLayout.getChildren().addAll(this.scrollPane, this.userInput, this.sendButton);

        this.scene = new Scene(mainLayout);

        stage.setScene(this.scene);
        stage.show();

        //Step 2. Formatting the window to look as expected
        stage.setTitle("Duke");
        stage.setResizable(false);
        stage.setMinHeight(600.0);
        stage.setMinWidth(400.0);

        mainLayout.setPrefSize(400.0, 600.0);

        this.scrollPane.setPrefSize(385, 535);
        this.scrollPane.setHbarPolicy(ScrollPane.ScrollBarPolicy.NEVER);
        this.scrollPane.setVbarPolicy(ScrollPane.ScrollBarPolicy.ALWAYS);

        this.scrollPane.setVvalue(1.0);
        this.scrollPane.setFitToWidth(true);

        // You will need to import `javafx.scene.layout.Region` for this.
        this.dialogContainer.setPrefHeight(Region.USE_COMPUTED_SIZE);

        this.userInput.setPrefWidth(325.0);

        this.sendButton.setPrefWidth(55.0);

        AnchorPane.setTopAnchor(this.scrollPane, 10.0);

        AnchorPane.setBottomAnchor(this.sendButton, 1.0);
        AnchorPane.setRightAnchor(this.sendButton, 5.0);

        AnchorPane.setLeftAnchor(this.userInput , 5.0);
        AnchorPane.setBottomAnchor(this.userInput, 1.0);

        Label dukeText = new Label(this.ui.showWelcome());
        this.dialogContainer.getChildren().addAll(
                DialogBox.getUserDialog(dukeText, new ImageView(this.user))
        );

        //Part 3. Add functionality to handle user input.
        this.sendButton.setOnMouseClicked((event) -> {
            this.handleUserInput(stage);
        });

        this.userInput.setOnAction((event) -> {
            this.handleUserInput(stage);
        });

        //Scroll down to the end every time dialogContainer's height changes.
        this.dialogContainer.heightProperty().addListener((observable) -> this.scrollPane.setVvalue(1.0));
    }

Example from src/main/java/duke/Parser.java lines 34-105:

    public static Command parse(String str) {
        Command command;
        String firstWord;
        final int strLength = str.length();

        int indexOfFirstSpace = str.indexOf(" ");
        if (indexOfFirstSpace == -1) {
            firstWord = str;
        } else {
            firstWord = str.substring(0, indexOfFirstSpace);
        }
        switch (firstWord) {
        case (Parser.TODO):
            Parser.checkToDoCommand(str);
            final var toDoFormatLength = Parser.TODO.length() + 1;
            assert strLength > toDoFormatLength;
            command = new AddCommand(str);
            break;
        case (Parser.DEADLINE):
            Parser.checkDeadlineCommand(str);
            final var deadlineFormatLength = 28;
            final var deadlineField = " /by ";
            assert strLength > deadlineFormatLength;
            assert str.contains(deadlineField);
            command = new AddCommand(str);
            break;
        case (Parser.EVENT):
            Parser.checkEventCommand(str);
            final var eventFormatLength = 44;
            final var eventField1 = " /from ";
            final var eventField2 = " /to ";
            assert strLength > eventFormatLength;
            assert str.contains(eventField1) && str.contains(eventField2);
            command = new AddCommand(str);
            break;
        case (Parser.DELETE):
            int deleteIndex = Parser.getDeleteIndex(str);
            assert !(deleteIndex < TaskList.MIN_INDEX && deleteIndex > TaskList.MAX_INDEX);
            command = new DeleteCommand(deleteIndex);
            break;
        case (Parser.MARK):
            int markIndex = Parser.getMarkIndex(str);
            assert !(markIndex < TaskList.MIN_INDEX && markIndex > TaskList.MAX_INDEX);
            command = new MarkCommand(markIndex);
            break;
        case (Parser.UNMARK):
            int unMarkIndex = Parser.getUnMarkIndex(str);
            assert !(unMarkIndex < TaskList.MIN_INDEX && unMarkIndex > TaskList.MAX_INDEX);
            command = new UnMarkCommand(unMarkIndex);
            break;
        case (Parser.LIST):
            Parser.checkListCommand(str);
            command = new ListCommand();
            break;
        case (Parser.FIND):
            String keyword = Parser.getKeyword(str);
            command = new FindCommand(keyword);
            break;
        case (Parser.UPDATE):
            int updateIndex = Parser.getUpdateIndex(str);
            String body = Parser.getUpdateBody(str);
            command = new UpdateCommand(updateIndex, body);
            break;
        case (Parser.BYE):
            Parser.checkExitCommand(str);
            command = new ExitCommand();
            break;
        default:
            throw new RuntimeException("Huh? I don't know what that means :(");
        }
        return command;
    }

Example from src/main/java/duke/Parser.java lines 146-181:

    private static void checkEventCommand(String str) {
        String eventSpace = Parser.EVENT + " ";
        if (str.equals(Parser.EVENT) || str.equals(eventSpace)) {
            throw new RuntimeException("This command's field can't be left blank!");
        }
        final String target1 = " /from ";
        final String target2 = " /to ";
        final var target1Length = target1.length();
        final var target2Length = target2.length();
        if (!(str.contains(target1) && str.contains(target2))) {
            throw new RuntimeException("Unable to create Event! Event commands need a /from and /to field!");
        }
        int index1 = str.indexOf(target1);
        int index2 = str.indexOf(target2);
        final var minCharBetweenTargets = 20;
        boolean isTarget1BehindTarget2 = index2 - index1 < 0;
        if (isTarget1BehindTarget2) {
            throw new RuntimeException("Unable to create event! "
                    + "The /from field has to be before the /to field.");
        } else if (index2 - index1 < minCharBetweenTargets) {
            throw new RuntimeException("Unable to create event! Please enter a valid /from field.");
        }
        final int eventDescriptionStartIndex = Parser.EVENT.length() + 1;
        if (index1 <= eventDescriptionStartIndex) {
            throw new RuntimeException("Unable to create event! Description for event cannot be left blank");
        }
        String start = str.substring(index1 + target1Length, index2);
        String end = str.substring(index2 + target2Length);
        int startFirstSlash = start.indexOf("/");
        int startSecondSlash = start.indexOf("/", startFirstSlash + 1);
        int endFirstSlash = end.indexOf("/");
        int endSecondSlash = end.indexOf("/", endFirstSlash + 1);
        if (startFirstSlash == -1 || startSecondSlash == -1 || endFirstSlash == -1 || endSecondSlash == -1) {
            throw new RuntimeException("Unable to create Deadline! Check your date and time format!");
        }
    }

Suggestion: Consider applying SLAP (and other abstraction mechanisms) to shorten methods e.g., extract some code blocks into separate methods. You may ignore this suggestion if you think a longer method is justified in a particular case.

Aspect: Class size

No easy-to-detect issues 👍

Aspect: Header Comments

Example from src/main/java/duke/TaskList.java lines 37-41:

    /**
     * Add task to task list.
     * @param task The Task object to be added.
     * @return The Duke chatbot's reply after adding a task.
     */

Example from src/main/java/duke/TaskList.java lines 52-56:

    /**
     * Delete task from task list.
     * @param index Index of task in task list to be deleted.
     * @return The Duke chatbot's reply after deleting a task.
     */

Example from src/main/java/duke/TaskList.java lines 73-77:

    /**
     * Mark a task in task list as done.
     * @param index Index of task to be marked as done.
     * @return The Duke chatbot's reply after marking a task as done.
     */

Suggestion: Ensure method/class header comments follow the format specified in the coding standard, in particular, the phrasing of the overview statement.

Aspect: Recent Git Commit Message (Subject Only)

No easy-to-detect issues 👍

Aspect: Binary files in repo

No easy-to-detect issues 👍

❗ You are not required to (but you are welcome to) fix the above problems in your iP, unless you have been separately asked to resubmit the iP due to code quality issues.

ℹ️ The bot account used to post this issue is un-manned. Do not reply to this post (as those replies will not be read). Instead, contact [email protected] if you want to follow up on this post.