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

[soc-se-script]

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

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/hichat/Storage.java lines 48-161:

    public static void readListFromFile(TaskList listOfTasks) {
        try {
            File dir = new File("data");
            if (!dir.exists()) {
                dir.mkdirs();
            }

            File file = new File("data/hiChat.txt");

            if (!file.exists()) {
                file.createNewFile();
            }

            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();
                    }
                    // check if task is priority
                    if (splitData[2].equals("[P]")) {
                        newTask.setIsPriority(true);
                    }
                    listOfTasks.add(newTask);
                } else if (splitData[0].equals("[D]")) {
                    // Ensure splitData has enough elements
                    if (splitData.length < 6) {
                        throw new IllegalArgumentException("Invalid input format: " + String.join(" ", splitData));
                    }

                    // Extract task description (everything before "(by:")
                    StringBuilder taskBuilder = new StringBuilder();
                    int deadlineIndex = -1; // To locate "(by:"
                    for (int i = 5; i < splitData.length; i++) {
                        if (splitData[i].startsWith("(by:")) {
                            deadlineIndex = i;
                            break;
                        }
                        taskBuilder.append(splitData[i]).append(" ");
                    }

                    // Ensure deadline marker "(by:" exists
                    if (deadlineIndex == -1) {
                        throw new IllegalArgumentException("Missing deadline marker '(by:' in input: " + String.join(" ", splitData));
                    }

                    String task = taskBuilder.toString().trim();

                    String task0 = data.substring(12, data.indexOf("(by:") - 1);
                    // Extract raw deadline part (after "(by:")
                    StringBuilder ddlBuilder = new StringBuilder();
                    for (int i = deadlineIndex + 1; i < splitData.length; i++) {
                        ddlBuilder.append(splitData[i]).append(" ");
                    }

                    String ddl = ddlBuilder.toString().trim();

                    // Ensure deadline format is valid
                    if (ddl.endsWith(")")) {
                        ddl = ddl.substring(0, ddl.length() - 1); // Remove closing ")"
                    } else {
                        throw new IllegalArgumentException("Unexpected deadline format: " + ddl);
                    }



                    // Define date format
                    DateTimeFormatter formatter = DateTimeFormatter.ofPattern("MMM d yyyy HH:mm");

                    // Parse deadline
                    LocalDateTime deadline;
                    try {
                        deadline = LocalDateTime.parse(ddl, formatter);
                    } catch (DateTimeParseException e) {
                        throw new IllegalArgumentException("Failed to parse deadline: " + ddl, e);
                    }

                    // Create task object
                    Task newTask = new Deadline(task0, deadline);

                    // Check priority
                    if (splitData[1].equals("[P]")) {
                        newTask.setIsPriority(true);
                    }

                    // Check if marked as done
                    if (splitData[2].equals("[X]")) {
                        newTask.markAsDone();
                    }

                    listOfTasks.add(newTask);
                } else if (splitData[0].equals("[E]")) {
                    listOfTasks.add(new Event(data.substring(12, data.indexOf("(") - 1), data.substring(data.indexOf("(") + 7, data.indexOf("to") - 1), data.substring(data.indexOf("to") + 4, data.length() - 1)));
                    if (splitData[1].equals("[P]")) {
                        listOfTasks.get(listOfTasks.size() - 1).setIsPriority(true);
                    }
                    if (splitData[1].equals("[X]")) {
                        listOfTasks.get(listOfTasks.size() - 1).markAsDone();
                    }
                }
            }
            fileReader.close();
        } catch (IOException e) {
            System.out.println("An error occurred.");
            e.printStackTrace();
        }
    }

Example from src/main/java/hichat/HiChat.java lines 19-185:

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

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

        if (Parser.isMark(command)) {
            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.isUnmark(command)) {
            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.isDelete(command)) {
            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.";
        }

        if (Parser.isPrioritizeTask(command)) {
            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).setIsPriority(true);
            // put it on top of the list
            Task task = listOfTasks.remove(taskNumber);
            listOfTasks.add(0, task);
            Storage.writeListToFile(listOfTasks);
            return "Noted. I've prioritized this task:\n" +
                    "   " + listOfTasks.get(0);
        }

        if (Parser.isUnPrioritizeTask(command)) {
            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).setIsPriority(false);
            Storage.writeListToFile(listOfTasks);
            return "Noted. I've un-prioritized this task:\n" +
                    "   " + listOfTasks.get(taskNumber);
        }

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

Example from src/main/java/hichat/Main.java lines 27-90:

    public void start(Stage stage) {
        //Setting up required components

        scrollPane = new ScrollPane();
        dialogContainer = new VBox();
        scrollPane.setContent(dialogContainer);

        userInput = new TextField();
        sendButton = new Button("Send");

        //Handling user input

        sendButton.setOnMouseClicked((event) -> {
            handleUserInput();
        });
        userInput.setOnAction((event) -> {
            handleUserInput();
        });

        AnchorPane mainLayout = new AnchorPane();
        mainLayout.getChildren().addAll(scrollPane, userInput, sendButton);

        scene = new Scene(mainLayout);

        stage.setScene(scene);
        stage.show();

        //More code to be added here later
        //Formatting the window to look as expected

        stage.setTitle("HiChat");
        stage.setResizable(false);
        stage.setMinHeight(600.0);
        stage.setMinWidth(400.0);

        mainLayout.setPrefSize(400.0, 600.0);

        scrollPane.setPrefSize(385, 535);
        scrollPane.setHbarPolicy(ScrollPane.ScrollBarPolicy.NEVER);
        scrollPane.setVbarPolicy(ScrollPane.ScrollBarPolicy.ALWAYS);

        scrollPane.setVvalue(1.0);
        scrollPane.setFitToWidth(true);

        dialogContainer.setPrefHeight(Region.USE_COMPUTED_SIZE);

        userInput.setPrefWidth(325.0);

        sendButton.setPrefWidth(55.0);

        AnchorPane.setTopAnchor(scrollPane, 1.0);

        AnchorPane.setBottomAnchor(sendButton, 1.0);
        AnchorPane.setRightAnchor(sendButton, 1.0);
        AnchorPane.setLeftAnchor(userInput, 1.0);
        AnchorPane.setBottomAnchor(userInput, 1.0);

        //Scroll down to the end every time dialogContainer's height changes.
        dialogContainer.heightProperty().addListener((observable) -> scrollPane.setVvalue(1.0));

        dialogContainer.getChildren().addAll(
                DialogBox.getDukeDialog("Hello! I'm HiChat\nWhat can I do for you today?", dukeImage)
        );
    }

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

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

Example from src/main/java/hichat/Parser.java lines 15-18:

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

Example from src/main/java/hichat/Parser.java lines 23-26:

    /**
     * Check if the input string is "list"
     * @return true if the input string is "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

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.