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-CS2113-T16-2] El Duque #35

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

Conversation

nicholasnovakovic
Copy link

x3chillax referenced this pull request in x3chillax/main Oct 16, 2019
@nicholasnovakovic nicholasnovakovic changed the title [AY1920S1-CS2113-T16-2] Task Manager [AY1920S1-CS2113-T16-2] El Duque Oct 16, 2019
Copy link

@sanjukta99 sanjukta99 left a comment

Choose a reason for hiding this comment

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

Take a look at my comments for some suggestions about your code. Keep in mind the Java Coding standards.
I have noticed that most of the commands so far are the Duke commands. I hope you are working on your new features already. Feel free to speak to us if you are facing any difficulty!



public AddCommand(String description, String taskType, Optional<String> filter) {
this.taskType = taskType;

Choose a reason for hiding this comment

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

Important that you guys have JavaDoc comments for all classes. You can find the Java Coding Standards on the moduel website

}
if (filter == tempTaskList.get(i).getFilter()) {
filteredListCounter++;
}

Choose a reason for hiding this comment

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

Is the indentation for this if statement correct?

String keyword = fcArray[0];

switch (keyword) {
case "bye":

Choose a reason for hiding this comment

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

There is no indentation for case clauses in Java. Configure CheckStyle to follow this style

* .create();
* }</pre>
* Like {@code GsonBuilder}, this API supports chaining: <pre> {@code
* RuntimeTypeAdapterFactory<Shape> shapeAdapterFactory = RuntimeTypeAdapterFactory.of(Shape.class)

Choose a reason for hiding this comment

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

Best to delete commented out code. Makes your code more readable

}

public void setPriority(int i) throws DukeException {
switch(i) {

Choose a reason for hiding this comment

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

Could the getting and setting of priority be handled elsewhere? (take note of SoC)

// assertEquals(LocalDate.of(2019, 10, 19), a.convertToLocalDate("19102019 1000"));
// final AddCommand b = new AddCommand("homework", "19012019 1000");
// assertEquals(LocalDate.of(2019, 01, 19), a.convertToLocalDate("19012019 1000"));
}

Choose a reason for hiding this comment

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

Delete commented out code

public class EditCommandTest {

// @Test

Choose a reason for hiding this comment

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

I would suggest that you guys write some test code, as and when you update the code

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.

5 participants