Skip to content

Sharing iP code quality feedback [for @Neilchen863] #1

@soc-se-bot

Description

@soc-se-bot

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

Example from src/main/java/hiChat/HiChat.java lines 320-321:

            else {

Suggestion: As specified by the coding standard, use egyptian style braces.

Aspect: Package Name Style

Example from src/test/java/hiChat/UiTest.java lines 1-1:

package hiChat;

Example from src/test/java/hiChat/event/TaskTest.java lines 1-1:

package hiChat.event;

Example from src/main/java/hiChat/TaskList.java lines 1-1:

package hiChat;

Suggestion: Follow the package naming convention specified by the coding standard.

Aspect: Class Name Style

No easy-to-detect issues 👍

Aspect: Dead Code

Example from src/main/java/hiChat/HiChat.java lines 170-170:

//        hiChat.TaskList taskList = new hiChat.TaskList();

Example from src/main/java/hiChat/HiChat.java lines 171-171:

//        List<Task> listOfTasks = taskList.getListOfTasks();

Example from src/main/java/hiChat/HiChat.java lines 172-172:

//        hiChat.Storage.readListFromFile(listOfTasks);

Suggestion: Remove dead code from the codebase.

Aspect: Method Length

Example from src/main/java/hiChat/Storage.java lines 40-94:

    public static void readListFromFile(List<Task> listOfTasks) {
        try {
            File file = new File("data/hiChat.txt");
            Scanner fileReader = new Scanner(file);
            while (fileReader.hasNextLine()) {
                String data = fileReader.nextLine();
                String[] splitData = data.split(" ");
                if (splitData[0].equals("[T]")) {
                    String task = "";
                    for (int i = 3; i < splitData.length; i++) {
                        task += splitData[i] + " ";
                    }
                    Task newTask = new ToDo(task);
                    if (splitData[1].equals("[X]")) {
                        newTask.markAsDone();
                    }
                    listOfTasks.add(newTask);
                } else if (splitData[0].equals("[D]")) {
                    String task = "";
                    String ddl = "";
                    int positionOfBy = 0;
                    for (int i = 3; i < splitData.length - 1; i++) {
                        if (!splitData[i].equals("(by:")) {
                            task += splitData[i] + " ";
                            positionOfBy = i;
                        } else {
                            break;
                        }
                    }
                    for (int i = positionOfBy + 2; i < splitData.length; i++) {
                        ddl += splitData[i] + " ";
                    }
                    ddl = ddl.replace("(", "").replace(")", "").trim();
                    DateTimeFormatter formatter = DateTimeFormatter.ofPattern("MMM d yyyy HH:mm");
                    LocalDateTime deadline = LocalDateTime.parse(ddl, formatter);
                    Task newTask = new Deadline(task, deadline);
                    if (splitData[1].equals("[X]")) {
                        newTask.markAsDone();
                    }
                    listOfTasks.add(newTask);
                } else if (splitData[0].equals("[E]")) {
                    if (splitData[1].equals("[X]")) {
                        listOfTasks.add(new Event(data.substring(8, data.indexOf("(") - 1), data.substring(data.indexOf("(") + 6, data.indexOf("to") - 1), data.substring(data.indexOf("to") + 4, data.length() - 1)));
                        listOfTasks.get(listOfTasks.size() - 1).markAsDone();
                    } else {
                        listOfTasks.add(new Event(data.substring(8, data.indexOf("(") - 1), data.substring(data.indexOf("(") + 6, data.indexOf("to") - 1), data.substring(data.indexOf("to") + 4, data.length() - 1)));
                    }
                }
            }
            fileReader.close();
        } catch (IOException e) {
            System.out.println("An error occurred.");
            e.printStackTrace();
        }
    }

Example from src/main/java/hiChat/HiChat.java lines 20-159:

    public String getResponse(String command) {
        if (Parser.isBye(command)) {
            return Ui.getFarewellMessage();
        }

        if (Parser.isList(command)) {
            return Ui.getListString(listOfTasks);
        }

        if (Parser.firstWord(command).equals("done")) {
            String[] splitCommand = command.split(" ");
            int taskNumber = Integer.parseInt(splitCommand[1]) - 1;
            if (taskNumber < 0 || taskNumber >= listOfTasks.size()) {
                return "☹ OOPS!!! Task number out of range.";
            }
            listOfTasks.get(taskNumber).markAsDone();
            Storage.writeListToFile(listOfTasks);
            return Ui.getMarkedAsDoneMessage(listOfTasks.get(taskNumber));
        }

        if (Parser.firstWord(command).equals("undone")) {
            String[] splitCommand = command.split(" ");
            int taskNumber = Integer.parseInt(splitCommand[1]) - 1;
            if (taskNumber < 0 || taskNumber >= listOfTasks.size()) {
                return "☹ OOPS!!! Task number out of range.";
            }
            listOfTasks.get(taskNumber).markAsUndone();
            Storage.writeListToFile(listOfTasks);
            return Ui.getMarkedAsUndoneMessage(listOfTasks.get(taskNumber));
        }

        if (Parser.firstWord(command).equals("delete")) {
            String[] splitCommand = command.split(" ");
            int taskNumber = Integer.parseInt(splitCommand[1]) - 1;
            if (taskNumber < 0 || taskNumber >= listOfTasks.size()) {
                return "☹ OOPS!!! Task number out of range.";
            }
            Task removedTask = listOfTasks.remove(taskNumber);
            Storage.writeListToFile(listOfTasks);
            return "Noted. I've removed this task:\n" +
                    "   " + removedTask + "\n" +
                    "Now you have " + listOfTasks.size() + " tasks in the list.";
        }

        if (Parser.isToDoTask(command)) {
            String[] splitCommand = command.split(" ");
            int len = splitCommand.length;
            String errorMsg = "☹ OOPS!!! The description of a todo cannot be empty.";
            try {
                if (len == 1) {
                    throw new Exception(errorMsg);
                }
            } catch (Exception e) {
                return e.getMessage();
            }
            String task = "";
            for (int i = 1; i < splitCommand.length; i++) {
                task += splitCommand[i] + " ";
            }

            listOfTasks.add(new ToDo(task));
            Storage.writeListToFile(listOfTasks);
            return "Got it. I've added this task:\n" +
                    "   " + listOfTasks.get(listOfTasks.size() - 1) + "\n" +
                    "Now you have " + listOfTasks.size() + " tasks in the list.";
        }

        if (Parser.isDeadlineTask(command)) {
            String[] splitCommand = command.split(" ");
            String task = "";
            String ddl = "";
            boolean isTask = true;
            boolean isDdl = false;

            for (int i = 1; i < splitCommand.length; i++) {
                if (splitCommand[i].equals("/by")) {
                    isTask = false;
                    isDdl = true;
                    continue;
                }

                if (isTask) {
                    task += splitCommand[i] + " ";
                } else if (isDdl) {
                    ddl += splitCommand[i] + " ";
                }
            }

            // Ensure correct date format
            DateTimeFormatter formatter = DateTimeFormatter.ofPattern("d/M/yyyy HHmm");
            LocalDateTime deadline = LocalDateTime.parse(ddl.trim(), formatter);

            Task newTask = new Deadline(task, deadline);
            listOfTasks.add(newTask);
            Storage.writeListToFile(listOfTasks);
            return "Got it. I've added this task:\n" +
                    "   " + listOfTasks.get(listOfTasks.size() - 1) + "\n" +
                    "Now you have " + listOfTasks.size() + " tasks in the list.";
        }

        if (Parser.isEventTask(command)) {
            String[] splitCommand = command.split(" ");
            String task = "";
            String startTime = "";
            String endTime = "";
            boolean isTask = true;
            boolean isStartTime = false;
            boolean isEndTime = false;

            for (int i = 1; i < splitCommand.length; i++) {
                if (splitCommand[i].equals("/from")) {
                    isTask = false;
                    isStartTime = true;
                    continue;
                }

                if (splitCommand[i].equals("/to")) {
                    isStartTime = false;
                    isEndTime = true;
                    continue;
                }

                if (isTask) {
                    task += splitCommand[i] + " ";
                } else if (isStartTime) {
                    startTime += splitCommand[i] + " ";
                } else if (isEndTime) {
                    endTime += splitCommand[i] + " ";
                }
            }
            Task newTask = new Event(task, startTime, endTime);
            listOfTasks.add(newTask);
            Storage.writeListToFile(listOfTasks);
            return "Got it. I've added this task:\n" +
                    "   " + listOfTasks.get(listOfTasks.size() - 1) + "\n" +
                    "Now you have " + listOfTasks.size() + " tasks in the list.";
        }

        return "Sorry, I don't understand that command.";
    }

Example from src/main/java/hiChat/HiChat.java lines 161-330:

    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);
        Ui.printGreeting();





