[AY1920S1-CS2113T-W13-3] SGTravel by Jefferson111 · Pull Request #71 · nusCS2113-AY1920S1/PersonalAssistant-Duke

Jefferson111 · 2019-09-28T01:34:15Z

@Sukrut1881
@hongchuan97
@Inno97
@Jefferson111

okkhoy · 2019-10-14T08:30:37Z

@Sukrut1881
@hongchuan97
@Inno97
@Jefferson111

if this is your official PR to the upstream repo, please title it in English!

okkhoy

Overall decent code. However, I could spot quite a few coding std violations. Do take care!

BTW, I didn't see much (any) test code? Please ensure you write test code as and when you implement your features.

okkhoy · 2019-10-22T06:47:28Z

src/main/java/duke/Main.java

+            mainWindow.show();
+            mainWindow.initialise(this);
+        } catch (Exception e) {
+            e.printStackTrace();


printing stacktrace is not good exception handling

okkhoy · 2019-10-22T06:49:40Z

src/main/java/duke/commons/Messages.java

+ * Messages used by duke.Duke.
+ */
+public class Messages {
+    public static final String UNKNOWN_COMMAND = "☹ I'm sorry, but I don't know what that means :-(";


Need to follow naming convention here.
Think if you can categorize these commands in some way.

are they related to a feature? if so can you name them as Feature_<message type>_<variable>

are they related to a particular type of message? e.g. info, error, etc.,

okkhoy · 2019-10-22T06:50:01Z

src/main/java/duke/commons/MessagesPrompt.java

+    public static final String PROMPT_ERROR = "Sorry, but something went wrong...";
+    public static final String PROMPT_TOO_MANY_ATTEMPTS = "Sorry, but you have exceeded 5 attempts...";
+    public static final String PROMPT_SPACES = "Please do not include spaces in your search!";
+    public static final String PROMPT_NOT_INT = "Please use a number!";


Apply this convention above, can?

okkhoy · 2019-10-22T06:50:24Z

src/main/java/duke/commons/enumerations/Constraint.java

@@ -0,0 +1,8 @@
+package duke.commons.enumerations;
+
+public enum Constraint {


header comments please!

okkhoy · 2019-10-22T06:50:48Z

src/main/java/duke/commons/enumerations/TimePatternType.java

+ * Enumerations for different specificity of time.
+ */
+public enum TimePatternType {
+    DAY_OF_WEEK, DATE_TIME, DATE, TIME


follow a standard convention to write enums.

okkhoy · 2019-10-22T07:03:38Z

src/main/java/duke/logic/api/requests/LocationSearchUrlRequest.java

+ * URL request to OneMap API to get coordinates of location.
+ */
+public class LocationSearchUrlRequest extends UrlRequest {
+    private static final String paramType = "searchVal";


variable name for "constants" don't follow the guideline

src/main/java/duke/logic/commands/ExitCommand.java

okkhoy · 2019-10-22T07:05:42Z

src/main/java/duke/logic/commands/GetBusRouteCommand.java

+            }
+            return new CommandResultText(MESSAGE_BUS_ROUTE + result);
+
+        } catch (Throwable e) {


woah! waay too generic. please use specific exception class

okkhoy · 2019-10-22T07:07:31Z

src/main/java/duke/logic/commands/MarkDoneCommand.java

+    }
+
+    /**
+     * Executes this command on the given task list and user interface.


may want to say what command is "this command"

okkhoy · 2019-10-22T07:07:42Z

src/main/java/duke/logic/commands/PromptCommand.java

+import duke.logic.commands.results.CommandResultText;
+import duke.model.Model;
+
+public class PromptCommand extends Command {


header comments missing

src/main/java/duke/logic/commands/ViewScheduleCommand.java

okkhoy · 2019-10-22T07:19:08Z

src/main/java/duke/logic/commands/results/CommandResult.java

@@ -0,0 +1,9 @@
+package duke.logic.commands.results;
+
+public abstract class CommandResult {


abstract classes MUST have header comments. it is like a contract between the component and the client class.
how will the client class implementer know what to expect without documentation?

okkhoy · 2019-10-22T07:19:48Z

src/main/java/duke/logic/commands/results/Imageable.java

+
+import javafx.scene.image.Image;
+
+public interface Imageable {


What is imageable?

We are unsure of how to name the interface, it is to mean the class that implements the interface should contains an Image and a method to get the image

okkhoy · 2019-10-22T07:20:00Z

src/main/java/duke/logic/commands/results/Taskable.java

+
+import duke.model.TaskList;
+
+public interface Taskable {


What is taskable?

We are unsure of how to name the interface, it is to mean the class that implements the interface should contains tasks and a method to get/set the tasks.

okkhoy · 2019-10-22T07:21:16Z

src/main/java/duke/logic/conversations/ConversationManager.java

+    /**
+     * Creates a Conversation object based on input.
+     *
+     * @param input The words from user input.


need to advertise the exception being thrown

src/main/java/duke/model/events/Event.java

okkhoy · 2019-10-22T07:43:18Z

src/main/java/duke/model/transports/RouteNode.java

@@ -0,0 +1,70 @@
+package duke.model.transports;
+
+import duke.commons.enumerations.Constraint;


this part of the code looks well written 👍

okkhoy · 2019-10-22T07:44:07Z

src/main/java/duke/storage/Storage.java

+        boolean isBusData = false;
+        while (s.hasNext()) {
+            String line = s.nextLine();
+            if ("==========".equals(line)) {


?? any specific reason? do you want to document?

okkhoy · 2019-10-22T07:44:59Z

src/main/java/duke/ui/calendar/CalendarWindow.java

+import duke.model.events.Task;
+import duke.model.events.TaskWithDates;
+import duke.ui.UiPart;
+import javafx.fxml.FXML;


give a line space to indicate logical grouping of imports.

okkhoy · 2019-10-22T07:45:47Z

src/main/java/duke/ui/map/LocationCard.java

+
+    private LocationCard(Venue location) {
+        super(FXML);
+        double offsetY = 600 - ((location.getLatitude() - 1.218) * 600 / (1.486 - 1.218));


can you make this size agnostic? things many not appear the same way on all resolution screens

Jefferson111 changed the title ~~[AY1920S1-CS2113T-W13-3] 天気の子見ましたか?~~ [AY1920S1-CS2113T-W13-3] SGTravel Oct 14, 2019

nus-se-pr-bot added team.3 tutorial.CS2113T-W13 tutor.okkhoy labels Oct 17, 2019

okkhoy reviewed Oct 22, 2019

View reviewed changes

Jefferson111 and others added 19 commits November 4, 2019 21:12

Remove unused import statements

e565376

Add javadoc

9fa1643

Fix display wrong info to user bug

4a1fe83

Command advertise exceptions

45301f9

Add javadoc

aeb30d5

Add java docs

332d26c

Fix ex

4c2fce4

Fix exceptions thrown

9d44027

Remove unsused imports

ba36bf2

Highlight exceptions

ae526b9

Fix exceptions thrown

1ad4a63

Remove unused imports

c397ada

Remove unused imports

6d23956

Cleanup commons package

f388467

Fix recommendation day too long bug

d92d98c

Remove observableLists

27892d3

Update UserGuide.adoc

5d1c3ed

Fix checkstyle

4878d6f

Update stub to not write

9234cdd

Jefferson111 and others added 30 commits November 10, 2019 23:01

Update image

9d9e022

Merge branch 'master' of https://github.com/AY1920S1-CS2113T-W13-3/main

6f7e2b0

Update UserGuide.adoc

926f1e6

Update UserGuide.adoc

8bd967c

Update UserGuide.adoc

8799332

Update UserGuide.adoc

675142d

Update UserGuide.adoc

1b9b061

Update DeveloperGuide.adoc

740ffb9

Add files via upload

5ca35be

Update DeveloperGuide.adoc

98f4b5d

update developerGuide

25f3757

update developerGuide.adoc

95ab3c0

Update UserGuide.adoc

e785790

Update DeveloperGuide.adoc

c4048e4

Update DeveloperGuide.adoc

d822710

Add files via upload

00c4b9f

Update README.adoc

7a76d7a

Update README.adoc

716c6b6

Update README.adoc

20c4883

Fix travis

f439ac1

Rename and fix travis

365ea20

Code quality

f248687

Fix tests

948892b

Update UserGuide.adoc

2fa4024

Update UserGuide.adoc

fb453fa

Update UserGuide.adoc

8458643

Update UserGuide.adoc

3a5b2ff

Update UserGuide.adoc

a3c8b2b

Update DeveloperGuide.adoc

ac833ad

Update DeveloperGuide.adoc

74a56d4

		@@ -0,0 +1,8 @@
		package duke.commons.enumerations;

		public enum Constraint {

		@@ -0,0 +1,9 @@
		package duke.logic.commands.results;

		public abstract class CommandResult {


		import javafx.scene.image.Image;

		public interface Imageable {

		@@ -0,0 +1,70 @@
		package duke.model.transports;

		import duke.commons.enumerations.Constraint;

Conversation

Jefferson111 commented Sep 28, 2019

Uh oh!

okkhoy commented Oct 14, 2019

Uh oh!

okkhoy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefferson111 Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefferson111 Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

6 participants

okkhoy left a comment •

edited

Loading

Jefferson111 Oct 22, 2019 •

edited

Loading

Jefferson111 Oct 22, 2019 •

edited

Loading