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

[T4A2][T17-B3]Sin Yu Fan #1691

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

howitzerg
Copy link

Change parseCommand, prepareAdd, prepareDelete, prepareView, prepareViewAll, parseArgsAsDisplayedIndex, prepareFind methods to static.

Add private sequenceNumber variable and public static nextSequenceNumber to person class.

…iewAll, parseArgsAsDisplayedIndex, prepareFind methods to static.

Add private sequenceNumber variable and public static nextSequenceNumber to person class.
Copy link

@bongbongbee bongbongbee left a comment

Choose a reason for hiding this comment

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

Since I don't see any commits on changes made to Main.java and ParserTest.java, I'm guessing you may have forgotten to change the code where your Parser.parseCommand(...) method is used. It should be changed to Parser.parseCommand(...) in Main class and ParserTest class.

@@ -14,6 +14,9 @@
private Phone phone;
private Email email;
private Address address;

private int sequenceNumber;
public static int nextSequenceNumber = 1;

Choose a reason for hiding this comment

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

nextSequenceNumber should be private

@bongbongbee
Copy link

@howitzerg Some comments added. Please close the PR after reading comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants