Skip to content

Sharing iP code quality feedback [for @hansneddyanto] #3

Description

@nus-se-script

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

No easy-to-detect issues 👍

Aspect: Method Length

Example from src/main/java/hana/Parser.java lines 25-138:

    public static Command parse(String fullCommand) throws HanaException {
        if (fullCommand == null || fullCommand.isEmpty()) {
            throw new HanaException("You must specify a command");
        }
        String[] parts = fullCommand.split(" ", 2);
        String commandWord = parts[0];
        int taskNumber;
        switch (commandWord) {
        case "list":
            return new ListCommand();
        case "mark":
            Pattern markPattern = Pattern.compile("^mark (\\d+)$");
            Matcher markMatcher = markPattern.matcher(fullCommand);
            if (!markMatcher.matches()) {
                throw new HanaException("Invalid mark syntax. Write only the task index after the word 'mark'.");
            }
            taskNumber = Integer.parseInt(markMatcher.group(1)) - 1;
            assert taskNumber >= 0 : "Task number should be non-negative";
            return new MarkCommand(taskNumber);
        case "unmark":
            Pattern unmarkPattern = Pattern.compile("^unmark (\\d+)$");
            Matcher unmarkMatcher = unmarkPattern.matcher(fullCommand);
            if (!unmarkMatcher.matches()) {
                throw new HanaException("Invalid unmark syntax. Write only the task index after the word 'unmark'.");
            }
            taskNumber = Integer.parseInt(unmarkMatcher.group(1)) - 1;
            assert taskNumber >= 0 : "Task number should be non-negative";
            return new UnmarkCommand(taskNumber);
        case "todo":
            if (parts.length < 2 || parts[1].trim().isEmpty()) {
                throw new HanaException("OOPS!!! The description of a todo cannot be empty.");
            }
            return new AddCommand(new ToDo(parts[1]));
        case "deadline":
            if (parts.length < 2 || parts[1].trim().isEmpty()) {
                throw new HanaException("OOPS!!! The description of a deadline cannot be empty.");
            }
            String[] deadlineParts = parts[1].split(" /by ");
            if (fullCommand.trim().equals("deadline") || fullCommand.startsWith("deadline /by")) {
                throw new HanaException("OOPS!!! The description of a deadline cannot be empty.");
            } else if (!fullCommand.contains(" /by ")) {
                throw new HanaException("OOPS!!! Please add deadline date/time by adding /by");
            }
            try {
                LocalDate dateBy = LocalDate.parse(deadlineParts[1]);
                deadlineParts[1] = dateBy.format(DateTimeFormatter.ofPattern("MMM dd yyyy"));
            } catch (DateTimeParseException e) {
                throw new HanaException("OOPS!!! The date format is incorrect. Please use the format YYYY-MM-DD.");
            }
            return new AddCommand(new Deadline(deadlineParts[0], deadlineParts[1]));
        case "event":
            if (parts.length < 2 || parts[1].trim().isEmpty()) {
                throw new HanaException("OOPS!!! The description of an event cannot be empty.");
            }
            String[] eventParts = parts[1].split(" /from | /to ");
            if (fullCommand.trim().equals("event") || fullCommand.startsWith("event /from")) {
                throw new HanaException("OOPS!!! The description of an event cannot be empty.");
            } else if (!fullCommand.contains(" /from ") || !fullCommand.contains(" /to ")) {
                throw new HanaException("OOPS!!! Please add event time by adding /from and /to");
            } else if (fullCommand.indexOf(" /to ") < fullCommand.indexOf(" /from ")) {
                throw new HanaException("OOPS!!! Please add /from before /to");
            }
            try {
                LocalDate dateFrom = LocalDate.parse(eventParts[1]);
                LocalDate dateTo = LocalDate.parse(eventParts[2]);
                if (dateFrom.isAfter(dateTo)) {
                    throw new HanaException("OOPS!!! from date cannot be after to date.");
                }
                assert !dateFrom.isAfter(dateTo) : "Event start date cannot be after end date";
                eventParts[1] = dateFrom.format(DateTimeFormatter.ofPattern("MMM dd yyyy"));
                eventParts[2] = dateTo.format(DateTimeFormatter.ofPattern("MMM dd yyyy"));
            } catch (DateTimeParseException e) {
                throw new HanaException("OOPS!!! The date format is incorrect. \nPlease use the format YYYY-MM-DD.");
            }
            return new AddCommand(new Event(eventParts[0], eventParts[1], eventParts[2]));
        case "delete":
            Pattern deletePattern = Pattern.compile("^delete (\\d+)$");
            Matcher deleteMatcher = deletePattern.matcher(fullCommand);
            if (!deleteMatcher.matches()) {
                throw new HanaException("Invalid delete syntax. Write only the task index after the word 'delete'.");
            }
            taskNumber = Integer.parseInt(parts[1]) - 1;
            assert taskNumber >= 0 : "Task number should be non-negative";
            return new DeleteCommand(taskNumber);
        case "find":
            if (parts.length < 2 || parts[1].trim().isEmpty()) {
                throw new HanaException("OOPS!!! The search keyword cannot be empty.");
            }
            assert parts.length == 2 : "Find command should have a keyword";
            return new FindCommand(parts[1]);
        case "massMark":
            if (parts.length < 2 || parts[1].trim().isEmpty()) {
                throw new HanaException("OOPS!!! You must provide a keyword for massMark.");
            }
            assert parts.length == 2 : "Find command should have a keyword";
            return new MassMarkCommand(parts[1]);
        case "massUnmark":
            if (parts.length < 2 || parts[1].trim().isEmpty()) {
                throw new HanaException("OOPS!!! You must provide a keyword for massUnmark.");
            }
            assert parts.length == 2 : "Find command should have a keyword";
            return new MassUnmarkCommand(parts[1]);
        case "massDelete":
            if (parts.length < 2 || parts[1].trim().isEmpty()) {
                throw new HanaException("OOPS!!! You must provide a keyword for massDelete.");
            }
            assert parts.length == 2 : "Find command should have a keyword";
            return new MassDeleteCommand(parts[1]);
        case "bye":
            return new ExitCommand();
        default:
            throw new HanaException("OOPS!!! I'm sorry, but I don't know what that means :-(");
        }
    }

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

