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

[Ryan Jee] iP #268

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

[Ryan Jee] iP #268

wants to merge 193 commits into from

Conversation

rjeez
Copy link

@rjeez rjeez commented Feb 6, 2021

No description provided.

Copy link

@danielonges danielonges left a comment

Choose a reason for hiding this comment

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

Generally code is quite well written! However, might want to be careful in leaving spaces where necessary in order not to violate coding standards. Other than that, LGTM! ☺️



public Duke(String filePath){
ui = new Ui();

Choose a reason for hiding this comment

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

Might want to consider removing extra space(s) between new Ui()

Command command = parser.parse(input);
String reply = command.execute(ui,taskList, storage);
storage.writeFile();
assert(reply!=null);

Choose a reason for hiding this comment

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

Good use of assertions!

DukeList dukeList = new DukeList();
StringBufferTasks = new StringBuffer();
try{
BufferedReader reader = new BufferedReader(new InputStreamReader(new DataInputStream(new FileInputStream(this.filename))));

Choose a reason for hiding this comment

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

Might have exceeded line limit

Path filePath = Path(current,filename);
boolean directoryExist = Files.exists(direcPath);
boolean fileExist = Files.exists(filePath);
try{

Choose a reason for hiding this comment

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

Might want to include spacing in between try {


public String showTaskAdded(String task, List<Task> taskList){
assert(task != null && taskList != null);
return String.format( "Added the following task : \n" + "%s\n" + "You now have %d tasks in your list.\n", task, taskList.size());

Choose a reason for hiding this comment

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

Might have exceeded line limit


public Storage (String filename){
this.filename = new File(filename);
accessTaskListInFileSystem(getCurrentDirectory());

Choose a reason for hiding this comment

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

Clear and concise naming of method

return " [" + type + "] " + " [" + getStatusIcon() + "] " + getname();
}
else{
return " [" + type + "] " + " [" + getStatusIcon() + "] " + getname() + " " + dateTime.format(DateTimeFormatter.ofPattern("yyyy-MM-dd kkmm"));

Choose a reason for hiding this comment

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

Might have exceeded line limit

Add CBye class for command bye
Add CDeadline class for command deadline
Add CDelete class for command delete
Add CDone class for command done
Add CEvent class for command event
Add CList class for command 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.

2 participants