Sharing iP code quality feedback [for @yuto1115]

[soc-se-bot-blue]

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

Example from src/main/java/odin/parser/Parser.java lines 58-58:

                Boolean finished = command instanceof ExitCommand;

Example from src/main/java/odin/task/Task.java lines 14-14:

    private boolean done;

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/test/java/odin/parser/ParserTest.java lines 174-220:

    public void parseAndHandle_findCommand() {
        // ok, found
        try {
            ArrayList<String> tokens = new ArrayList<>(Arrays.asList("find", "book"));
            TaskList taskList = new TaskListStub();
            Pair<Boolean, ArrayList<String>> p = new Parser().parseAndHandle(tokens, taskList);
            assertEquals(false, p.getKey());
            assertEquals(new ArrayList<>(Arrays.asList(
                    "These are the tasks that contain the keyword(s) 'book'.", "1. stub")), p.getValue());
        } catch (WrongFormatException e) {
            fail();
        }

        // ok, found 2
        try {
            ArrayList<String> tokens = new ArrayList<>(Arrays.asList("find", "read", "boo"));
            TaskList taskList = new TaskListStub();
            Pair<Boolean, ArrayList<String>> p = new Parser().parseAndHandle(tokens, taskList);
            assertEquals(false, p.getKey());
            assertEquals(new ArrayList<>(Arrays.asList(
                    "These are the tasks that contain the keyword(s) 'read boo'.", "1. stub")), p.getValue());
        } catch (WrongFormatException e) {
            fail();
        }

        // ok, not found
        try {
            ArrayList<String> tokens = new ArrayList<>(Arrays.asList("find", "real", "book"));
            TaskList taskList = new TaskListStub();
            Pair<Boolean, ArrayList<String>> p = new Parser().parseAndHandle(tokens, taskList);
            assertEquals(false, p.getKey());
            assertEquals(new ArrayList<>(List.of(
                    "There are no tasks that contain the keyword(s) 'real book'.")), p.getValue());
        } catch (WrongFormatException e) {
            fail();
        }

        // bad
        try {
            ArrayList<String> tokens = new ArrayList<>(List.of("find"));
            TaskList taskList = new TaskListStub();
            new Parser().parseAndHandle(tokens, taskList);
            fail();
        } catch (WrongFormatException e) {
            assertEquals("KEYWORDS cannot be empty.", e.getMessage());
        }
    }

Example from src/test/java/odin/parser/ParserTest.java lines 361-406:

    public void parseAndHandle_addDeadlineCommand() {
        // ok
        try {
            ArrayList<String> tokens = new ArrayList<>(Arrays.asList(
                    "deadline", "read", "book", "/by", "2030-01-01", "12:00"));
            TaskList taskList = new TaskListStubDeadline();
            Pair<Boolean, ArrayList<String>> p = new Parser().parseAndHandle(tokens, taskList);
            assertEquals(false, p.getKey());
            assertEquals(new ArrayList<>(Arrays.asList("This task has been added to the list.",
                    "  deadline stub", "Now, 2 tasks stand before you. Choose wisely, for time is ever fleeting.")),
                    p.getValue());
        } catch (WrongFormatException e) {
            fail();
        }

        // bad
        try {
            ArrayList<String> tokens = new ArrayList<>(Arrays.asList(
                    "deadline", "read", "book", "2030-01-01", "12:00"));
            TaskList taskList = new TaskListStubDeadline();
            new Parser().parseAndHandle(tokens, taskList);
            fail();
        } catch (WrongFormatException e) {
            assertEquals("'/by not found.", e.getMessage());
        }

        // bad 2
        try {
            ArrayList<String> tokens = new ArrayList<>(Arrays.asList("deadline", "/by", "2030-01-01", "12:00"));
            TaskList taskList = new TaskListStubDeadline();
            new Parser().parseAndHandle(tokens, taskList);
            fail();
        } catch (WrongFormatException e) {
            assertEquals("TASK cannot be empty.", e.getMessage());
        }

        // bad 3
        try {
            ArrayList<String> tokens = new ArrayList<>(Arrays.asList("deadline", "read", "book", "/by"));
            TaskList taskList = new TaskListStubDeadline();
            new Parser().parseAndHandle(tokens, taskList);
            fail();
        } catch (WrongFormatException e) {
            assertEquals("DATE cannot be empty.", e.getMessage());
        }
    }

