-
Notifications
You must be signed in to change notification settings - Fork 484
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
[SinhaVedant] iP #567
base: master
Are you sure you want to change the base?
[SinhaVedant] iP #567
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from a couple of minor problems with regards to coding style, your code is easy to read overall.
src/main/java/duke/Duke.java
Outdated
|
||
System.out.println(dashLine); | ||
System.out.println(" Got it. I've added this task:"); | ||
System.out.println(" " + taskList.get(taskList.size() - 1)); | ||
System.out.println(" Now you have " + taskList.size() + " tasks in the list."); | ||
System.out.println(dashLine); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some unnecessary indentation here
src/main/java/duke/Duke.java
Outdated
switch (taskType) { | ||
case 'T': | ||
ToDo toDoTask = new ToDo(description, taskType); | ||
if (isDone) { | ||
toDoTask.markAsDone(); | ||
} else { | ||
toDoTask.markAsNotDone(); | ||
} | ||
return toDoTask; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch (taskType) { | |
case 'T': | |
ToDo toDoTask = new ToDo(description, taskType); | |
if (isDone) { | |
toDoTask.markAsDone(); | |
} else { | |
toDoTask.markAsNotDone(); | |
} | |
return toDoTask; | |
switch (taskType) { | |
case 'T': |
If i'm not wrong, the 'case' line should not have indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall a reasonable implementation, with a few coding standard nits
@@ -13,7 +13,7 @@ Prerequisites: JDK 11, update Intellij to the most recent version. | |||
1. If there are any further prompts, accept the defaults. | |||
1. Configure the project to use **JDK 11** (not other versions) as explained in [here](https://www.jetbrains.com/help/idea/sdk.html#set-up-jdk).<br> | |||
In the same dialog, set the **Project language level** field to the `SDK default` option. | |||
3. After that, locate the `src/main/java/Duke.java` file, right-click it, and choose `Run Duke.main()` (if the code editor is showing compile errors, try restarting the IDE). If the setup is correct, you should see something like the below as the output: | |||
3. After that, locate the `src/main/java/duke.Duke.java` file, right-click it, and choose `Run duke.Duke.main()` (if the code editor is showing compile errors, try restarting the IDE). If the setup is correct, you should see something like the below as the output: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is a typo that might've been introduced by IntelliJ's auto-refactoring process?
import java.io.IOException; | ||
|
||
|
||
public class Duke { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add header comments to public classes
Branch a code quality
Sivraj
Sivraj frees your mind of having to remember things you need to do. It's
FASTSUPER FAST to use.All you need to do is,
And it is FREE
Features:
If you are a Java programmer, you can use it to practice Java too. Here's the
main
method: