Sharing iP code quality feedback [for @HakkaNgin]

[soc-se-script]

@HakkaNgin 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/pixel/Pixel.java lines 15-15:

    private boolean justInitialised = true;

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

Example from src/main/java/pixel/task/Task.java lines 66-66:

                // System.out.println("date is working");

Example from src/main/java/pixel/task/Task.java lines 70-70:

                // System.out.println("time input is working");

Example from src/main/java/pixel/task/Task.java lines 74-74:

                // System.out.println("time output is working");

Suggestion: Remove dead code from the codebase.

Aspect: Method Length

Example from src/main/java/pixel/task/Task.java lines 54-108:

    private String dateTimeProcessing(String due) {
        String[] tempStringArray = due.strip().split(" ", 2);

        try {
            String dueDate = tempStringArray[0];
            String dueTime = tempStringArray.length < 2 ? "" : tempStringArray[1];

            if (validator.isValid(dueDate)) {
                LocalDate inputDue = LocalDate.parse(dueDate);
                String year = String.valueOf(inputDue.getYear());
                String month = String.valueOf(inputDue.getMonth());
                String date = String.valueOf(inputDue.getDayOfMonth());
                // System.out.println("date is working");

                //time pattern of input date in 24 hour format -- HH for 24h, hh for 12h
                DateFormat timeFormat = new SimpleDateFormat("HHmm");
                // System.out.println("time input is working");

                //Date/time pattern of desired output date
                DateFormat outputFormat = new SimpleDateFormat("hh:mm aa"); // aa for AM/ PM
                // System.out.println("time output is working");
                Date oldTimeFormat = timeFormat.parse(dueTime);
                String finalTimeFormat = outputFormat.format(oldTimeFormat);

                return month + " " + date + " " + year + " " + finalTimeFormat;

            } else {
                System.out.println("(Note: Due is not in yyyy-MM-dd(SPACE)HHmm format)");
                return due;
            }

        } catch (DateTimeParseException e) {
            // System.out.println(e);
            return ("Please ensure that your date & time input are in yyyy-MM-dd(SPACE)HHmm(24h) format \n"
                + UserInterface.PROMPT_MESSAGE);

        } catch (IndexOutOfBoundsException e) {
            // System.out.println(e);
            return ("Please ensure that you have entered both date and time in yyyy-MM-dd(SPACE)HHmm(24h) format \n"
                + UserInterface.PROMPT_MESSAGE);

        } catch (ParseException e) {
           //  System.out.println(e);
            return ("Caught parse exception! \n"
                + UserInterface.AFTER_INVALID_INPUT + "\n"
                + UserInterface.PROMPT_MESSAGE);

        } catch (Exception e) {
            // System.out.println(e);
            return ("Some other error occurred \n"
                + UserInterface.AFTER_INVALID_INPUT + "\n"
                + UserInterface.PROMPT_MESSAGE);

        }
    }

Example from src/main/java/pixel/util/Parser.java lines 46-151:

    public String parse(String userInput) {

        String strippedInput = userInput.strip();

        try {
            if (userInput.strip().startsWith("bye")) {
                return UserInterface.GOODBYE_MESSAGE;
                // System.exit(0);

            } else if (userInput.strip().startsWith("todo ")) {
                return taskList.handleNewTask(userInput, "T");

            } else if (userInput.strip().startsWith("deadline ")) {
                return taskList.handleNewTask(userInput, "D");

            } else if (userInput.strip().startsWith("event ")) {
                return taskList.handleNewTask(userInput, "E");

            } else if (strippedInput.startsWith("mark ")) {
                int indexToChange = getMarkOrUnmarkIndex(strippedInput, Marking.MARK);

                if ((indexToChange < 1) || (indexToChange > 100)) {
                    throw new IndexOutOfBoundsException("Only index 1 to 100 are supported");
                }

                Storage.INPUT_TASKS.get(indexToChange - 1).markAsDone();
                Storage.resetFile(this.filePath);
                Storage.appendAllTasksToFile(this.filePath);

                return (" Nice! I've marked this task as done: \n"
                    + Storage.INPUT_TASKS.get(indexToChange - 1) + "\n"
                    + UserInterface.AFTER_VALID_INPUT);

            } else if (strippedInput.startsWith("unmark ")) {
                int indexToChange = getMarkOrUnmarkIndex(strippedInput, Marking.UNMARK);

                if ((indexToChange < 1) || (indexToChange > 100)) {
                    throw new IndexOutOfBoundsException("Only index 1 to 100 are supported");
                }

                Storage.INPUT_TASKS.get(indexToChange - 1).markAsNotDone();
                Storage.resetFile(this.filePath);
                Storage.appendAllTasksToFile(this.filePath);

                return ("OK, I've marked this task as not done yet: \n"
                    + Storage.INPUT_TASKS.get(indexToChange - 1) + "\n"
                    + UserInterface.AFTER_VALID_INPUT);

            } else if (userInput.strip().equals("list")) {
                String listOfTasks = this.taskList.listTasks();
                return listOfTasks + UserInterface.AFTER_VALID_INPUT;

            } else if (userInput.strip().startsWith("delete ")) {
                String output = Storage.deleteEntry(userInput, filePath);
                return output + "\n" + UserInterface.AFTER_VALID_INPUT;

            } else if (userInput.strip().startsWith("find ")) {
                ArrayList<Task> results = Storage.findEntry(userInput);
                String findResults = this.taskList.listFindResults(results);
                return findResults + UserInterface.AFTER_VALID_INPUT;

            } else {
                throw new IncorrectFormatException("Input should be a task or command!"); // programme breaks
            }

        } catch (IndexOutOfBoundsException e) {
            return (e + "\n"
                + "caught Index Out of Bounds Exception \n"
                + UserInterface.AFTER_INVALID_INPUT + "\n"
                + UserInterface.PROMPT_MESSAGE);

        } catch (StackOverflowError e) {
            return ("caught Stack Overflow Error \n"
                + UserInterface.AFTER_INVALID_INPUT + "\n"
                + UserInterface.PROMPT_MESSAGE);

        } catch (NullPointerException e) {
            return ("caught Null pointer exception \n"
                + UserInterface.AFTER_INVALID_INPUT + "\n"
                + UserInterface.PROMPT_MESSAGE);

        } catch (IncorrectFormatException e) {
            return (e + "\n"
                + "Incorrect format exception! \n"
                + UserInterface.AFTER_INVALID_INPUT + "\n"
                + UserInterface.PROMPT_MESSAGE);

        } catch (IOException e) {

            if (e instanceof FileNotFoundException) {
                File tempFile = new File("./data/pixel", "pixel.txt");
                return ("Caught FileNotFound exception! \n"
                    + "New file is created for you \n"
                    + UserInterface.PROMPT_MESSAGE);
            } else {
                return ("Caught IO exception! \n"
                    + UserInterface.PROMPT_MESSAGE);
            }

        } catch (DuplicateEntryException e) {
            return (e + "\n"
                + "You may want to add a different task :) \n"
                + UserInterface.AFTER_INVALID_INPUT + "\n"
                + UserInterface.PROMPT_MESSAGE);
        }
    }

