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

Feedback for the collated code of A0003878Y #175

Closed
cmkumar87 opened this issue Oct 30, 2016 · 3 comments
Closed

Feedback for the collated code of A0003878Y #175

cmkumar87 opened this issue Oct 30, 2016 · 3 comments
Assignees

Comments

@cmkumar87
Copy link

cmkumar87 commented Oct 30, 2016

  • Looks like you have relatively fewer lines of code than your teammates (189 lines plus 88 test code lines)
  • Nesting looks deep. need refactoring? Is it doing more what it should?
    • private Command prepareAdd(String args) { ...

      while (matcher.find()) {
  • Same comment for this method too: private Command prepareSchedule(String args) {
  • The fact that the two methods looks similar is more of an indication that common code can be refactored.
  • Doesn't look like test case methods are following the convention taught in the earlier tutorial.
@burnflare burnflare self-assigned this Oct 30, 2016
burnflare added a commit that referenced this issue Nov 1, 2016
@burnflare
Copy link

burnflare commented Nov 1, 2016

  • Not all my code was tagged by last week, some files were missing. This will be fixed by tomorrow's collate submission.
  • Thanks for the note, I agree, will refactor.
  • ^Same as above^
  • I agree
  • @cmkumar87 Do you mind pointing out which convention again?

@cmkumar87
Copy link
Author

@burnflare conventions like in this PR: https://github.com/nus-cs2103-AY1617S1/addressbook-level2/pull/1163/files
I am unable to find the reference in the handbook

@burnflare
Copy link

Thank you!

Sent from my iPhone

On 2 Nov 2016, at 1:32 AM, Muthu Kumar Chandrasekaran [email protected] wrote:

@burnflare conventions like in this PR: https://github.com/nus-cs2103-AY1617S1/addressbook-level2/pull/1163/files
I am unable to find the reference in the handbook


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

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

No branches or pull requests

2 participants