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

[Sun Yu Ting] iP #22

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

Conversation

Effixion
Copy link

@Effixion Effixion commented Aug 19, 2022

No description provided.

damithc and others added 30 commits July 31, 2022 17:20
# Conflicts:
#	src/main/java/Deadline.java
#	src/main/java/Event.java
#	src/main/java/TaskList.java
Merge Level-8 branch with main branch
# Conflicts:
#	src/main/java/duke/parser/Parser.java
Copy link

@lilythchu lilythchu left a comment

Choose a reason for hiding this comment

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

Your code looks good! I feel that your code was easy to read as it was very organized.

public class Storage {

private static String filePath;
private File f;

Choose a reason for hiding this comment

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

Maybe you could name this variable more specifically. As said in the textbook, a name is not just for differentiation; it should explain the named entity to the reader accurately and at a sufficient level of detail.

Copy link
Author

Choose a reason for hiding this comment

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

Okay thanks for your recommendation!

String[] splitCommand = s.split(" ", 2);
String command = splitCommand[0];
switch (command) {
case "bye":

Choose a reason for hiding this comment

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

This method is quite long. Maybe you could split it into some small methods for each case.

Copy link
Author

Choose a reason for hiding this comment

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

I agree with your criticism here. I will try to reduce the method length when I do have the time to look at this again.

Copy link

@yeoyujie yeoyujie left a comment

Choose a reason for hiding this comment

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

LGTM, overall this seem v solid.

*/
public class Parser {

private static TaskList taskList;
Copy link

Choose a reason for hiding this comment

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

Should we extract out tasklist storage and UI? Could consider using Duke.java to control the logic flow instead of exposing storage, UI and tasklist all to parser?

Copy link
Author

Choose a reason for hiding this comment

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

This is true since I am indeed exposing these classes to parser. I will try to address this issue when I have the time.

* Reads the tasks from the file and loads them into an <code>ArrayList</code>
* @return An <code>ArrayList</code> containing the tasks from the file.
*/
public ArrayList<String> load() {
Copy link

Choose a reason for hiding this comment

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

Could consider renaming this loadFromFile to standardise with appendToFile

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. I will change it soon.

Effixion and others added 23 commits September 8, 2022 14:27
The "bye" case in readInputString method was not adhering to
SLAP.

Thus, I added in two methods into taskList so that I could abstract
out what I was doing before so as to SLAP hard. This would make
it easier to understand the code.

Also, since I had not fully implemented changing of the date format,
I practiced KISSing. I made the code simple to change the date
format. I also added in new methods in the UI to show related
messages when the user want to change the date format.
to fit naming conventions.

Assertions were placed in the readInputString method in Parser
so that the first element in splitCommand will never be null. This
is important to check since the rest of the code depends on
what is contained in the first element of splitCommand.

Also, comments made by my peers about the naming of certain
variables and methods were heeded. I changed the name of load()
to loadFromFile() and I changed the name of File f to File taskFile.
Users now have the ability to tag the tasks that they add.
They can also choose not to tag the task as well.
All tags will be prefixed by a hash symbol, i.e. #.
This will allow users to customize their tasks to a greater
extent.
reading and writing to the external file. Add a findtag command for
the user to find a tag in their task list.
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