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.

Sign up for GitHub

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

Jump to bottom

Tan Eu Zin iP #470

Open

euzintan wants to merge 39 commits into nus-cs2103-AY2021S1:master from euzintan:master

euzintan commented Aug 31, 2020

No description provided.

damithc and others added 12 commits

July 23, 2020 23:27


          Add Gradle support

3b19ba1


          This is level 1, I hope im committing this correctly.

5fa8ff1


          Level 2

913c098


          Level 2

193dc5b


          Revert "Level 2"

41f3eac

This reverts commit 193dc5b.


          Level 3

f699f2b


          Level 4:

fc87ead

Added classes Event and Deadline
Changed response to print via toString()
Added a counter that tells users number of things in list


          Merge remote-tracking branch 'origin/master' into master

60c2438


          Added Automated Text UI Testing


          Implemented Duke specific extensions

4deb825


          Implemented Delete

1cd2bc0


          Updated Exceptions to continue accepting input after exceptions thrown

0c53018

BILLXYR reviewed

View reviewed changes

BILLXYR left a comment

Two main problems observed: "the websites says: "Class variables should never be declared public"
So, perhaps it should not public." and " Plural form should be used on names representing a collection of objects." Others are fine 👍 Good job 👍

src/main/java/Duke.java Outdated

-                      System.out.println("Hello from\n" + logo);
+                      System.out.println(logo + "\nHello im Eu Zin's Duke, he spent thursday afternoon creating me cuz he forgot abt the iP");
+                      ArrayList<Task> taskList = new ArrayList<>();

BILLXYR Aug 31, 2020

From the websites it says that : "Plural form should be used on names representing a collection of objects."
So I think perhaps it should be taskLists instead of taskList.

src/main/java/Deadline.java

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

BILLXYR Aug 31, 2020

the websites says: "Class variables should never be declared public"
So, perhaps it should not public.

src/main/java/Duke.java

+              import java.util.ArrayList;
+              import java.util.Iterator;
+              import java.util.Scanner;
               public class Duke {

BILLXYR Aug 31, 2020

the websites says: "Class variables should never be declared public"
So, perhaps it should not public.

src/main/java/DukeException.java

		@@ -0,0 +1,7 @@
		public class DukeException extends Throwable {

BILLXYR Aug 31, 2020

the websites says: "Class variables should never be declared public"
So, perhaps it should not public.

src/main/java/Event.java

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

BILLXYR Aug 31, 2020

the websites says: "Class variables should never be declared public"
So, perhaps it should not public.

src/main/java/Task.java

		@@ -0,0 +1,21 @@
		public class Task {

BILLXYR Aug 31, 2020

the websites says: "Class variables should never be declared public"
So, perhaps it should not public.

euzintan added 12 commits

September 1, 2020 15:36


          Created new class Storage to deal with saving and loading

30be8f6

Implemented saving to and laoding from hard disk function


          Created new class Storage to deal with saving and loading

c290955

Implemented saving to and laoding from hard disk function


          Changed Deadline class so Duke can deal with LocalDate and LocalTime …

37b18f1

…rather than string


          Merge branch 'branch-Level-8'

5c8e64f


          Resolved conflicts caused during merging of save to harddisk feature …

59863e8

…and recognising LocalDate feature


          Created Parser and Ui classes

cc4252f


          Added JUnit Tests for Parser

c6fb802


          Added JavaDoc Comments

aef36dd


          Implemented Find feature

cb2a2ce


          Made changes to comply to Coding Standards

bb86215


          Resolved conflicts when merging increment branches

e1d5ed0

# Conflicts:
#	src/main/java/TaskList.java
#	src/main/java/Ui.java


          Merge remote-tracking branch 'origin/add-gradle-support'

46425bf

juandavinlie reviewed

View reviewed changes

juandavinlie left a comment

Overall, your code is well written and understandable. Some names of variables and methods can be modified to make it self-explanatory. Good job!!

src/main/java/TaskList.java Outdated

+                              boolean isDone = taskDataDivided[2].equals("1");
+                              switch (taskDataDivided[0]) {
+                                  case "E":
+                                      returnTaskList.add(new Event(taskDataDivided[2], taskDataDivided[3], isDone));

juandavinlie Sep 1, 2020

Should taskDataDivided[0, 2, 3] be assigned to a variable that is self-descriptive? It would make the code easier to read and understand.

src/main/java/TaskList.java

+                   * @return copy of ArrayList
+                   */
+                  public ArrayList<Task> getList() {
+                      ArrayList<Task> returnTaskList = new ArrayList<>();

juandavinlie Sep 1, 2020

It might be more appropriate to name this variable as taskList instead of returnTaskList since return is a verb and could refer to a function.

src/main/java/TaskList.java

Comment on lines +76 to +77

		* Add Task given to the list
		* @param task

juandavinlie Sep 1, 2020

It is good to separate the description and parameter using a blank line.

src/main/java/TaskList.java

+                  public ArrayList<Task> searchFor(String searchString) {
+                      ArrayList<Task> returnArrayList = new ArrayList<>();
+                      for (Task task : taskList) {
+                          if (task.getDescription().contains(searchString)) returnArrayList.add(task);

juandavinlie Sep 1, 2020

It may be better to use the Egyptian style when writing if-else statement as it becomes easier to read.

src/main/java/TaskList.java

+                      return taskList.size();
+                  }
+                  public ArrayList<Task> searchFor(String searchString) {

juandavinlie Sep 1, 2020

May be better to name it searchTaskWithName(String searchString), as it is understandable by just reading it.

euzintan added 4 commits

September 2, 2020 12:05


          Prepared Duke for GUI, tested out Hello World

dc639f2


          Created Working GUI for Duke

66408eb

All features of Duke available on GUI


          Added assertions in Parser class to make sure input of the user is ap…

623bee8

…propriate before any processing


          Improve Code Quality

e747751

euzintan and others added 11 commits

September 8, 2020 02:10


          Created shadowJar file

779e689


          Merge pull request #1 from euzintan/branch-Assertions

5ea632b

Pull Request for Assertions


          Merge branch 'branch-A-CodeQuality'

e5eacc7

# Conflicts:
#	src/main/java/Deadline.java
#	src/main/java/Duke.java


          Implemented FixedDurationTasks so that durations can be added to all …

cd290bf

…types of tasks


          SLAPed harder to reduce abstraction problems

b1d46b4

Made Exceptions more explicit so the user can know how to enter an appropriate command.


          Set theme jekyll-theme-slate

d21247f


          Edited README.md to be a proper User Guide

e61f350


          Merge from GitHub to change theme

56b2c52


          Improved User Guide

8d9ad84

Added Product showcase screenshot


          Changed Ui Screenshot

dda2132


          Fixed Duke to work with Mac

a9f9480

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet