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

[soc-se-script]

@avinazz3 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/babe/DialogBox.java lines 23-69:

    private DialogBox(String text, Image img, boolean isUser) {
        try {
            FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("/view/DialogBox.fxml"));
            fxmlLoader.setController(this);
            fxmlLoader.setRoot(this);
            fxmlLoader.load();

            // Configure text
            dialog.setText(text);
            dialog.setWrapText(true);
            dialog.setMinWidth(100);
            dialog.setMaxWidth(400);

            // Configure image
            displayPicture.setImage(img);
            displayPicture.setFitHeight(AVATAR_SIZE);
            displayPicture.setFitWidth(AVATAR_SIZE);

            // Add spacing and padding
            setSpacing(8);
            setPadding(new Insets(8));

            // Create flexible spacer
            Region spacer = new Region();
            HBox.setHgrow(spacer, Priority.ALWAYS);

            // Clear existing children (to avoid duplicates)
            getChildren().clear();

            // Style based on message type
            if (isUser) {
                // User message styling
                setAlignment(Pos.CENTER_RIGHT);
                dialog.setStyle("-fx-background-color: #0084ff; -fx-text-fill: white; " +
                        "-fx-padding: 8 12; -fx-background-radius: 15;");
                getChildren().addAll(spacer, dialog, displayPicture);
            } else {
                // Bot message styling
                setAlignment(Pos.CENTER_LEFT);
                dialog.setStyle("-fx-background-color: white; -fx-text-fill: black; " +
                        "-fx-padding: 8 12; -fx-background-radius: 15;");
                getChildren().addAll(displayPicture, dialog, spacer);
            }
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

Example from src/main/java/babe/parser/Parser.java lines 118-163:

    static Task createDeadline(String input) throws BabeException {
        assert input != null : "Input string cannot be null"; // Ensure input is not null
        if (input.equals("deadline")) {
            throw new BabeException("The description of a deadline cannot be empty!");
        }
        if (!input.contains("/by")) {
            throw new BabeException("Please provide a deadline using /by!");
        }
        String[] mainParts = input.split(" /by ");
        if (mainParts.length != 2) {
            throw new BabeException("Please provide both a description and deadline!");
        }

        // Extract description and priority
        String descriptionPart = mainParts[0].substring(9).trim();
        String[] descAndPriority = descriptionPart.split(" /p ");
        String description = descAndPriority[0].trim();

        // Handle priority
        Task.Priority priority;
        try {
            if (descAndPriority.length > 1) {
                int priorityLevel = Integer.parseInt(descAndPriority[1].trim());
                validatePriorityLevel(priorityLevel);
                priority = Task.Priority.fromLevel(priorityLevel);
            } else {
                priority = Task.Priority.MEDIUM;
            }
        } catch (NumberFormatException e) {
            throw new BabeException("Please provide a valid priority level (1-3)!");
        }

        String byStr = mainParts[1].trim();
        if (description.isEmpty() || byStr.isEmpty()) {
            throw new BabeException("The description and deadline cannot be empty!");
        }
        assert !description.isEmpty() && !byStr.isEmpty() : "Description and deadline should not be empty"; // Ensure both are not empty

        try {
            LocalDateTime by = LocalDateTime.parse(byStr, INPUT_FORMATTER);
            return new Deadline(description, by, priority);
        } catch (DateTimeParseException e) {
            throw new BabeException("Please enter date and time in the format: yyyy-MM-dd HHmm\n" +
                    "For example: 2024-01-30 1430 for January 30, 2024, 2:30 PM");
        }
    }

Example from src/main/java/babe/parser/Parser.java lines 172-226:

    private static Task createEvent(String input) throws BabeException {
        assert input != null : "Input string cannot be null"; // Ensure input is not null
        if (input.equals("event")) {
            throw new BabeException("The description of an event cannot be empty!");
        }
        if (!input.contains("/from") || !input.contains("/to")) {
            throw new BabeException("Please provide event time using /from and /to!");
        }
        String[] timeParts = input.split(" /from | /to ");
        if (timeParts.length != 3) {
            throw new BabeException("Please provide a description, start time, and end time!");
        }

        // Extract description and priority
        String descriptionPart = timeParts[0].substring(6).trim();
        String[] descAndPriority = descriptionPart.split(" /p ");
        String description = descAndPriority[0].trim();

        // Handle priority
        Task.Priority priority;
        try {
            if (descAndPriority.length > 1) {
                int priorityLevel = Integer.parseInt(descAndPriority[1].trim());
                validatePriorityLevel(priorityLevel);
                priority = Task.Priority.fromLevel(priorityLevel);
            } else {
                priority = Task.Priority.MEDIUM;
            }
        } catch (NumberFormatException e) {
            throw new BabeException("Please provide a valid priority level (1-3)!");
        }

        String startStr = timeParts[1].trim();
        String endStr = timeParts[2].trim();

        if (description.isEmpty() || startStr.isEmpty() || endStr.isEmpty()) {
            throw new BabeException("The description, start time, and end time cannot be empty!");
        }
        assert !description.isEmpty() && !startStr.isEmpty() && !endStr.isEmpty() :
                "Description, start time, and end time should not be empty"; // Ensure all are not empty

        try {
            LocalDateTime start = LocalDateTime.parse(startStr, INPUT_FORMATTER);
            LocalDateTime end = LocalDateTime.parse(endStr, INPUT_FORMATTER);

            if (end.isBefore(start)) {
                throw new BabeException("Event end time cannot be before start time!");
            }

            return new Event(description, start, end, priority);
        } catch (DateTimeParseException e) {
            throw new BabeException("Please enter date and time in the format: yyyy-MM-dd HHmm\n" +
                    "For example: 2024-01-30 1430 for January 30, 2024, 2:30 PM");
        }
    }

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 Messages

possible problems in commit bec9747:

fix: Remove redundant exception handling in priority parser

NumberFormatException is a subclass of IllegalArgumentException, so catching both was redundant. Now only catching the parent exception to handle both cases.

• body not wrapped at 72 characters: e.g., NumberFormatException is a subclass of IllegalArgumentException, so catching both was redundant. Now only catching the parent exception to handle both cases.

possible problems in commit db946d4:

Improve GUI appearance and fix JavaFX loading issues

- Fix architecture compatibility issues by updating JavaFX dependencies for Apple Silicon (arm64)
- Resolve FXML loading errors in MainWindow and DialogBox
- Fix duplicate children issue in DialogBox component
- Improve message styling with proper alignment and colors
- Reduce avatar size to optimize space usage
- Add proper padding and spacing for better visual layout
- Update build configuration for Apple Silicon support

Technical improvements:
- Remove fx:root from MainWindow.fxml and use proper controller pattern
- Clear children before adding new ones in DialogBox to prevent duplicates
- Add proper anchoring of components in MainWindow
- Update JavaFX version to match JDK version

• body not wrapped at 72 characters: e.g., - Fix architecture compatibility issues by updating JavaFX dependencies for Apple Silicon (arm64)

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 👍

---

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