Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sharing iP code quality feedback [for @techjay-c] #3

Open
soc-se-script opened this issue Sep 16, 2023 · 0 comments
Open

Sharing iP code quality feedback [for @techjay-c] #3

soc-se-script opened this issue Sep 16, 2023 · 0 comments

Comments

@soc-se-script
Copy link

@techjay-c 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/duke/Parser.java lines 25-74:

    public static Command parseCommand(String userInput) throws DukeException {
        String[] split = userInput.split(" ");
        String command = split[0];
        String arguments = userInput.replaceFirst(command, "").trim();

        switch (command) {
        case "bye":
            return new ByeCommand();
        case "list":
            return new ListCommand();
        case "search":
            if (arguments.isEmpty()) {
                throw new DukeException.SearchException();
            }
            return new SearchCommand(arguments);
        case "delete":
            int indexToDelete = Integer.parseInt(arguments) - 1;
            return new DeleteCommand(indexToDelete);
        case "todo":
            if (arguments.isEmpty()) {
                throw new DukeException.ToDoException();
            }
            return new AddCommand(AddCommand.TaskType.TODO, arguments);
        case "event":
            if (arguments.isEmpty() || !arguments.contains("/from")) {
                throw new DukeException.EventException();
            }
            return EventParser.parseEventCommand(arguments);
        case "deadline":
            // Solution below adapted and inspired by https://chat.openai.com/share/b706b4df-ab30-4d0f-93eb-b85617616319
            if (arguments.isEmpty()) {
                throw new DukeException.DeadlineException();
            }
            return DeadlineParser.parseDeadlineCommand(arguments);
        case "unmark":
            if (arguments.isEmpty()) {
                throw new DukeException.UnmarkException();
            }
            int indexToUnmark = Integer.parseInt(arguments) - 1;
            return new UnmarkCommand(indexToUnmark);
        case "mark":
            if (arguments.isEmpty()) {
                throw new DukeException.MarkException();
            }
            int indexToMark = Integer.parseInt(arguments) - 1;
            return new MarkCommand(indexToMark);
        default:
            throw new DukeException.NoSuchItemException();
        }
    }

Example from src/main/java/duke/Storage.java lines 42-80:

    public static void saveTasks(ArrayList<Task> tasks) {

        try (FileWriter writer = new FileWriter(filePath)) {

            for (Task task : tasks) {
                String taskType = "";
                if (task instanceof Event) {
                    taskType = EVENT_TASK;
                } else if (task instanceof Todo) {
                    taskType = TODO_TASK;
                } else if (task instanceof Deadline) {
                    taskType = DEADLINE_TASK;
                }

                String statusIcon = (task.getStatusIcon().equals("X")) ? "1" : "0";

                if (taskType.equals("Event")) {
                    Event event = (Event) task;
                    LocalDateTime fromDateTime = event.fromDateAndTime();
                    LocalDateTime toDateTime = event.toDateAndTime();
                    String formattedFromDateTime = fromDateTime.format(DateTimeFormatter.ofPattern("MMMM d yyyy ha"));
                    String formattedToDateTime = toDateTime.format(DateTimeFormatter.ofPattern("MMMM d yyyy ha"));
                    writer.write("E " + "| " + statusIcon + " | " + event.description
                        + " | " + formattedFromDateTime + " - " + formattedToDateTime + "\n");
                } else if (taskType.equals("Deadline")) {
                    Deadline deadline = (Deadline) task;
                    LocalDateTime dateTime = deadline.getDateAndTime();
                    String formattedDateTime = dateTime.format(DateTimeFormatter.ofPattern("dd MMM yyyy h:mma"));
                    writer.write("D " + "| " + statusIcon + " | " + deadline.description
                        + " | " + formattedDateTime + "\n");
                } else if (taskType.equals("Todo")) {
                    Todo todo = (Todo) task;
                    writer.write("T " + "| " + statusIcon + " | " + todo.description + "\n");
                }
            }
        } catch (IOException e) {
            System.out.println("Error writing to: " + filePath);
        }
    }

Example from src/main/java/duke/Storage.java lines 93-167:

    static ArrayList<Task> loadTasks() throws IOException {
        ArrayList<Task> tasks = new ArrayList<>();

        try {
            File file = new File(filePath);
            assert file.exists() : "File does not exist: " + filePath;
            if (!file.exists()) {
                // Handling data file to be in a specific folder
                if (file.getParentFile() != null) {
                    // Solution below adapted from
                    // https://www.oreilly.com/library/view/java-cookbook/0596001703/ch10s10.html#:~:
                    // text=Of%20the%20two%20methods%20used,%2Fian%2Fbin%22).
                    file.getParentFile().mkdirs();
                }
                file.createNewFile();
            } else {
                BufferedReader reader = new BufferedReader(new FileReader(filePath));
                String dataToWrite;
                while ((dataToWrite = reader.readLine()) != null) {
                    System.out.println(dataToWrite); // Debug line


                    String[] sections = dataToWrite.split(" \\| ");
                    if (sections.length >= 3) {
                        String taskType = sections[0];
                        String taskStatus = sections[1];
                        String taskDescription = sections[2];
                        String taskOtherInfo = sections.length > 3 ? sections[3] : "";

                        // declares variable of type Task and initialize with null value
                        Task task = null;
                        boolean isDone = taskStatus.equals("1");

                        switch (taskType) {

                        case "E":
                            String[] eventParts = taskOtherInfo.trim().split("-");
                            DateTimeFormatter formatter = DateTimeFormatter.ofPattern("MMMM d yyyy ha");
                            LocalDateTime parsedFromStartDateTime = LocalDateTime.parse(eventParts[0].trim(),
                                formatter);
                            LocalDateTime parsedFromEndDateTime = LocalDateTime.parse(eventParts[1].trim(),
                                formatter);
                            if (eventParts.length == 2) {
                                task = new Event(taskDescription, parsedFromStartDateTime, parsedFromEndDateTime);
                            }
                            break;
                        case "D":
                            DateTimeFormatter formatterD = DateTimeFormatter.ofPattern("dd MMM yyyy h:mma");
                            LocalDateTime dateTimeD = LocalDateTime.parse(taskOtherInfo, formatterD);
                            task = new Deadline(taskDescription, dateTimeD);
                            break;
                        case "T":
                            task = new Todo(taskDescription);
                            break;
                        default:
                            task = new Task(taskDescription, Task.Type.OTHERS);
                        }

                        if (task != null) {
                            if (isDone) {
                                task.markAsDone();
                            } else {
                                task.markAsNotDone();
                            }
                            tasks.add(task);
                        }
                    }
                }
                reader.close();
            }
        } catch (IOException e) {
            System.out.println("Error reading from: " + filePath);
        }
        return tasks;
    }

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


Improve Code Quality

There are several sections in the project that can be improved further to achieve better code quality and readability.

Making these changes improves the code quality.

Changed the naming of certain static variables and added separate Parser classes for Event and Deadline.


  • body not wrapped at 72 characters: e.g., There are several sections in the project that can be improved further to achieve better code quality and readability.

possible problems in commit c284afa:


Implemented JavaFX as a separate project


  • 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 [email protected] if you want to follow up on this post.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant