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

[seandias] iP #627

Open
wants to merge 59 commits into
base: master
Choose a base branch
from
Open

[seandias] iP #627

wants to merge 59 commits into from

Conversation

seandias
Copy link

@seandias seandias commented Sep 2, 2024

iP Project: Code Refactoring and Javadoc Update

This pull request includes significant improvements to the code structure and documentation. Below are the details of the changes made:

Key Changes Implemented

  • 📝 Added missing Javadoc comments across multiple classes.
  • 🔧 Resolved Checkstyle issues related to:
    • Import ordering
    • Indentation
    • Line length.
  • 🛠️ Improved the structure and readability of core classes:
    • Parser.java
    • ToStore.java
    • Task-related classes like Deadline, Task, and ToDo.

Detailed Steps Performed

  1. Refactored the Parser.java class to fix operator wrapping and indentation issues.
  2. Added Javadoc comments for all methods and classes, including CommandType, Event, Task, Deadline, and ToDo.
  3. Merged changes from the Git-Standard branch into master.
  4. Passed all unit tests after making the changes.
  5. Fixed all remaining Checkstyle violations.

Code Example: Parser.java

Here is an updated snippet from the Parser.java class, showing improved indentation and operator wrapping:

// Example of refactored code from Parser.java
public static Task parseAddCommand(String input, CommandType type) {
    switch (type) {
    case TODO:
        String description = input.substring(5).trim();
        if (description.isEmpty()) {
            throw new IllegalArgumentException("OOPS!!! The description of a todo cannot be empty.");
        }
        return new ToDo(description);
    case DEADLINE:
        String[] deadlineParts = input.substring(9).split(" /by ");
        if (deadlineParts.length < 2 or deadlineParts[0].trim().isEmpty() || deadlineParts[1].trim().isEmpty()) {
            throw new IllegalArgumentException("OOPS!!! The description or deadline cannot be empty.");
        }
        return new Deadline(deadlineParts[0].trim(), deadlineParts[1].trim());
    // ... remaining cases
    default:
        throw new IllegalArgumentException("OOPS!!! I'm sorry, but I don't know what that means :-(");
    }
}

Task List

  • Refactor Parser.java for better readability.
  • Add Javadoc comments to all methods.
  • Resolve all Checkstyle violations.
  • Merge the latest changes from the documentation branch.

"Code is like humor. When you have to explain it, it’s bad." — Cory House

For more information, check out the Java Coding Standards.

damithc and others added 27 commits July 11, 2024 16:52
In build.gradle, the dependencies on distZip and/or distTar causes
the shadowJar task to generate a second JAR file for which the
mainClass.set("seedu.duke.Duke") does not take effect.
Hence, this additional JAR file cannot be run.
For this product, there is no need to generate a second JAR file
to begin with.

Let's remove this dependency from the build.gradle to prevent the
shadowJar task from generating the extra JAR file.
# Conflicts:
#	src/main/java/skd/CommandType.java
#	src/main/java/skd/Parser.java
#	src/main/java/skd/SKD.java
#	src/main/java/skd/ToStore.java
#	src/main/java/skd/Ui.java
#	src/main/java/task/Deadline.java
#	src/main/java/task/Event.java
#	src/main/java/task/Task.java
#	src/main/java/task/TaskType.java
#	src/main/java/task/ToDo.java
# Conflicts:
#	src/main/java/skd/SKD.java
#	src/main/java/skd/command/CommandType.java
#	src/main/java/skd/command/Parser.java
#	src/main/java/task/Deadline.java
#	src/main/java/task/Event.java
#	src/main/java/task/Task.java
#	src/main/java/task/TaskType.java
#	src/main/java/task/ToDo.java
# Conflicts:
#	src/main/java/skd/SKD.java
#	src/main/java/skd/command/CommandType.java
#	src/main/java/skd/command/Parser.java
#	src/main/java/task/Deadline.java
#	src/main/java/task/Event.java
#	src/main/java/task/Task.java
#	src/main/java/task/TaskType.java
#	src/main/java/task/ToDo.java
Copy link

@L-rrrr L-rrrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is very good overall! There is no violation in naming convention!

import skd.command.Parser;
import skd.storage.ToStore;
import skd.ui.Ui;
import task.Task;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how you organise the imports by separating different kinds of them

public void run() {
ui.showWelcome();
Scanner scanner = new Scanner(System.in);
boolean isRunning = true;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how you name the boolean variable

/**
* Represents the different types of commands that the parser can interpret from user input.
*/
public enum CommandType {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how you used enum here

Comment on lines +71 to +72
|| eventParts[1].trim().isEmpty()
|| eventParts[2].trim().isEmpty()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how you make an extremely long line shorter by dividing them into 3 parts

*
* @param taskCount The current number of tasks in the list.
*/
public void printTaskRemovedMessage(int taskCount) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how you name the method to explain what it does

package skd.command;

import java.util.List;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The separation between different groups of import looks good to me.

*/
public class Deadline extends Task {
private static final DateTimeFormatter FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm");
private static final DateTimeFormatter OUTPUT_FORMATTER = DateTimeFormatter.ofPattern("MMM dd yyyy, HH:mm");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how those final fields are represented.

/**
* Represents a To-Do task, which is a basic task with only a description and no time constraints.
*/
public class ToDo extends Task {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your explanation about this class

* of the user's task list between sessions.
*/
public class SkD {
private ToStore storage;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how you name this variable

*/
public abstract class Task {
protected String description;
protected boolean isDone;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how you name the boolean variable

Copy link

@FYL2003 FYL2003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is no violation of coding standard

seandias and others added 30 commits September 11, 2024 13:21
This reverts commit 1329973.
-It needs to have a JavaDoc comment to pass.
-I added a JavaDoc comment for it.
-Now with the comment it passes checkstyle conditions.
-It needs to have a JavaDoc comment to pass.
-I added a JavaDoc comment for it.
-Now with the comment it passes checkstyle conditions.
This reverts commit 58ce543.
Allows for code to be tested and easier identification of error location.
Assertions added in to check storage.
Nne of the main checkpoints.
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

Successfully merging this pull request may close these issues.

5 participants