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

[Zhang Yuanyu] Duke Increments #12

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

Conversation

Yuanyu97
Copy link

Pull request for Duke Individual Project levels 1-6.

j-lum and others added 30 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.
This reverts commit 254e72b.
Currently merging branch-level-8 with master(which is merged with branch-level-7 previously).
Merging Level-7 with master

/**
* You should have your own function to generate a response to user input.
* Replace this stub with your completed method.

Choose a reason for hiding this comment

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

Overall really good abstraction and encapsulation! Only a minor feedback for javadocs, perhaps you could be more consistent with the format of the comments and spacings between methods. Using gradle's checkstyle might help. But overall really clean code!

Copy link

@nishantbudhdev nishantbudhdev left a comment

Choose a reason for hiding this comment

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

Keep up the good job!
There are some minor things that to improve. Please see the individual comments.

PS: you seem to be tracking many unnecessary files in your repo. Check how you can remove them from being tracked and update the git ignore file to prevent them from being tracked. (be careful when you use git rm in the command line)

fxmlLoader.<MainWindow>getController().setDuke(duke);
stage.show();
} catch (IOException e) {
e.printStackTrace();

Choose a reason for hiding this comment

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

In general, printing stack trace is not good error handling. Think if there is anything better you can do about the exception rather than printing the stack trace.

* You should have your own function to generate a response to user input.
* Replace this stub with your completed method.
*/
public String getResponse(String input) {

Choose a reason for hiding this comment

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

Update header comments to reflect the changes.

*/
public abstract class Task {
protected String description;
protected boolean isDone;

Choose a reason for hiding this comment

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

good job on naming the boolean variable correctly

}
/**
* Returns the text to be saved into the storage file.
* @return The text to be saved to storage file.

Choose a reason for hiding this comment

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

@return is redundant as the earlier line conveys the same information.

PotatoCombat pushed a commit to PotatoCombat/duke that referenced this pull request Mar 15, 2020
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.

7 participants