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

[Mengzhe] iP #466

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

Conversation

charliemoweng
Copy link

@charliemoweng charliemoweng commented Aug 26, 2021

DukeAzure

“Focus on being productive instead of busy.” - Tim Ferriss, American podcaster, author and entrepreneur

Here at DukeAzure, we help you be productive so you don't have to be busy taking down your tasks everywhere, ever.

Some selling points include:

  • easy to use
  • small-sized
  • fast VERY fast
  • Permanently FREE

Get yours from here today!

Some simple steps to follow:

  1. Follow the link above to the webpage.
  2. Download it!
  3. Run it in your Com 💻 !
  4. Start adding your tasks and manage them as you wish.

Some features (many more are on the way)

  • Add and store a task
  • Mark a task as done
  • Show all your tasks
  • Find a specific task
  • Send reminders before a task is due

For those avid coders out there

Here is a snippet of the code. Feel free to give suggestions!
The main method of Duke:

public static void main(String[] args) {
       new Duke(FILE_PATH).run();
} 

Copy link

@jackgugz jackgugz left a comment

Choose a reason for hiding this comment

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

Overall, I see consistent good coding standards being especially for the for loops and conditional statements! The spacing and indentation made it super easy to read. However, I do observe some common places for improvements:

  1. ordering of the imports
  2. comments indentation and lack of javadoc comments
  3. naming of variables.

Nonetheless still good job! Looking forward to the more updated version as I think it may be a few versions behind the schedule.

import java.time.LocalDateTime;
import java.time.format.DateTimeFormatter;
import java.time.temporal.ChronoUnit;

Choose a reason for hiding this comment

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

We need to add some JavaDoc comments for all public classes and methods. Noticed the lack of javadoc in some other places too.

Copy link
Author

Choose a reason for hiding this comment

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

Yes indeed. I will add that in soon. Thank you for pointing it out.

Comment on lines 1 to 11
import java.util.Locale;
import java.util.Scanner;
import java.util.ArrayList;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.io.FileNotFoundException;
import java.time.LocalDateTime;
import java.time.format.DateTimeFormatter;

Choose a reason for hiding this comment

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

Should an empty line be added between imports from different packages? Also shall we order imports from same package by their alphabetical order?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you are right. I will reorder them to make them look nicer.

public class Duke {

private static void appendToFile(String filePath, String textToAppend) throws IOException {
FileWriter fw = new FileWriter(filePath, true); // create a FileWriter in append mode

Choose a reason for hiding this comment

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

Should comments be indented relative to their position in the code?

Suggested change
FileWriter fw = new FileWriter(filePath, true); // create a FileWriter in append mode
// create a FileWriter in append mode
FileWriter fw = new FileWriter(filePath, true);

Copy link
Author

Choose a reason for hiding this comment

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

you are right.

Comment on lines 27 to 34
// private static void printFileContents(File file) throws FileNotFoundException {
// //File f = new File(filePath); // create a File for the given file path
// Scanner s = new Scanner(file); // create a Scanner using the File as the source
// while (s.hasNext()) {
// System.out.println(s.nextLine());
// }
// }

Choose a reason for hiding this comment

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

Shall we avoid such commented out codes?

Comment on lines 63 to 65
} else {
// System.out.println("Data folder is already present.");
}

Choose a reason for hiding this comment

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

Is it possible that we take out the else clause then?

Copy link
Author

Choose a reason for hiding this comment

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

yes thank you for pointing it out. i have removed it since.

// mark a task as Done
Pattern pattern = Pattern.compile("done\\s\\d+");
Matcher matcher = pattern.matcher(input);
boolean matchFound = matcher.find();

Choose a reason for hiding this comment

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

Suggested change
boolean matchFound = matcher.find();
boolean ifMatchFound = matcher.find();

so that it sounds more like a boolean

// delete a task
Pattern patternDelete = Pattern.compile("delete\\s\\d+");
Matcher matcherDelete = patternDelete.matcher(input);
boolean matchFoundDelete = matcherDelete.find();

Choose a reason for hiding this comment

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

Same to the boolean name here as well.

// Case Event
if (group[0].equals(taskEve) && input.contains(" /at ")) {
String[] eveGroup = input.split(" /at ");
String eveToAdd = eveGroup[0].substring(6); // name of the task is after "event" and space

Choose a reason for hiding this comment

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

Suggested change
String eveToAdd = eveGroup[0].substring(6); // name of the task is after "event" and space
// name of the task is after "event" and space
String eveToAdd = eveGroup[0].substring(6);

Comment on lines 1 to 4
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.format.DateTimeFormatter;
import java.time.temporal.ChronoUnit;

Choose a reason for hiding this comment

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

The ordering of the imports can be better!

Copy link
Author

Choose a reason for hiding this comment

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

yes i will improve upon it.

}

public String getStatusIcon() {
return (isDone ? "X" : " "); // mark done task with X

Choose a reason for hiding this comment

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

Suggested change
return (isDone ? "X" : " "); // mark done task with X
// mark done task with X
return (isDone ? "X" : " ");

Copy link

@1waykiat 1waykiat left a comment

Choose a reason for hiding this comment

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

Remember to complete the tasks from last week!
Mostly just need to extract away the Storage and Parser class so that the parsing and loading and storing of data can be abstracted away.

Comment on lines 72 to 85
// if data.txt does not exist, create file
if (!dataFile.exists()) {
try {
File dataDirectory = new File("./data/data.txt");
if (dataDirectory.createNewFile()) {
System.out.println("Data file has been created.");
} else {
// System.out.println("Data folder is already present.");
}
} catch (IOException e) {
System.out.println("An error occurred.");
e.printStackTrace();
}
}
Copy link

Choose a reason for hiding this comment

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

Could be extracted out to the Storage class to handle instead

Pattern pattern = Pattern.compile("done\\s\\d+");
Matcher matcher = pattern.matcher(input);
boolean matchFound = matcher.find();
if (matchFound) {
Copy link

Choose a reason for hiding this comment

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

Perhaps let Parser class handle the match finding instead, and extract the code there.

charliemoweng and others added 25 commits September 17, 2021 23:52
Assertions are a crucial part of Java error handling since.
It helps detects errors at runtime and terminates the application.

Assertions are missing in this project.

Add assert features in TaskList.java because it has many if blocks
that are suitable for using assert.
Good code quality denotes that methods are not too long, not beyond
one screen.

Some methods in Storage.java and TaskList.java are too long.

It is hard to read and locate specific functionalities.

Extract the logic and create helper methods within its class.
We assume that users would not want to input the same task twice.
This prevents possible confusion.

TaskList allows the creation of duplicates.

Add in methods to check for duplicating Todo, Deadline, and Event.
Only add one if it is not a duplicate.

This enhances user experience.
Find code to make Label of DialogBox have infinite height to
accommodate longer responses on GitHub and CS2103 Forum.

Acknowledge it.
It shows warning of incompatible version numbers when the application
is executed.

Remove the version number from fxml files.
Data file under the default path of ./data/data.txt
may or may not exist.
There is no need to create it if it already exists.

The Storage class always creates the data file.
If the file already exists, it will enter the catch block and
print out error message when there is no error at all.

Add in a check before creating the file. Only create it when
it is not yet existent.
Should be the finalised version of README
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