Sharing iP code quality feedback [for @kohkaixun]

[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, to help you improve the iP code further.

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

Example from src/main/java/duke/DialogBox.java lines 38-38:

        //this.displayPicture.setFitWidth(100.0);

Example from src/main/java/duke/DialogBox.java lines 39-39:

        //this.displayPicture.setFitHeight(100.0);

Suggestion: Remove dead code from the codebase.

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/DukeException.java lines 18-21:

    /**
     * Used to get the error message describing the problem.
     * @return The error message.
     */

Example from src/main/java/duke/Storage.java lines 45-49:

    /**
     * Load past data inputted by user.
     * @return An array of tasks containing the list of tasks still in the list after user previously exited.
     * @throws DukeException When filepath of file that is supposed to store past data cannot be created or found.
     */

Example from src/main/java/duke/Storage.java lines 72-76:

    /**
     * Save new data inputted by user.
     * @param taskList The TaskList object storing the list of Task objects.
     * @throws DukeException When filepath of file that is supposed to store new data cannot be created or found.
     */

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 👍

ℹ️ 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.