Mor Ben Ami #66

morbenami1 · 2022-08-30T10:38:24Z

level 1,2,3

CheahHaoYi

Nice

CheahHaoYi · 2022-09-02T03:42:01Z

src/main/java/List.java

+
+
+    public List() {
+        tasks = new Task[100];


The use of magic number here is ambiguous, feel free to consider using a constant

CheahHaoYi · 2022-09-02T03:44:30Z

src/main/java/Task.java

@@ -0,0 +1,33 @@
+public class Task {
+    private String description;
+    private boolean marking;


boolean values should be named to sound like booleans, consider "isMarked" or "isFinished"

CheahHaoYi · 2022-09-02T03:48:50Z

src/main/java/Task.java

+        marking = false;
+    }
+
+    public void markDone() {


Perhaps it would be better to have a method to print the string, rather than having a function that does both setting the boolean and printing

CheahHaoYi · 2022-09-02T03:49:30Z

src/main/java/Duke.java

+        line = in.nextLine();
+        while (!line.equals("bye")){
+            words = line.split(" ");
+            if (line.equals("list")){


Consider using a switch-case statement for neatness

kaseykwok

Nice work. Hope my review could help you well.

kaseykwok · 2022-09-02T03:57:08Z

src/main/java/Task.java

@@ -0,0 +1,33 @@
+public class Task {
+    private String description;
+    private boolean marking;


Consider changing the naming of the boolean variable to "isXXX".

kaseykwok · 2022-09-02T04:01:41Z

src/main/java/Message.java

@@ -0,0 +1,25 @@
+public class Message {
+    public static void printingGreeting() {


Consider naming the function's name as printGreeting() instead of printingGreeting().

kaseykwok · 2022-09-02T04:03:09Z

src/main/java/Message.java

+        printingHorizontalLine();
+    }
+
+    public static void printingExit() {


May change the function name to printExit()

kaseykwok · 2022-09-02T04:03:38Z

src/main/java/Message.java

+        printingHorizontalLine();
+    }
+
+    public static void printingHorizontalLine() {


May change the function name to printHorizontalLine()

kaseykwok · 2022-09-02T04:04:10Z

src/main/java/Message.java

+        System.out.println("-----------------------------------------------------------");
+    }
+
+    public static void printingEcho(String line) {


May change the function name to printEcho()

kaseykwok · 2022-09-02T04:10:57Z

src/main/java/List.java

+    public void addTask(String input) {
+        if (input.contains("/by") || input.split(" ")[0].equals("deadline")) {
+            if (input.split(" ", 2)[0].equals("deadline")) {
+                input = input.split(" ", 2)[1];


Avoid recycling variable. Consider declaring another variable String description to store instead of using input again.

kaseykwok · 2022-09-02T04:11:42Z

src/main/java/List.java

+
+
+    public List() {
+        tasks = new Task[100];


Avoid using magic literals. Consider declaring a constant for it.

RiaVora · 2022-09-15T12:58:17Z

src/main/java/duke/Duke.java

+        try {
+            wordsInput = input.split(space);
+        } catch (ArrayIndexOutOfBoundsException e) {
+            throw new DukeException();


consider adding a message to the Duke Exception so you can send a direct error message to the user!

RiaVora · 2022-09-15T12:59:15Z

src/main/java/duke/Message.java

@@ -0,0 +1,35 @@
+package duke;
+public class Message {


I really like the fact that you made a Message class! Wish I did this in mine because it would be easier to read many of my methods :)

RiaVora · 2022-09-15T13:00:41Z

src/main/java/duke/Duke.java

+    public static void echo() {
+        String line;
+        Scanner in = new Scanner(System.in);
+        line = in.nextLine();


maybe consider doing in.nextLine().toLowerCase() so it still works if the user accidentally makes anything uppercase

RiaVora · 2022-09-15T13:01:24Z

src/main/java/duke/Duke.java

+        } catch (NumberFormatException e) {
+            throw new DukeException();
+        }
+        if (taskNumber > dukeList.getListSize()) {


make you can also put this in the try, since you are throwing an exception as well (might make it easier to read). not necessary at all though

RiaVora · 2022-09-15T13:02:52Z

src/main/java/duke/task/Task.java

+
+    public void markDone() {
+        System.out.println("Nice! I've marked this task as done:");
+        isDone = true;


maybe you can make isDone = "X" or a " " since it would slightly shorten your code. Whenever you want to check isDone is true, you can just check if isDone == "X" instead. not necessary for functionality!

sevenseasofbri

Hi Mor Ben Ami, good job on the iP so far! I have added some comments on the code quality, coding standard violations and naming conventions. Hope they will be helpful. 👍🏽

sevenseasofbri · 2022-09-16T14:45:43Z

src/main/java/duke/Duke.java

+    private static final List dukeList = new List();
+    public static final String markDone = "mark";
+    public static final String bye = "bye";
+    public static final String list = "list";
+    public static final String space = " ";
+    public static final String unmarkDone = "unmark";


Please follow the naming convention for final variables. That is, the names should be all-caps.

sevenseasofbri · 2022-09-16T14:46:17Z

src/main/java/duke/Duke.java

+    public static final String unmarkDone = "unmark";
+
+    public static void main(String[] args) {
+        String logo = " ____        _        \n"


Consider refactoring this magic literal to a final variable in the class.

sevenseasofbri · 2022-09-16T14:55:58Z

src/main/java/duke/List.java

+    public static final int arraySize = 100;
+    private static int amountOfItems = 0;
+    private static duke.task.Task[] tasks;
+    public static final String deadline = "deadline";
+    public static final String todo = "todo";
+    public static final String event = "event";
+    public static final String by = " /by ";
+    public static final String at = " /at ";
+    public static final String space = " ";


Consider using the correct naming convention for final variables.

sevenseasofbri · 2022-09-16T14:57:21Z

src/main/java/duke/List.java

+        if (divideByFirstSpace.length < 2 || divideByFirstSpace[1].equals("")){
+            throw new DukeException();
+        }
+        switch (divideByFirstSpace[0]) {


Perhaps store divideByFirstSpace[0] in a variable and then perform switch-case using it. It will make the code more understandable to the reader.

sevenseasofbri · 2022-09-16T14:58:46Z

src/main/java/duke/List.java

+    public void markItemDone(int i) {
+        tasks[i - 1].markDone();
+    }
+
+    public void unmarkItemDone(int i) {
+        tasks[i - 1].unmarkDone();
+    }


Avoid naming variables that are not counters non-descriptive names like i. Consider naming it itemNum or something along those lines.

sevenseasofbri · 2022-09-16T15:09:36Z

src/main/java/duke/Message.java

+        printHorizontalLine();
+    }
+
+    public static void printingExit() {


Consider renaming this function to printExit rather than printingExit

# Conflicts: # src/main/java/duke/List.java

added javaDoc

# Conflicts: # src/main/java/duke/TaskList.java # src/main/java/duke/task/Task.java

added find

morbenami1 added 7 commits August 27, 2022 20:00

Level-0

3936521

Level 1

2441777

Level-1

d9d4dd1

Level-2

457aa97

Level-3

9a93152

level 4 - todo, deadline and event

663e04a

level 4 - added deadline, todo and event

ebce00b

CheahHaoYi reviewed Sep 2, 2022

View reviewed changes

kaseykwok reviewed Sep 2, 2022

View reviewed changes

morbenami1 added 4 commits September 12, 2022 16:46

Level-5

9b10885

A-Packages

fa0a0b7

A-Packages

3014403

delete added

a75f0e9

RiaVora reviewed Sep 15, 2022

View reviewed changes

sevenseasofbri reviewed Sep 16, 2022

View reviewed changes

morbenami1 and others added 11 commits September 17, 2022 12:32

file(level-7)

384558f

Merge level-6

1863a24

Merge branch 'branch-Level-7'

4415c2e

# Conflicts: # src/main/java/duke/List.java

merge start

cc20b63

commit merge try

9aea67a

delete added

96e1a6a

level 6 and level 7

ffefb0f

A-MoreOOP

62c739f

added find

4cea348

added javaDoc

72f1c66

Merge pull request #1 from morbenami1/A-JavaDoc

11ac390

added javaDoc

morbenami1 and others added 5 commits September 30, 2022 17:20

Merge branch 'master' into branch-Level-9

76f79c2

# Conflicts: # src/main/java/duke/TaskList.java # src/main/java/duke/task/Task.java

Merge pull request #2 from morbenami1/branch-Level-9

31ee941

added find

finishing touches

e88fde8

A-UserGuide

b7f67af

Update README.md

586a162

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mor Ben Ami #66

Mor Ben Ami #66

morbenami1 commented Aug 30, 2022

CheahHaoYi left a comment

CheahHaoYi Sep 2, 2022

CheahHaoYi Sep 2, 2022

CheahHaoYi Sep 2, 2022

CheahHaoYi Sep 2, 2022

kaseykwok left a comment

kaseykwok Sep 2, 2022

kaseykwok Sep 2, 2022

kaseykwok Sep 2, 2022

kaseykwok Sep 2, 2022

kaseykwok Sep 2, 2022

kaseykwok Sep 2, 2022

kaseykwok Sep 2, 2022

RiaVora Sep 15, 2022

RiaVora Sep 15, 2022

RiaVora Sep 15, 2022

RiaVora Sep 15, 2022

RiaVora Sep 15, 2022

sevenseasofbri left a comment

sevenseasofbri Sep 16, 2022

sevenseasofbri Sep 16, 2022

sevenseasofbri Sep 16, 2022

sevenseasofbri Sep 16, 2022

sevenseasofbri Sep 16, 2022

sevenseasofbri Sep 16, 2022

		@@ -0,0 +1,25 @@
		public class Message {
		public static void printingGreeting() {

Mor Ben Ami #66

Are you sure you want to change the base?

Mor Ben Ami #66

Conversation

morbenami1 commented Aug 30, 2022

CheahHaoYi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaseykwok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sevenseasofbri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment