Skip to content

ParthGandhiNUS iP#174

Open
ParthGandhiNUS wants to merge 62 commits into
nus-cs2113-AY2324S2:masterfrom
ParthGandhiNUS:master
Open

ParthGandhiNUS iP#174
ParthGandhiNUS wants to merge 62 commits into
nus-cs2113-AY2324S2:masterfrom
ParthGandhiNUS:master

Conversation

@ParthGandhiNUS

Copy link
Copy Markdown

No description provided.

@rouvinerh rouvinerh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Overall I think it is good.

Comment thread src/main/java/Kowalski.java Outdated
public class Kowalski {
public static void main(String[] args){
Task[] currentTasks = new Task[100];
String dividingLine = "____________________________________________________________";

@rouvinerh rouvinerh Feb 8, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

dividingLine is a constant variable, and I think the variable name should be in capital letters.

Comment thread src/main/java/Kowalski.java Outdated
+ currentTasks[taskNumber-1].description);
System.out.println(dividingLine);
} else {
break;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think the repeated nested loops can be avoided by including the checking for a valid task number in another function.

Comment thread src/main/java/Kowalski.java Outdated
Task[] currentTasks = new Task[100];
String dividingLine = "____________________________________________________________";
String userInput;
int counter = 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think 'counter' can be replaced with 'taskCounter', so the name explains the variable meaning.

Comment thread src/main/java/Kowalski.java Outdated
Scanner in = new Scanner(System.in);

//Continuous loop until break statement
while (true){

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

While loop is a bit long, but readable and easily understood.

@sevenseasofbri sevenseasofbri left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Coding standards generally adhered to. Good job! 👍🏽

Comment thread src/main/java/Kowalski.java Outdated

private static final String DIVIDING_LINE = "____________________________________________________________";

public static List <Task> currentTask = new ArrayList<>();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Plural form should be used on names representing a collection of objects.

https://se-education.org/guides/conventions/java/basic.html#naming

Comment thread src/main/java/Kowalski.java Outdated
/**
* Prints out the message introducing the functionalities of Kowalski Bot
*/
public static void printIntro(){

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

printIntro() {
maintain spacing consistently in classes and methods

Comment thread src/main/java/Kowalski.java Outdated
* @param userInput : String which the user inputs
* @return String which is in lowercase and clear of any unnecessary whitespace
*/
public static String processInput(String userInput){

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

use the name to explain what the function does. perhaps you could name it trimAndMakeLowerCase() instead?
https://nus-cs2113-ay2324s2.github.io/website/se-book-adapted/chapters/codeQuality.html#use-name-to-explain

Comment thread src/main/java/Kowalski.java Outdated
* Prints out an accurate message for the number of tasks in the list.
* @param number : represents the total current task count
*/
public static void printCurrentTaskMessage(int number){

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the name number is not very descriptive, you can call is taskNumber instead ?

https://nus-cs2113-ay2324s2.github.io/website/se-book-adapted/chapters/codeQuality.html#use-name-to-explain

Comment thread src/main/java/Kowalski.java Outdated
for (int i = 0; i < deadlineArray.length; i++) {
deadlineArray[i] = deadlineArray[i].trim();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

remove one empty line (unnecessary whitespace)

Comment thread src/main/java/Kowalski.java Outdated
@@ -0,0 +1,255 @@
import java.util.Scanner;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

overall good use of javadoc comments

Comment thread src/main/java/Kowalski.java Outdated
private static Task getNewEventTask(String eventDetails) {
String[] eventArray = eventDetails.split("/from");
String eventInformation = eventArray[0].trim();
String [] fromAndTo = eventArray[1].split("/to");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

array specifiers should be attached to the type
https://se-education.org/guides/conventions/java/basic.html#types

Comment thread src/main/java/Kowalski.java Outdated
Comment on lines +103 to +104
String[] eventArray = eventDetails.split("/from");
String eventInformation = eventArray[0].trim();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comment thread src/main/java/Kowalski.java Outdated
* Processes all the inputs from the user and categorises the User Commands
* @param UserCommand: The first word input by the user
*/
public static void parseUserCommand(String UserCommand) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this function is very long (>>30 LOC). consider separating the code into multiple functions

https://nus-cs2113-ay2324s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-long-methods

Comment thread src/main/java/Kowalski.java Outdated
break;
}

try{

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

spacing try {

ParthGandhiNUS and others added 30 commits March 5, 2024 21:39
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