//
//        hiChat.TaskList taskList = new hiChat.TaskList();
//        List<Task> listOfTasks = taskList.getListOfTasks();
//        hiChat.Storage.readListFromFile(listOfTasks);

        while (true) {
            String command = scanner.nextLine();
            if (Parser.isBye(command)) {
                Ui.printFarewell();
                break;
            }

            if (Parser.isList(command)) {
                Ui.printList(listOfTasks);
                continue;
            }

            if (Parser.firstWord(command).equals("done")) {
                String[] splitCommand = command.split(" ");
                int taskNumber = Integer.parseInt(splitCommand[1]);
                listOfTasks.get(taskNumber - 1).markAsDone();
                Ui.printMarkedAsDone(listOfTasks.get(taskNumber - 1));
                Storage.writeListToFile(listOfTasks);
                continue;
            }

            if (Parser.firstWord(command).equals("undone")) {
                String[] splitCommand = command.split(" ");
                int taskNumber = Integer.parseInt(splitCommand[1]);
                listOfTasks.get(taskNumber - 1).markAsUndone();
                Ui.printMarkedAsUndone(listOfTasks.get(taskNumber - 1));
                Storage.writeListToFile(listOfTasks);
                continue;
            }

            if (Parser.firstWord(command).equals("delete")) {
                String[] splitCommand = command.split(" ");
                int taskNumber = Integer.parseInt(splitCommand[1]);
                System.out.println("____________________________________________________________\n" +
                        " Noted. I've removed this task:\n" +
                        "   " + listOfTasks.get(taskNumber - 1) + "\n" +
                        " Now you have " + (listOfTasks.size() - 1) + " tasks in the list.\n" +
                        "____________________________________________________________\n");
                listOfTasks.remove(taskNumber - 1);
                Storage.writeListToFile(listOfTasks);
                continue;
            }


            if (Parser.isToDoTask(command)) {
                String[] splitCommand = command.split(" ");
                int len = splitCommand.length;
                String errorMsg = "____________________________________________________________\n" +
                        "☹ OOPS!!! The description of a todo cannot be empty." + "\n" +
                        "____________________________________________________________\n";
                try {
                    if (len == 1) {
                        throw new Exception(errorMsg);
                    }
                } catch (Exception e) {
                    System.out.println(e.getMessage());
                    continue;
                }
                String task = "";
                for (int i = 1; i < splitCommand.length; i++) {
                    task += splitCommand[i] + " ";
                }

                listOfTasks.add(new ToDo(task));
                Storage.writeListToFile(listOfTasks);
                Ui.printAddedTask(listOfTasks.get(listOfTasks.size() - 1), listOfTasks);
            }

            else if (Parser.isDeadlineTask(command)) {
                String[] splitCommand = command.split(" ");
                String task = "";
                String ddl = "";
                boolean isTask = true;
                boolean isDdl = false;

                for (int i = 1; i < splitCommand.length; i++) {
                    if (splitCommand[i].equals("/by")) {
                        isTask = false;
                        isDdl = true;
                        continue;
                    }

                    if (isTask) {
                        task += splitCommand[i] + " ";
                    } else if (isDdl) {
                        ddl += splitCommand[i] + " ";
                    }
                }

                // Ensure correct date format
                DateTimeFormatter formatter = DateTimeFormatter.ofPattern("d/M/yyyy HHmm");
                LocalDateTime deadline = LocalDateTime.parse(ddl.trim(), formatter);

                Task newTask = new Deadline(task, deadline);
                listOfTasks.add(newTask);
                Storage.writeListToFile(listOfTasks);
                Ui.printAddedTask(listOfTasks.get(listOfTasks.size() - 1), listOfTasks);
            }

            else if (Parser.isEventTask(command)) {
                String[] splitCommand = command.split(" ");
                String task = "";
                String startTime = "";
                String endTime = "";
                boolean isTask = true;
                boolean isStartTime = false;
                boolean isEndTime = false;

                for (int i = 1; i < splitCommand.length; i++) {
                    if (splitCommand[i].equals("/from")) {
                        isTask = false;
                        isStartTime = true;
                        continue;
                    }

                    if (splitCommand[i].equals("/to")) {
                        isStartTime = false;
                        isEndTime = true;
                        continue;
                    }

                    if (isTask) {
                        task += splitCommand[i] + " ";
                    } else if (isStartTime) {
                        startTime += splitCommand[i] + " ";
                    } else if (isEndTime) {
                        endTime += splitCommand[i] + " ";
                    }
                }
                Task newTask = new Event(task, startTime, endTime);
                listOfTasks.add(newTask);
                Storage.writeListToFile(listOfTasks);
                Ui.printAddedTask(listOfTasks.get(listOfTasks.size() - 1), listOfTasks);
            }

            else if (Parser.isFindTask(command)) {
                String[] splitCommand = command.split(" ");
                String keyword = splitCommand[1];
                List<Task> foundTasks = new ArrayList<>();
                for (Task task : listOfTasks) {
                    if (task.getTask().contains(keyword)) {
                        foundTasks.add(task);
                    }
                }
                Ui.printFoundTasks(foundTasks);
            }

            else {
                Ui.printSorry();
                continue;
            }


        }

        scanner.close();
    }

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/hiChat/Parser.java lines 7-11:

    /**
     * Parse the input string
     * @param input
     * @return input
     */

Example from src/main/java/hiChat/Parser.java lines 16-20:

    /**
     * Get the first word of the input string
     * @param input
     * @return first word of the input string
     */

Example from src/main/java/hiChat/Parser.java lines 25-29:

    /**
     * Get the second word of the input string
     * @param input
     * @return second word of the input string
     */

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

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