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

[jasperng] iP #461

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

Conversation

jasperng-nus
Copy link

@jasperng-nus jasperng-nus commented Feb 5, 2024

Luke

“Learn from yesterday, live for today, hope for tomorrow. The important thing is not to stop questioning.” – Albert Einstein (source)

  • text based
  • easy to use
  • SUPER FAST to use
  1. download from here.
  2. double click it.
  3. add your tasks.
  4. let it manage yours tasks for you 👍

And it is FREE!

Features:

  • managing task
  • add task
  • delete task

Note

There some things to note.

this is an inline code it has back-ticks around it

public class Main {
    public static void main(String[] args) {
        System.out.println("Hello World!");
    }
}

damithc and others added 11 commits January 7, 2024 18:33
Let's tweak the docs/README.md (which is used as the user guide)
to fit Duke better. Specifically,

1. mention product name in the title
2. mention adding a product screenshot and a product intro
3. tweak the flow to describe feature-by-feature
@jasperng-nus jasperng-nus changed the title jasperng-nus IP [jasperng] IP Feb 5, 2024
@jasperng-nus jasperng-nus changed the title [jasperng] IP [jasperng] iP Feb 5, 2024
Copy link

@Austintjh19 Austintjh19 left a comment

Choose a reason for hiding this comment

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

Good start. The basics are there just some modifications to the Luke class are needed.

@@ -0,0 +1,13 @@
public class Deadline extends Task {
protected String by;

Choose a reason for hiding this comment

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

Is using protected the most suitable option ? It makes it accessible to other classes from the same package ... I noticed the same issue in several other places too.

@@ -0,0 +1,16 @@
public class Event extends Task{

Choose a reason for hiding this comment

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

Whitespace should proceed { , according to our coding standard. Just something to take note.


// Conditions
while (!input.equals("bye")) {
if (input.equals("list")) {

Choose a reason for hiding this comment

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

Maybe consider creating functions to execute the commands ? E.g. when input.equals("list ") it call a separate function like outputTasks(). You can possibly perform you validation checks in these functions as well.

import java.util.ArrayList;
import java.util.Scanner;

public class Luke {

Choose a reason for hiding this comment

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

Maybe enhance the class structure by decomposing the functionality into separate classes such as InputValidator for validating user inputs, Outputs for managing output operations, and InputInterpreter for processing user commands, thereby promoting a clearer separation of concerns.


// Conditions
while (!input.equals("bye")) {
if (input.equals("list")) {

Choose a reason for hiding this comment

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

Consider switching to a case statement it can potentially enhance code readability.

int index = Integer.parseInt(instruction[1]) - 1; // array is 0-indexed

if (index >= list.size()) {
throw new LukeException("Hold up!! There is no such task in the list.\n"

Choose a reason for hiding this comment

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

Nice to see you are already incorporating Exceptions.

@@ -0,0 +1,5 @@
public class TaskException extends Exception {

Choose a reason for hiding this comment

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

Maybe have this extend LukeException ? e.g. LukeTaskException. Won't have to check for LukeException | TaskException for each relevant exception.

}
System.out.println("________________________________________________________________________");

} else if (input.contains("mark")) {

Choose a reason for hiding this comment

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

Might be a redundant if check. You can just split the string and verify if the mark/ unmark is present, while also ensuring the length of the split instruction is 2. This way, you confirm the task name's presence without extra checks. This might be applicable for other commands as well.

// User inputs
Scanner sc = new Scanner(System.in);
String input = sc.nextLine();
ArrayList<Task> list = new ArrayList<>();

Choose a reason for hiding this comment

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

Possibly a more appropriate variable name to make the code more readable.


} else {
try {
if (input.equals("todo") || input.equals("deadline") || input.equals("event")) {

Choose a reason for hiding this comment

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

Maybe consider the case where user enters "todo " or "deadline " or "event ".

Copy link

@Austintjh19 Austintjh19 left a comment

Choose a reason for hiding this comment

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

Great work on coding standards. Seems like it was well followed.

@@ -0,0 +1,13 @@
public class Deadline extends Task {

Choose a reason for hiding this comment

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

JavaDoc comments of classes and functions would be nice.

@Override
public String toString() {
return "[E]" + super.toString() + " (from: " + this.from + " to: " + this.to + ")";

Choose a reason for hiding this comment

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

Trailing whitespaces are quite prevalent in your code. Just remove them and you're good.

import luke.exception.FileException;
import luke.exception.LukeException;
import luke.exception.TaskException;
import luke.task.*;

Choose a reason for hiding this comment

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

Would it be better if you split the wildcard import and instead import each module explicitly?

* @param ui The user interface.
* @param storage The storage of the tasks.
*/
public void parse(TaskList list, Ui ui, Storage storage) {

Choose a reason for hiding this comment

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

Will a switch-case work better instead of if-else blocks?

Comment on lines 102 to 115
} else if (input.contains("delete")) {
try {
if (input.equals("delete")) {
throw new LukeException("Hold up!! There must be a task to delete!\n"
+ "Please enter an index after " + input + ".");
}

try {
String[] instruction = input.split(" ");
String delete = instruction[0];

if (!delete.equals("delete")) {
throw new LukeException("Hold up!! I am sorry, but I don't know what you mean by that :'(");
}

Choose a reason for hiding this comment

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

How about using ".startsWith("delete")" instead to check for commands? That way, you wouldn't need a separate if equals block.

Comment on lines 171 to 177
// 6 is the index after "event ", so starts from index 6
// -1 to remove the space before "/from"
String description = input.substring(6, input.indexOf("/from") - 1);
// +6 to remove the "/from " and start from the index after "/from "
// -1 to remove the space before "/to"
String from = input.substring(input.indexOf("/from") + 6, input.indexOf("/to") - 1);
// +4 to remove the "/to " and start from the index after "/to "

Choose a reason for hiding this comment

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

I like that you use comments to explain the purpose of your code.

Comment on lines +3 to +7
import luke.exception.DateException;
import luke.parser.DateTimeParser;

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.

I like that you separated the imports by packages.

Choose a reason for hiding this comment

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

Would it be better to have a separate function of constant variable to print the horizontal lines instead?

jasperng-nus and others added 16 commits February 16, 2024 08:56
Ensuring consistent coding standards is crucial for code quality and readability.

Applied CheckStyle rules to the entire codebase. Resolved CheckStyle violations related to indentation, naming conventions, and coding style.

Enforcing CheckStyle rules ensures a clean and consistent codebase, making it easier to maintain and understand.
The readTask() method is too long.

To enhance code maintainability and readability, it is necessary to address the existing method's length and complexity.

Using a new method handleMarkOrUnmark(), to handle the conditional statement to shorten the method length and make it more readable.
The current situation lacks assumption in the Storage and Luke classes.

Introduce assertions at various points to ensure that important assumptions are made before running the rest of the code.
Add assertions into Storage and Luke classes
Add more methods to separate and shorten the parse method
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.

4 participants