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

[Jarrett0203] iP #10

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

Conversation

Jarrett0203
Copy link

@Jarrett0203 Jarrett0203 commented Jan 17, 2023

Fate Eggplant Assistant (FEA)

"The Archer class really is made up of archers!" -- Rin Tohsaka (source)

FEA lets you focus on defeating lostbelts by having Mash remember things you need to do. It's,

  • text-based
  • easy to learn
  • FAST SUPER FAST to use

All you need to do is,

  1. Download it from here.
  2. Find the latest version and install fea.jar.
  3. Add your tasks.
  4. Let Mash manage your tasks for you 😉

And it is FREE!

Features:

  • Managing tasks
  • Managing deadlines (coming soon)
  • Reminders (coming soon)

If you are a Java programmer, you can use it to practice Java too. Here's the main method:

public class Duke {
   public static void main(String[] args) { 
      Application.launch(Fea.class, args);
   }
}

Copy link

@pangrwa pangrwa 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 like the structure of your code and is very readable. However, I think you can improve on certain aspects of code quality. For an example, there are a couple of methods that have quite deep nesting. Keep it up!

}
default:
throw new InvalidInputException(INVALID_UNKNOWN);
}
Copy link

Choose a reason for hiding this comment

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

Perhaps for this method you might want to consider separating parts of the logic into another method to avoid this rather long method, so that it's easier to debug.

Comment on lines +22 to +30
private static final String INVALID_DEADLINE_FORMAT = "The deadline must be in the format:"
+ " deadline <task> /by <date>, <task> and <date> cannot be empty.";
private static final String INVALID_EVENT_FORMAT = "The event must be in the format:"
+ " event <task> /from <date> /to <date>, <task> and <date> cannot be empty.";
private static final String INVALID_DATE_FORMAT =
"Dates must be in the format: dd/mm/yyyy HHmm (e.g. 12/12/2023 1800)";
private static final String INVALID_UNKNOWN = "I'm sorry, but I don't know what that means.";
private static final String END_BEFORE_START = "The start date must be before the end date.";
private static final String FULL_LIST = "List is full!";
Copy link

Choose a reason for hiding this comment

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

The use of constant string lines is great and really thoughtful! It does help to abstract your code.

public String execute(TaskList tasks, Ui ui, Storage storage) throws FeaException {
try {
int taskNum = Integer.parseInt(fullCommand.substring(7));
if (taskNum > tasks.size() || taskNum < 1) {
Copy link

Choose a reason for hiding this comment

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

Maybe you could consider abstracting the boolean expression to avoid complicated expressions.

Comment on lines +24 to +41
String lowercaseCommand = fullCommand.toLowerCase();
Command command;
if (lowercaseCommand.equals("bye")) {
command = new ByeCommand();
} else if (lowercaseCommand.equals("list")) {
command = new ListCommand();
} else if (lowercaseCommand.startsWith("todo")) {
command = new AddCommand('T', fullCommand);
} else if (lowercaseCommand.startsWith("deadline")) {
command = new AddCommand('D', fullCommand);
} else if (lowercaseCommand.startsWith("event")) {
command = new AddCommand('E', fullCommand);
} else if (lowercaseCommand.startsWith("delete")) {
command = new DeleteCommand(fullCommand);
} else if (lowercaseCommand.contains("mark")) {
command = new MarkCommand(!lowercaseCommand.startsWith("unmark"), fullCommand);
} else if (lowercaseCommand.startsWith("find")) {
command = new FindCommand(fullCommand);
Copy link

Choose a reason for hiding this comment

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

Very neat and readable!

Comment on lines 48 to 75
checkTaskFormat(args[0], args[1]);
switch (args[0]) {
case "T":
Todo newTodo = new Todo(args[2]);
if (args[1].equals("1")) {
newTodo.toggleMark();
}
loadedList.add(newTodo);
break;
case "D":
Deadline newDeadline = new Deadline(args[2], LocalDateTime.parse(args[3], formatter));
if (args[1].equals("1")) {
newDeadline.toggleMark();
}
loadedList.add(newDeadline);
break;
case "E":
Event newEvent = new Event(args[2], LocalDateTime.parse(args[3], formatter),
LocalDateTime.parse(args[4], formatter));
if (args[1].equals("1")) {
newEvent.toggleMark();
}
loadedList.add(newEvent);
break;
default:
break;
}
}
Copy link

Choose a reason for hiding this comment

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

Seems like there is quite a deep nesting here. Might want to consider abstracting some parts out.

Copy link

@Douglch Douglch left a comment

Choose a reason for hiding this comment

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

The code overall is really neat, liked the use of constant naming and how you separated the command classes. Good job! 👍

* InvalidCommand class that implements the Command interface.
*/

public class InvalidCommand implements Command {
Copy link

Choose a reason for hiding this comment

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

Perhaps this could be refactored to the exception package instead?


try (FileWriter fileWriter = new FileWriter(file)) {
for (Task task : tasks) {
int mark = task.getMark().equals('X') ? 1 : 0;
Copy link

Choose a reason for hiding this comment

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

If I'm not wrong, using instanceof is usually a bad practice as it misses out on OOP design, maybe you could try creating/using the respective task class's functions to do the checks instead?

Copy link

Choose a reason for hiding this comment

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

Other that that, very clean way of saving tasks

/**
* Prints task when it is added or removed.
*
* @param task The task to be printed.
Copy link

Choose a reason for hiding this comment

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

Think there's unnecessary white space between task and The

* @return String The string of the task to be marked.
*/
public String printMarkTask(Task task, boolean toMark) {
StringBuilder message = new StringBuilder();
Copy link

Choose a reason for hiding this comment

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

Perhaps you would want to create one instance of StringBuilder for the entire class, then calling sb.setLength(0) at the start to clear the string builder, instead of creating multiple instances?

@francisyzy francisyzy mentioned this pull request Feb 4, 2023
Copy link

@glyfy glyfy left a comment

Choose a reason for hiding this comment

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

Good job overall

if (tasks.size() >= 100) {
throw new ListException(FULL_LIST);
}
switch (taskType) {
Copy link

Choose a reason for hiding this comment

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

I like the use of switch statement here for efficiency!

public String execute(TaskList tasks, Ui ui, Storage storage) throws FeaException {
try {
int taskNum = Integer.parseInt(fullCommand.substring(7));
if (taskNum > tasks.size() || taskNum < 1) {
Copy link

Choose a reason for hiding this comment

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

Maybe you can change the logic to avoid complicated expressions?

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.

None yet

4 participants