No easy-to-detect issues 👍

Aspect: Recent Git Commit Message

possible problems in commit 5721a70:


Improve code styling by reducing nested statements in method getResponse(String input)


  • Longer than 72 characters

possible problems in commit 7acef8e:


Add support for mass operations like mark, unmark, and delete tasks

The current system requires users to perform task operations one at a
time, making it inefficient to manage a large number of tasks.

Allowing users to perform operations on multiple tasks in one go
improves the efficiency of task management, especially when dealing
with large lists.

Implement mass operations by adding `MassMarkCommand`, `MassUnmarkCommand`,
and `MassDeleteCommand` to handle bulk task actions. These commands
search for tasks based on a keyword, display the matching tasks, and
prompt the user for confirmation before marking, unmarking, or deleting
tasks in bulk.

This approach ensures that users have control over bulk actions through
a confirmation process, reducing the chance of accidental changes.
It also maintains consistency with the existing command processing
flow, ensuring the user experience remains intuitive.

Multi-step user interactions are handled by storing the command state
until the user confirms the action, enabling a smooth multi-step
operation for bulk task management.


  • body not wrapped at 72 characters: e.g., Implement mass operations by adding MassMarkCommand, MassUnmarkCommand,

possible problems in commit 82370ce:


Merge branch 'master' of https://github.com/hansneddyanto/ip

* 'master' of https://github.com/hansneddyanto/ip:
  Codebase contains inconsistent indentation, redundant variables, and non-uniform variable names


  • body not wrapped at 72 characters: e.g., Codebase contains inconsistent indentation, redundant variables, and non-uniform variable names

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions