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

[Arun Kumarr] Duke Increments #353

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

Conversation

ArunBeCoding
Copy link

No description provided.

@yaaanch
Copy link

yaaanch commented Sep 5, 2019

It's really good that you've written javadocs comments! But in the style guide, they suggest that the javadoc comments should be capitalised and start with a singular verb. Also, @param doesn't need the "is" word. So for example,

constructor for child class
defines the midcommand, and calculates the formattedDate
@param command is the user input string
@throws DukeException in case date is not entered in the correct format

could be something like

Constructs a Deadline with a description of the deadline and the date of the deadline.
@param command The user input string.
@throws DukeException If date is not entered in the correct format.

It's good that you've split up the tasks into more OOP, like with the parser, ui, and storage. I think another way of splitting it up would be to move the interpretation of the commands into the parser, and just have the ui for the printing.

It's also possible to go into even more OOP, for example having different commands. Then your parser could pass command objects back to Duke, then Duke executes the command with the Ui, Storage, and TasksList.

For error handling, some error names could be more descriptive, like DukeException for date formatting.

Good job overall!

@yyuanxin
Copy link

yyuanxin commented Sep 6, 2019

Hi Arun! I like that you have included Javadoc comments for most of your methods and have separated your test cases into different classes for jUnit testing which makes it more organised! (something I should consider doing as well).

Some suggestions:

  • introduce command classes to handle the user input - making it more OOP
  • organise your code with packages.

Overall, I think you have done a good job!

* to print for "list" command
* @return string in the format required
*/
public String printer(){

Choose a reason for hiding this comment

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

Need a space before the curly bracket.

try {
int val = Integer.parseInt(splitWords[1]);
ui.deleteMessage(val-1, tasks);
//System.out.println("Noted. I've removed this task:"+ "\n" + tasks.taskPrint(val-1) +
Copy link

Choose a reason for hiding this comment

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

Can delete the comments if it is not needed anymore :)

* to print for text file
* @return string in the format required
*/
public String printToOutput(){

Choose a reason for hiding this comment

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

add a space between () and {

* @throws DukeException in case segments[3] is not in the proper date format
*/
public static Task outputAsDeadline(String s) throws DukeException {
String[]segments = s.split("\\|");

Choose a reason for hiding this comment

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

add a space between [] and segments

* @throws Exception in case user inputs in an incorrect format
*/
public static void createDeadline(String command, TaskList tasks, Storage storage) throws DukeException {
String[]splitWords = command.trim().split("\\s",2);

Choose a reason for hiding this comment

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

add space between [] and splitWords


if (segments[1].equals(" 1 ")) {
newTask.taskDone();
} else {}
Copy link

Choose a reason for hiding this comment

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

Should not have an empty else block :)

* @return a Task (Deadline) object
* @throws DukeException in case segments[3] is not in the proper date format
*/
public static Task outputAsDeadline(String s) throws DukeException {

Choose a reason for hiding this comment

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

Maybe can use better argument name instead of s as it is not intuitive

/**
* marks when task is done
*/
public void taskDone(){
Copy link

@Yen-Peng Yen-Peng Sep 12, 2019

Choose a reason for hiding this comment

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

line 97, 104 & 112: space between () & {
line 97 to 107: You may consider either using try-catch block and throw your exception inside the try block, or just if-else and throw your exception in else block.

* exception for incorrect date format
* @param message is dummy parameter
*/
public DukeException(String message){

Choose a reason for hiding this comment

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

Space before curly bracket

String taskCommand = tasks.get(i).getCommand();
if (taskCommand.contains(wordToFind) ){
findResults.add(tasks.get(i));
}else{}

Choose a reason for hiding this comment

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

Consider removing empty else.

String[]splitUpDate = command.split("/",2);

try {
SimpleDateFormat ft = new SimpleDateFormat("dd/MM/yyyy HHmm");
Copy link

Choose a reason for hiding this comment

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

Could use more informative naming :) eg. dateformat

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

Successfully merging this pull request may close these issues.

8 participants