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

[jellywaiyan] iP #538

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

Conversation

jellywaiyan
Copy link

@jellywaiyan jellywaiyan commented Sep 3, 2023

Jelly

Hello :o

Jelly frees your mind of having to remember things you need to do. It's,

  1. Text-based
  2. Easy to learn
  3. FAST MAYBE FAST to use :o

All you need to do is,

  • Download it from here.
  • Double-click it.
  • Add your tasks.
  • Let it manage your tasks for you 😮

And it is FREE!

Features:

  • Managing tasks
  • Managing deadlines
  • Reminders

If you are a java programmer, you can use it to practice Java too. Here's the main method:

public class Main {
    public static void main(String[] args) {
        Application.launch(MainApp.class, args);
    }
}

# Conflicts:
#	src/main/java/Jelly/main/Storage.java
#	src/main/java/Jelly/main/TaskList.java
#	src/main/java/Jelly/main/Ui.java
#	src/main/java/Jelly/task/Deadline.java
#	src/main/java/Jelly/task/Event.java
#	src/main/java/Jelly/task/Todo.java
# Conflicts:
#	src/main/java/Jelly/main/Jelly.java
#	src/main/java/Jelly/main/Ui.java
Copy link

@yiwen101 yiwen101 left a comment

Choose a reason for hiding this comment

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

Looks good
Quite comprehensive documentation
Well designed

@@ -54,6 +58,7 @@ public String toString() {
}
return "[E]" + super.toString() + "(from: " + fromWhen + " to: " + toWhen + ")";
}

@Override
public String writeToFile() {
Copy link

Choose a reason for hiding this comment

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

Could be better named.
I do not expect"writeToFile" to be a method that returns the String to be written

/**
* The main initialiser for the Jelly Chat bot.
*
* @param args
Copy link

Choose a reason for hiding this comment

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

I do not think "@param args" help much
And I believe that you do not need to document every method as well.

public Ui() {
sc = new Scanner(System.in);
}

/**
* Prints out a welcome message when the Chat Bot is booted up.
*/
public void startUpMessage() {
Copy link

Choose a reason for hiding this comment

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

sounds like a noun

* @param addedTask The task that was added.
* @param noOfTasks The total number of tasks in the list after adding.
*/
public void addedTaskMessage(Task addedTask, int noOfTasks) {
Copy link

Choose a reason for hiding this comment

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

noun


/**
* Displays a final message to the user before the Chat Bot shuts down.
*/
public void byeMessage() {
Copy link

Choose a reason for hiding this comment

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

noun

/**
* Should not happen, as tasks should be either one of these: todo, deadline or event.
* @return Error message.
*/
public String writeToFile() {
Copy link

Choose a reason for hiding this comment

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

if this should not happen, why not throw exception when called?

Copy link

@ElginTZM ElginTZM left a comment

Choose a reason for hiding this comment

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

Looks great! No major coding standard issues, just some minor issues with how you name some variables like the other reviewer have mentioned

Comment on lines 11 to 15
private String description;

private String fromWhen;

private String toWhen;
Copy link

@ElginTZM ElginTZM Sep 8, 2023

Choose a reason for hiding this comment

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

Perhaps can be grouped together

Comment on lines 4 to 9
import Jelly.task.Task;
import Jelly.task.Todo;
import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertFalse;
Copy link

Choose a reason for hiding this comment

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

Perhaps you can separate out the imports?


public class StorageTest {
@Test
public void startUpTest() throws IOException, JellyException {
Copy link

Choose a reason for hiding this comment

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

Test methods name can utilize another convention for naming them: featureUnderTest_testScenario_expectedBehavior()

jellywaiyan and others added 18 commits September 10, 2023 01:03
Delete command: Save to file after deleting
Mark command: Save to file after marking a task as done
Unmark command: Save to file after marking a task as not done

Previously, these commands did not save to file, which caused the file to be outdated.

Let's,
*add a saveToFile method after completing the command
Improve code quality by making methods more efficient and readable.
Assert feature allows the documentation of important assumptions that should hold at various points in the code.

Let's,
*add assertions in Task, Parser,DeleteCommand, MarkCommand, UnmarkCommand.
Current implementation does not use assertions.
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.

4 participants