Example from src/test/java/odin/parser/ParserTest.java lines 409-491:

    public void parseAndHandle_addEventCommand() {
        // ok
        try {
            ArrayList<String> tokens = new ArrayList<>(Arrays.asList(
                    "event", "read", "book", "/from", "2030-01-01", "12:00", "/to", "2040-01-01", "12:00"));
            TaskList taskList = new TaskListStubEvent();
            Pair<Boolean, ArrayList<String>> p = new Parser().parseAndHandle(tokens, taskList);
            assertEquals(false, p.getKey());
            assertEquals(new ArrayList<>(Arrays.asList(
                    "This task has been added to the list.",
                    "  event stub", "Now, 2 tasks stand before you. Choose wisely, for time is ever fleeting.")),
                    p.getValue());
        } catch (WrongFormatException e) {
            System.out.println(e.getMessage());
            fail();
        }

        // bad
        try {
            ArrayList<String> tokens = new ArrayList<>(Arrays.asList(
                    "event", "read", "book", "2030-01-01", "12:00", "/to", "2040-01-01", "12:00"));
            TaskList taskList = new TaskListStubEvent();
            new Parser().parseAndHandle(tokens, taskList);
            fail();
        } catch (WrongFormatException e) {
            assertEquals("/from not found.", e.getMessage());
        }

        // bad 2
        try {
            ArrayList<String> tokens = new ArrayList<>(Arrays.asList(
                    "event", "read", "book", "/from", "2030-01-01", "12:00", "2040-01-01", "12:00"));
            TaskList taskList = new TaskListStubEvent();
            new Parser().parseAndHandle(tokens, taskList);
            fail();
        } catch (WrongFormatException e) {
            assertEquals("/to not found.", e.getMessage());
        }

        // bad 3
        try {
            ArrayList<String> tokens = new ArrayList<>(Arrays.asList(
                    "event", "read", "book", "/to", "2030-01-01", "12:00", "/from", "2040-01-01", "12:00"));
            TaskList taskList = new TaskListStubEvent();
            new Parser().parseAndHandle(tokens, taskList);
            fail();
        } catch (WrongFormatException e) {
            assertEquals("/from must come before /to.", e.getMessage());
        }

        // bad 4
        try {
            ArrayList<String> tokens = new ArrayList<>(Arrays.asList(
                    "event", "/from", "2030-01-01", "12:00", "/to", "2040-01-01", "12:00"));
            TaskList taskList = new TaskListStubEvent();
            new Parser().parseAndHandle(tokens, taskList);
            fail();
        } catch (WrongFormatException e) {
            assertEquals("TASK cannot be empty.", e.getMessage());
        }

        // bad 5
        try {
            ArrayList<String> tokens = new ArrayList<>(Arrays.asList(
                    "event", "read", "book", "/from", "/to", "2040-01-01", "12:00"));
            TaskList taskList = new TaskListStubEvent();
            new Parser().parseAndHandle(tokens, taskList);
            fail();
        } catch (WrongFormatException e) {
            assertEquals("DATE cannot be empty.", e.getMessage());
        }

        // bad 6
        try {
            ArrayList<String> tokens = new ArrayList<>(Arrays.asList(
                    "event", "read", "book", "/from", "2030-01-01", "12:00", "/to"));
            TaskList taskList = new TaskListStubEvent();
            new Parser().parseAndHandle(tokens, taskList);
            fail();
        } catch (WrongFormatException e) {
            assertEquals("DATE cannot be empty.", e.getMessage());
        }
    }

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/odin/Odin.java lines 34-36:

    /**
     * Repeatedly asks user for commands and processes them appropriately.
     */

Example from src/main/java/odin/storage/Storage.java lines 66-70:

    /**
     * Save the data of task list to the record file.
     *
     * @param taskList Task list to be saved.
     */

Example from src/main/java/odin/task/TaskList.java lines 27-29:

    /**
     * Add a new task to the back of the list.
     */

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 Messages

possible problems in commit dcc9f29:

Add a suppression in suppressions.xml: suppress MethodName warning for test files

• Longer than 72 characters

Suggestion: Follow the given conventions for Git commit messages for future commits (do not modify past commit messages as doing so will change the commit timestamp that we used to detect your commit timings).

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