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

[soc-se-bot-red]

@yadobler 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

Example from src/main/java/yappingbot/commands/CommandBase.java lines 24-24:

    private boolean argumentsLoaded = false;

Example from src/main/java/yappingbot/commands/commands/ResetViewCommand.java lines 15-15:

    private boolean silent;

Example from src/main/java/yappingbot/ui/gui/MainWindow.java lines 37-37:

    private boolean updateOutput = false;

Suggestion: Follow the given naming convention for boolean variables/methods (e.g., use a boolean-sounding prefix).You may ignore the above if you think the name already follows the convention (the script can report false positives in some cases)

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/yappingbot/YappingBot.java lines 84-174:

    private void mainLoop() {
        while (ui.hasInputLines()) {

            String userInput = ui.getNextInputLine().trim();
            String[] userInputSlices = userInput.split(" ");

            try {
                switch (parser.parseCommand(userInputSlices[0])) {
                case EXIT:
                    // exits the function, ending the main loop
                    return;
                case ALIAS:
                    // creates new alias
                    // TODO: abstract this out to a Command object
                    parser.addAlias(userInputSlices[1], userInputSlices[2]);
                    ui.printf(ReplyTextMessages.ALIAS_ADDED_TEXT_2s,
                              userInputSlices[1],
                              userInputSlices[2]);
                    break;
                case RESET_LIST:
                    // resets any filter on the list
                    userList = new ResetViewCommand()
                            .setEnvironment(ui, userList, false)
                            .runCommand()
                            .getNewUserList();
                    break;
                case LIST:
                    // lists out all tasks that fits current filter (if any)
                    new PrintUserTaskListCommand()
                            .setEnvironment(ui, userList).runCommand();
                    break;
                case MARK:
                    // marks a task as Done, given the index
                    userList = new ChangeTaskListStatusCommand(userInputSlices)
                            .setEnvironment(ui, userList, true)
                            .runCommand()
                            .getNewUserList();
                    break;
                case UNMARK:
                    // unmarks a task as Done, given the index
                    userList = new ChangeTaskListStatusCommand(userInputSlices)
                            .setEnvironment(ui, userList, false)
                            .runCommand()
                            .getNewUserList();
                    break;
                case DELETE:
                    // deletes a task from list, given the index
                    userList = new DeleteTaskCommand(userInputSlices)
                            .setEnvironment(ui, userList)
                            .runCommand()
                            .getNewUserList();
                    break;
                case TODO:
                    // creates a new to-do task
                    userList = new CreateTodoCommand(userInputSlices)
                            .setEnvironment(ui, userList)
                            .runCommand()
                            .getNewUserList();
                    break;
                case EVENT:
                    // creates a new event task
                    userList = new CreateEventCommand(userInputSlices)
                            .setEnvironment(ui, userList)
                            .runCommand()
                            .getNewUserList();
                    break;
                case DEADLINE:
                    // creates a new deadline task
                    userList = new CreateDeadlineCommand(userInputSlices)
                            .setEnvironment(ui, userList)
                            .runCommand()
                            .getNewUserList();
                    break;
                case FIND:
                    // creates a filtered list view with tasks matching the given criteria
                    userList = new FindStringInTasksCommand(userInputSlices)
                            .setEnvironmen(ui, userList)
                            .runCommand()
                            .getNewUserList();
                    break;
                case UNKNOWN:
                default:
                    // catch-all for malformed commands marked UNKNOWN, or any uncaught and
                    // unimplemented comannd
                    throw new YappingBotUnknownCommandException();
                }
            } catch (YappingBotException e) {
                ui.printError(e);
            }
        }
    }

Example from src/test/java/yappingbot/tasks/DeadlineTest.java lines 103-145:

    void deserializeTest() {
        Deadline t = new Deadline();
        String input1 = "DEADLINE:a:false:2023-12-11";
        t.deserialize(input1.split(":"));
        assertEquals("a", t.getTaskName());
        assertFalse(t.isTaskDone());
        assertEquals("2023-12-11", t.getDeadline());

        String input2 = "DEADLINE:b:true:2011-12-23";
        t.deserialize(input2.split(":"));
        assertEquals("b", t.getTaskName());
        assertTrue(t.isTaskDone());
        assertEquals("2011-12-23", t.getDeadline());

        String input3 = "DEADLINE:c:true:2011-12-23:extra:data";
        t.deserialize(input3.split(":"));
        assertEquals("c", t.getTaskName());
        assertTrue(t.isTaskDone());

        String input4 = "asdlkjdlkdja:c:true:extra:data:12323123";
        YappingBotException e1 = assertThrowsExactly(YappingBotInvalidSaveFileException.class,
                                                     () -> t.deserialize(input4.split(":")));
        String expectedErrorMessage1 = "Error Reading save file! The following error was "
                                       + "encountered: No enum constant yappingbot.tasks."
                                       + "tasklist.TaskTypes." + input4.split(":")[0];
        assertEquals(expectedErrorMessage1, e1.getErrorMessage());

        String input5 = "DEADLINE:";
        YappingBotException e2 = assertThrowsExactly(YappingBotInvalidSaveFileException.class,
                                                     () -> t.deserialize(input5.split(":")));
        String expectedErrorMessage2 = "Error Reading save file! The following error was "
                                       + "encountered: Missing Data Values";
        assertEquals(expectedErrorMessage2, e2.getErrorMessage());

        String input6 = "DEADLINE:f:true:INVALID-date";
        YappingBotException e3 = assertThrowsExactly(YappingBotInvalidSaveFileException.class,
                                                     () -> t.deserialize(input6.split(":")));
        String expectedErrorMessage3 = "Error Reading save file! The following error was "
                                       + "encountered: I'm sorry, I do not understand what "
                                       + "'INVALID-date' is!\nTime value must be given in any "
                                       + "valid date-time format!";
        assertEquals(expectedErrorMessage3, e3.getErrorMessage());
    }