Example from src/main/java/pixel/util/TaskList.java lines 50-116:

    public String handleNewTask(String userInput, String type) throws IOException, DuplicateEntryException {

        int indexOfSlash = userInput.indexOf("/"); // returns -1 if such a string doesn't exist
        // If there's a "/by" or "/at" in the input string, then the info behind the "/by" or "/at" is the due
        // if there's no "/by" and "/at" string, then due should be empty
        String due = indexOfSlash == -1 ? "" : userInput.substring(indexOfSlash + 4);
        int indexOfEndOfDescription = indexOfSlash == -1 ? userInput.length() : indexOfSlash;
        Task newTask;
        String commandWord = "";

        if (indexOfSlash != -1) {
            if (userInput.substring(indexOfSlash + 1).startsWith("by")) {
                commandWord = "by";
            } else if (userInput.substring(indexOfSlash + 1).startsWith("at")) {
                commandWord = "at";
            } else {
                throw new IncorrectFormatException("Slash should be followed by \"by\" or \"at\"!"); // programme breaks
            }
        }

        switch (type) {
        case "T": { // todo
            String description = userInput.substring(5, indexOfEndOfDescription).strip();
            newTask = new ToDo(description, due, commandWord); // Stores user input

            break;
        }
        case "D": { // deadline
            String description = userInput.substring(9, indexOfEndOfDescription).strip();
            newTask = new Deadline(description, due, commandWord); // Stores user input

            break;
        }
        case "E": { // event
            String description = userInput.substring(6, indexOfEndOfDescription).strip();
            newTask = new Event(description, due, commandWord); // Stores user input

            break;
        }
        default: //shouldn't reach here
            throw new IncorrectFormatException("Incorrect format of input!"); // programme breaks
        }

        // CHECK IF TASK ALREADY EXISTS
        // If yes, throw exception
        if (findDuplicate(newTask)) {
            throw new DuplicateEntryException("Same task already exists in database!");
        }
        Storage.INPUT_TASKS.add(Pixel.count, newTask);

        // index of last element in ArrayList is always smaller than size
        assert Storage.INPUT_TASKS.size() == (Pixel.count + 1)
            : "Size of ArrayList did not increase by 1 after adding new task";

        // Not so efficient method
        // first delete existing content in old file
        Storage.resetFile(this.filePath);

        // run through all the files in the list and update pixel.txt accordingly
        Storage.appendAllTasksToFile(this.filePath);

        Pixel.count += 1;

        return ("Got it. I've added this task: \n"
            + newTask + "\n"
            + "Now you have " + Pixel.count + " tasks in the list.");
    }

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/pixel/Pixel.java lines 44-47:

    /**
     * You should have your own function to generate a response to user input.
     * Replace this stub with your completed method.
     */

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)

possible problems in commit 17b15b2:

C-Detect Duplicates: Add the ability to recognize and deal with duplicate items. e.g., the same task added multiple times.

• Longer than 72 characters

possible problems in commit e7562d3:

Resolved some branching conflicts

• Not in imperative mood (?)

possible problems in commit c5938bc:

Added DuplicateEntryException to handle duplicate entries

• Not in imperative mood (?)

Suggestion: Follow the given conventions for Git commit messages for future commits (no need to modify past commit messages).

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.