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

[Lim Yu Long] ip #634

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

Conversation

yulonglim
Copy link

No description provided.

damithc and others added 30 commits July 29, 2021 20:30
User may not remember the letter casing of the task description

Made find not case sensitive

To improve user experience using the application
Current find is case sensitive
Had many essential methods not have javadocs

Placed taskparse function in Parser.java and add javadocs for some methods

Improved code quality
User may have made small errors when adding task and needs to delete and add

This is not user friendly and waste a lot of time

Hence new gui allows for updating of task easily
yulonglim and others added 20 commits September 7, 2021 22:44
User may have made small errors when adding task and needs to delete and add

This is not user friendly and waste a lot of time

Hence new gui allows for updating of task easily
Screenshot for user guide was not present

Added Ui.png

Let users see a preview of the ui before they use it
Users previously may input wrong items for add task

Now gui will inform the specific problem

Duke.txt moved to resource folder
Previous code was done at java 15

Updated structure and built new jar at java 11
This is inconvenient for the user

Changed code to initialize with the list loaded
… functions to stop working

Changed code and tested
… not noted

Shifted Duketest file along with updating gradle file for test to work
Add javadoc to all the public methods
Abstract part of load() method to parser class and cleaned up code
Copy link
Contributor

@damithc damithc left a comment

Choose a reason for hiding this comment

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

Had a quick look (not a full review) and added some comments. Optional to fix. FYI only.

try {
storage.save(tasks);
} catch (IOException e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like a good way to handle this exception. 🤔

} catch (NumberFormatException e) {
updateError.setText("Please enter integer");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These three methods seem to be very similar. See if you can reduce duplication.

at.split("/")[0]));
} else {
date = LocalDate.parse(at);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some similar code elsewhere. Minimize code duplication.

*/
public static ArrayList<Task> storageParse(String storageData) {
ArrayList<Task> tasks = new ArrayList<>();
if (!storageData.equals("")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use a guard clause to reduce nesting. e.g.,

if (storageData.equals("")) {
    return tasks;
}
...

ArrayList<Task> tasks = new ArrayList<>();
if (!storageData.equals("")) {
for (String i :
storageData.split("\n")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line break needed?

public String toString() {
if (this.list.isEmpty()) {
return "There are no tasks in your list!\n";
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to put this inside else as line 116 is a return statement.

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.

2 participants