Example from src/test/java/yappingbot/tasks/EventTest.java lines 105-159:

    void deserializeTest() {
        Event t = new Event();
        String input1 = "EVENT:a:false:2023-12-11:2024-12-23";
        t.deserialize(input1.split(":"));
        assertEquals("a", t.getTaskName());
        assertFalse(t.isTaskDone());
        assertEquals("2023-12-11", t.getStartTime());
        assertEquals("2024-12-23", t.getEndTime());

        String input2 = "EVENT:b:true:2003-12-11:2014-12-23";
        t.deserialize(input2.split(":"));
        assertEquals("b", t.getTaskName());
        assertTrue(t.isTaskDone());
        assertEquals("2003-12-11", t.getStartTime());
        assertEquals("2014-12-23", t.getEndTime());

        String input3 = "EVENT:c:true:2011-12-23:2023-12-22:extra:data";
        t.deserialize(input3.split(":"));
        assertEquals("c", t.getTaskName());
        assertTrue(t.isTaskDone());

        String input4 = "asdlkjdlkdja:c:true:extra:data:12323123";
        YappingBotException e1 = assertThrowsExactly(YappingBotInvalidSaveFileException.class,
                                                     () -> t.deserialize(input4.split(":")));
        String expectedErrorMessage1 = "Error Reading save file! The following error was "
                                       + "encountered: No enum constant yappingbot.tasks."
                                       + "tasklist.TaskTypes." + input4.split(":")[0];
        assertEquals(expectedErrorMessage1, e1.getErrorMessage());

        String input5 = "EVENT:";
        YappingBotException e2 = assertThrowsExactly(YappingBotInvalidSaveFileException.class,
                                                     () -> t.deserialize(input5.split(":")));
        String expectedErrorMessage2 = "Error Reading save file! The following error was "
                                       + "encountered: Missing Data Values";
        assertEquals(expectedErrorMessage2, e2.getErrorMessage());

        String input6 = "EVENT:f:true:INVALID-date:INVALID-date";
        YappingBotException e3 = assertThrowsExactly(YappingBotInvalidSaveFileException.class,
                                                     () -> t.deserialize(input6.split(":")));
        String expectedErrorMessage3 = "Error Reading save file! The following error was "
                                       + "encountered: I'm sorry, I do not understand what "
                                       + "'INVALID-date' is!\nTime value must be given in any "
                                       + "valid date-time format!";

        assertEquals(expectedErrorMessage3, e3.getErrorMessage());

        String input7 = "EVENT:f:true:2023-12-12:INVALID-date";
        YappingBotException e4 = assertThrowsExactly(YappingBotInvalidSaveFileException.class,
                                                     () -> t.deserialize(input7.split(":")));
        String expectedErrorMessage4 = "Error Reading save file! The following error was "
                                       + "encountered: I'm sorry, I do not understand what "
                                       + "'INVALID-date' is!\nTime value must be given in any "
                                       + "valid date-time format!";
        assertEquals(expectedErrorMessage4, e4.getErrorMessage());
    }

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/yappingbot/Launcher.java lines 13-20:

    /**
     * MainGuiApplication entry point. Parses arguments and launches YappingBot appropriately.
     * Currently supported flags:
     * -c: use CLI instead of JavaFX GUI.
     * -s or --savefile [SAVEFILE_PATH]: use a custom savefile path.
     *
     * @param args Commandline arguments passed in.
     */

Example from src/main/java/yappingbot/YappingBot.java lines 81-83:

    /**
     * The main loop that receives and executes commands.
     */

Example from src/main/java/yappingbot/commands/CreateTaskCommandBase.java lines 61-66:

    /**
     * Set the necessary values needed to execute this command.
     *
     * @param currentUserList TaskList that needs to be resetted
     * @return this object, useful for chainning
     */

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

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 cs2103@comp.nus.edu.sg if you want to follow up on this post.