Skip to content
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.

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

[Yap Jin Fa] iP #64

Open
wants to merge 103 commits into
base: master
Choose a base branch
from
Open

[Yap Jin Fa] iP #64

wants to merge 103 commits into from

Conversation

jinfayap
Copy link

No description provided.

j-lum and others added 21 commits August 6, 2019 15:25
Add toolVersion block in to Gradle code sample to prevent errors.
Gradle defaults to an empty stdin which results in runtime exceptions
when attempting to read from `System.in`. Let's add some sensible
defaults for students who may still need to work with the standard
input stream.
Add configuration for console applications
The OpenJFX plugin expects applications to be modular and bundled
with jlink, resulting in fat jars that are not cross-platform. Let's
manually include the required dependencies so that shadow can package
them properly.
Copy link

@jichngan jichngan left a comment

Choose a reason for hiding this comment

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

Overall, we found your code easy to read. The various constants declared made the code understandable. All the best for your iP.
[Also reviewed by @E0309556]

}
}

private static void displayBlahMessage() {
Copy link

Choose a reason for hiding this comment

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

Is this method a debug method? By removing this method, does it improve readability?

System.out.println(DIVIDER);
}

private static void displayDoneMessage(String index) {
Copy link

Choose a reason for hiding this comment

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

Perhaps can use a better variable name as index?


private static void displayDoneMessage(String index) {
int doneTaskIndex = Integer.parseInt(index) - 1;
if(Task.getTaskCount() <= doneTaskIndex){
Copy link

Choose a reason for hiding this comment

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

Missing spacing between (Task.getTaskCount() <= doneTaskIndex) and {.

@@ -0,0 +1,20 @@
public class Deadline extends Task{
Copy link

Choose a reason for hiding this comment

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

Missing spacing between Task and {.


private static void displayAddMessage() {
System.out.println(DIVIDER);
System.out.println(String.format(COMMAND_ADD_MESSAGE, tasks[Task.getTaskCount() -1], Task.getTaskCount()));
Copy link

Choose a reason for hiding this comment

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

Missing space between Task.getTaskCount() - and 1.

//Solution below adapted from https://github.com/nus-cs2113-AY1920S2/contacts/blob/master/src/main/java/Contacts1.java
public static String[] splitInputLine(String rawUserInput, String regex) {
String[] split = rawUserInput.trim().split(regex, 2);
return split.length == 2? split : new String[] {split[0], ""}; // else no parameters
Copy link

Choose a reason for hiding this comment

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

Missing space between 2 and ?.

//Solution below adapted from https://www.baeldung.com/java-add-character-to-string
protected String stringBuilder(String input) {
StringBuilder sb = new StringBuilder(input);
sb.insert(3,":");
Copy link

Choose a reason for hiding this comment

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

Is 3 a magic number is this case? Perhaps making it into a constant variable?

@jinfayap jinfayap force-pushed the master branch 4 times, most recently from 72990cb to ce91df7 Compare February 28, 2020 04:26
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.

4 participants