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

[Tang Zhiying] iP #472

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

[Tang Zhiying] iP #472

wants to merge 44 commits into from

Conversation

zhing22
Copy link

@zhing22 zhing22 commented Aug 26, 2021

TiTi Catbot

"Meow Meow Meowww~ ฅ^•ﻌ•^ฅ" – TiTi

(and yes whatever TiTi says shall be correct)

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

  • text-based
  • SUPER easy to learn
  • SUPER SUPER fast to use (, and most importantly)
  • SUPER SUPER SUPER CUTE!

All you need to do is,

  1. download it from here.
  2. and start chatting with TiTi! 🐾

And it is FREE!!!


Features:

  • Managing tasks
  • Managing deadlines and events
  • Reminders (coming soon)
  • Cute pictures of TiTi (coming soon)

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

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

zhing22 added 20 commits August 18, 2021 21:00
* branch-Level-8:
  Add ability to understand dates and time
Extracted following classes:
Ui: deals with interactions with the user
Parser: deals with making sense of user command
TaskList: contains the task list
Added package duke.util and duke.task
* branch-A-CodingStandard:
  Slight tweak to align with coding standard

# Conflicts:
#	src/main/java/TiTi/util/Parser.java
#	src/main/java/TiTi/util/TaskList.java
#	src/main/java/TiTi/util/Ui.java
* branch-Level-9:
  Add function to search task using keyword

# Conflicts:
#	src/main/java/TiTi/util/Response.java
#	src/main/java/TiTi/util/TaskList.java
checkDate(by);
}

private void checkDate(String by) {
Copy link

Choose a reason for hiding this comment

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

Perhaps this function could be named with a verb, such as dateChecker.

Copy link
Author

Choose a reason for hiding this comment

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

thanks for the review! but dateChecker is actually a noun! And by convention class and variable should be names with nouns, and methods should be named with verb! Just saying in case you might have made the same assumptions for your own code ;-;

Comment on lines 42 to 44
if ((cue.equals("todo") || cue.equals("deadline")
|| cue.equals("event") || cue.equals("find"))
&& input.split(" ", 2).length == 1) {
Copy link

Choose a reason for hiding this comment

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

This expression seems a little bit complicated. Perhaps you could set aside a boolean isDescriptionMissing to handle this checking.

Comment on lines 10 to 14
private static final String STARTER_NORMAL = " (=^ ・ェ・^=) < ";
private static final String STARTER_BUFFER = " ";
private static final String STARTER_SLEEPY = " (=^ ‐ェ‐^=) < ";
private static final String STARTER_CONFUSED = " (=^ ・x・^=)? < ";
private static final String STARTER_HAPPY = " (=^ ・ω・^=)❀ < ";
Copy link

Choose a reason for hiding this comment

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

so cute!! 😸

public String toString() {
return "[T]" + super.toString();
}
}
Copy link

Choose a reason for hiding this comment

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

Do remember to add a new line here at the end of each file.

Comment on lines 64 to 70
if (taskNumber > numberOfTasks) {
return new Response(Response.Cue.TASKERROR);
} else {
Task task = taskList.get(taskNumber - 1);
task.complete();
return new Response(Response.Cue.DONE, task);
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
if (taskNumber > numberOfTasks) {
return new Response(Response.Cue.TASKERROR);
} else {
Task task = taskList.get(taskNumber - 1);
task.complete();
return new Response(Response.Cue.DONE, task);
}
if (taskNumber > numberOfTasks) {
return new Response(Response.Cue.TASKERROR);
}
Task task = taskList.get(taskNumber - 1);
task.complete();
return new Response(Response.Cue.DONE, task);

Since a new Response is returned in the if-block, the else-block is not required and can be removed so the main path is un-indented.
I noticed the same issue in several other places too.

@zhing22 zhing22 marked this pull request as ready for review August 31, 2021 10:49
Copy link

@xyliew25 xyliew25 left a comment

Choose a reason for hiding this comment

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

There are quite a few coding standards violations, but kudos to all the effort you put into your iP! It is indeed a CUTEE chatbot!! Also, do correct me if I misunderstood or commented something wrongly :)

@@ -0,0 +1,36 @@
import TiTi.util.SavedHistory;

Choose a reason for hiding this comment

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

According to the coding standard, every class should be part of some package. Perhaps package titi;

@@ -0,0 +1,41 @@
package TiTi.task;

Choose a reason for hiding this comment

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

According to the coding standard, names representing packages should be in all lower case. So in this case, perhaps package titi.task;? This applies to the other files as well.

Comment on lines 3 to 9
import TiTi.task.Task;
import TiTi.task.Event;
import TiTi.task.Deadline;
import TiTi.task.ToDo;

import java.util.ArrayList;
import java.util.Scanner;

Choose a reason for hiding this comment

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

Not sure if there is a fixed import order convention, but I think as long as it is consistent throughout the project, it is alright. According to this link, the above import is reversed.

* Contain the task to be printed (if needed).
*/
public class Response {
public enum Cue {EXIT, LIST, DONE, DELETE, TASKERROR, TODO, DEADLINE,

Choose a reason for hiding this comment

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

Not sure if enum names should have underscore to separate two words, as in like constants.

*
* @return size of list
*/
public int size() {

Choose a reason for hiding this comment

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

According to the coding standards, names representing methods must be verbs. So perhaps getSzie?

/**
* Prints the response for the user command onto the user interface.
*/
public void userCommand() {

Choose a reason for hiding this comment

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

Similarly, perhaps printUserCommand?

}

@Test
public void DateTest(){
Copy link

@xyliew25 xyliew25 Aug 31, 2021

Choose a reason for hiding this comment

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

According to coding standards, names representing methods must be verbs and written in camelCase. So perhaps testDate()?

Comment on lines 3 to 4
import TiTi.task.Deadline;
import org.junit.jupiter.api.Test;

Choose a reason for hiding this comment

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

Regardless of the import order, I think there should be a new line separate these two imports as they are from different sources.

Comment on lines 8 to 11
* Contain String description of the task.
* Contain String description of the date of task.
* Contain LocalDate object of the date of task (if applicable).
* Contain boolean value of whether the task has been completed.

Choose a reason for hiding this comment

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

Should be Contains instead of Contain. These are probably overlooked but I noticed the same issue in several other places too.

protected LocalDate date;

/**
* Constructor for Deadline class.
Copy link

@xyliew25 xyliew25 Aug 31, 2021

Choose a reason for hiding this comment

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

Not sure if nouns are allowed. If not, perhaps can put Constructs a Deadline instance. ?

zhing22 and others added 7 commits September 1, 2021 17:08
Package the app as an executable JAR file so that it can be distributed easily.
Input description from user when adding tasks might be empty. If so, response would be taken care in UI response. Hence tasks with empty description should not be created.

* Assertion test is used to stop program if attempt to initialise empty description tasks
zhing22 and others added 17 commits September 11, 2021 12:49
Examines the code and refactor to improve the code quality where necessary:
* break long method into smaller sub-methods
* improve variable naming
* improve documentation style
* other slight changes
* branch-A-CodeQuality:
  Improve code quality.
Provide a way to perform tasks on multiple items:
* Multiple tasks can be deleted at once
* Multiple tasks can be marked done at once
changes to:
*package naming
*fix header comments for TiTi.main()
Includes a quick description of TiTi Chatbot, and a list of all usages and commands
* Wrong picture namings
* Problem with running program
* Problems with delete and done functionalites
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.

3 participants