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

[AY1920S1-CS2113T-W13-4] DIYeats #19

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

Conversation

ivandika3
Copy link

@ivandika3 ivandika3 changed the title [AY1920S1-CS2113T-W13-4] Duke 2.0 [AY1920S1-CS2113T-W13-4] DIYeats Oct 15, 2019
x3chillax referenced this pull request in x3chillax/main Oct 16, 2019
Copy link
Collaborator

@okkhoy okkhoy left a comment

Choose a reason for hiding this comment

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

Take care of your header comments.

  1. Many classes/non-trivial methods don't have header comments.
  2. almost all of those present violate the coding standards.

You need to rethink a bit in terms of SLAP. I can see many SLAP violations across the codebase.

Exceptions need to be more granular.

It is good that you have started off on the tests, but you need to write more tests to cover each feature. easiest to write tests as and when you update code for your features.

} else {
ui.showWelcomeBack(user);
}
while (user.getIsSetup() == false) { //setup user profile if it's empty
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can improve SLAP in this method

}
}


Copy link
Collaborator

Choose a reason for hiding this comment

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

While it is good to have vertical spacing, unnecessary vertical space is not good!


/**
* This class file defines all the filepaths that will be used in the storage component.
* @author Chua Zong Wei
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove the @author tag. We can identify your commits via VCS

*/
public class FilePaths {
public static final String sep = System.getProperty("file.separator");
public static final File DATA_FILE = new File("src" + sep + "main" + sep + "java" + sep + "duke"
Copy link
Collaborator

Choose a reason for hiding this comment

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

are you violating the naming convention for similar things?

public static final File DATA_FILE = new File("src" + sep + "main" + sep + "java" + sep + "duke"
+ sep + "Data" + sep + "duke.txt");
public static final File DEFAULTS_FILE = new File("src" + sep + "main" + sep + "java" + sep + "duke"
+ sep + "Data" + sep + "defaultItems.txt");
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should probably use a config file to specify the paths; makes it much elegant

}

@Test
public void dummyTest2() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should move past dummy tests.

Command c;
Parser parser = new Parser(autocorrect);
try {
c = parser.parse("add burger /calorie 100 /sodium 100 /fats 100");
Copy link
Collaborator

Choose a reason for hiding this comment

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

certain things can be moved to setup and teardown methods. checkout @before and @after annotations in Junit

} catch (DukeException e) {
System.out.println("Error");
}
autocorrect.setWord("calorei");
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should be testing the method that is called within execute to unit test this.

package duke.logic.parsers;

import duke.logic.autocorrect.Autocorrect;
import duke.logic.commands.*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't import using *; import classes that are necessary specifically

import static org.junit.jupiter.api.Assertions.*;

public class UserTest {
private User user = new User("Foo Chi Hen", 22, 100, Gender.MALE, 0, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

you may want to exclude personal data from public repositories. the choice, however, is completely yours.

Fractalisk and others added 23 commits November 8, 2019 13:44
…anager

# Conflicts:
#	src/main/java/duke/Data/meals.json
#	src/main/java/duke/Data/user.json
#	src/main/java/duke/logic/parsers/AddDefaultValueCommandParser.java
#	src/main/java/duke/model/Item.java
#	src/main/java/duke/model/meal/MealList.java
#	src/main/java/duke/storage/Load.java
Changed user setup to one line as per requested. Changed code accordi…
…anager

# Conflicts:
#	src/main/java/DIYeats/Data/meals.json
#	src/main/java/DIYeats/logic/commands/UserSetup.java
#	src/main/java/DIYeats/model/user/User.java
#	src/main/java/duke/Data/user.json
#	src/main/java/duke/commons/file/FilePathNames.java
#	src/test/java/DIYeats/logic/command/UserSetupTest.java
Resolve bug where date is not included and add some exception
nishanthelango pushed a commit to nishanthelango/Duchess that referenced this pull request Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants