[Ege Demirkirkan] iP by edemirkirkan · Pull Request #71 · nus-cs2113-AY2122S2/ip

edemirkirkan · 2022-02-02T12:00:56Z

No description provided.

FTang21

Overall good job on following the coding standard

FTang21 · 2022-02-04T02:31:12Z

@@ -0,0 +1,83 @@
+public class InputParser {
+    protected String[] args;
+    protected static String[] validCommands = {"mark", "m", "unmark", "um", "todo", "t", "deadline", "d", "event", "e", "list", "ls", "bye", "exit", "quit", "q"};


I believe this is over 120 characters, perhaps make this into two separate lines?

FTang21 · 2022-02-04T02:40:06Z

+                continue;
+            }
+            String command = parser.getPrefix();
+            switch (command) {


Well done on the switch statement formatting!

suenalaba

Clean code with little violation.

suenalaba · 2022-02-04T02:35:20Z

+public class TaskManager {
+    ArrayList<Task> tasks;
+


Good: Plural form used to represent a collection of tasks. This is a common error that you avoided!

suenalaba · 2022-02-04T02:40:14Z

@@ -0,0 +1,83 @@
+public class InputParser {
+    protected String[] args;
+    protected static String[] validCommands = {"mark", "m", "unmark", "um", "todo", "t", "deadline", "d", "event", "e", "list", "ls", "bye", "exit", "quit", "q"};


Should use line wrapping instead, try to keep to a maximum of 120 characters per line.
Take note indentation for wrapped lines should be 8 spaces.

suenalaba · 2022-02-04T02:42:57Z

+            switch (command) {
+            case "mark":
+            case "m":
+                manager.markTask(parser.getBody());
+                break;
+            case "unmark":
+            case "um":
+                manager.unmarkTask(parser.getBody());
+                break;
+            case "list":
+            case "ls":
+                manager.displayTaskList();
+                break;
+            case "bye":
+            case "b":
+            case "quit":
+            case "q":
+                break label;
+            default:
+                manager.addTask(parser.getBody(), parser.getSuffix(), parser.getType());
+                break;
+            }


Good: You indented the case statement correctly, this is a common mistake which you avoid.

suenalaba · 2022-02-04T02:47:22Z

+            printWithSeparator("Nice! I've marked this task as done:", "\t[" + task.getStatusIcon() + "] "
+                    + task.getDescription());


Good use of line wrapping here so that code is kept within 120 characters per line.
Correct indentation of wrapped line too at 8 spaces.

Add input validations

xseh

Good attempt! Do take note of magic literals and some variable naming (especially when it comes to loops).

xseh · 2022-02-16T09:08:04Z

+    public static void run(Scanner scanner, TaskManager manager) {
+        manager.welcomeUser();
+        label:
+        while (true) {


Avoid magic booleans. Consider putting into variable (e.g. canContinue)

xseh · 2022-02-16T09:10:44Z

+                    manager.addTask(parser.getBody(), parser.getSuffix(), parser.getType());
+                } catch (jrobo.exception.InvalidFormatException | jrobo.exception.InvalidTypeException e) {


Avoid complicated expressions. Consider refactoring the line into separator methods.

xseh · 2022-02-16T09:12:41Z

+    }
+
+    public String getPrefix() {
+        for (String s : validCommands) {


Consider renaming the variable s in improve readability, such as command

xseh · 2022-02-16T09:14:59Z

+
+    public String getSuffix() {
+        int suffixIndex = findSuffixIndex();
+        if (suffixIndex == -1) {


Avoid magic numbers. Consider declaring a variable (e.g. INVALID_INDEX) for the integer -1 instead.

xseh · 2022-02-16T09:16:29Z

+        if (!isDeadline && !isEvent && (getPrefix().equals("todo") || getPrefix().equals("t"))) {
+            return "todo";
+        }
+        if (isDeadline && (getPrefix().equals("deadline") || getPrefix().equals("d"))) {
+            return "deadline";
+        }
+        if (isEvent && (getPrefix().equals("event") || getPrefix().equals("e"))) {
+            return "event";
+        }
+        throw new InvalidFormatException("Invalid command format!");


Consider SLAPing more by refactoring the code into separate methods.

xseh · 2022-02-16T09:17:32Z

+        int found = -1;
+        for (int i = 0; i < args.length; i++) {
+            String s = args[i];
+            if (s.equals("/at") || s.equals("/by")) {


Avoid magic literals. Consider putting strings "/at" and "/by" into variables (e.g. COMMAND_AT and COMMAND_BY)

Branch level 7

add date capabilities

add find task command

Update javadoc

Ege Demirkırkan added 9 commits January 19, 2022 05:27

Ignore .class files

03559aa

Implement initial skeletal version of JRobo

039cea9

Implement a skeletal version

2ef4b25

Implement task history

3fee1e9

Implement task management

e6cfbb5

Standardize coding style conventions

73b4a69

Implement todos, deadlines, and events

c1fb01e

Implement text-ui-test

9bd9d21

General refactor

5ad4499

FTang21 reviewed Feb 4, 2022

View reviewed changes

suenalaba reviewed Feb 4, 2022

View reviewed changes

edemirkirkan added 5 commits February 11, 2022 09:46

Add input validations

c1ec3f6

Merge pull request #1 from edemirkirkan/branch-Level-5

c805574

Add input validations

Add packages

4bac088

Merge remote-tracking branch 'origin/master'

1d6d709

Add packages

32a0838

xseh reviewed Feb 16, 2022

View reviewed changes

edemirkirkan added 13 commits February 18, 2022 08:07

Implement delete task capability

2192bdf

Implement save and load capabilities

f76aa33

Merge pull request #2 from edemirkirkan/branch-Level-7

4068eb4

Branch level 7

Creating binary JAR file

1a2c296

add storage and ui classes

2d2161c

add date capabilities

5dca803

add find task command

fc6a322

Merge pull request #3 from edemirkirkan/branch-Level-8

699b9b5

add date capabilities

Merge pull request #4 from edemirkirkan/branch-Level-9

852a9c5

add find task command

Set theme jekyll-theme-architect

2989288

Set theme jekyll-theme-tactile

d891fd5

add documentation

20632e8

Merge remote-tracking branch 'origin/master'

5ebc7f6

Ege Demirkırkan added 7 commits February 23, 2022 21:30

Set theme jekyll-theme-tactile

b291c96

change README.md

a1203f0

Merge remote-tracking branch 'origin/master'

09bdb3c

change README.md

57f7476

Update README.md

80c8dc2

Update javadoc

a1d5bd7

Merge pull request #5 from edemirkirkan/branch-A-JavaDoc

67673f0

Update javadoc

		printWithSeparator("Nice! I've marked this task as done:", "\t[" + task.getStatusIcon() + "] "
		+ task.getDescription());

		manager.addTask(parser.getBody(), parser.getSuffix(), parser.getType());
		} catch (jrobo.exception.InvalidFormatException \| jrobo.exception.InvalidTypeException e) {

Uh oh!

Conversation

edemirkirkan commented Feb 2, 2022

Uh oh!

FTang21 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suenalaba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xseh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants