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

[T2A2][W15-A3]Sriram Sami #90

Closed
wants to merge 2 commits into from

Conversation

frizensami
Copy link

No description provided.

@frizensami
Copy link
Author

Ready for review.

@frizensami frizensami changed the title [T2A2][T06-A3]Sriram Sami [T2A2][W15-A3]]Sriram Sami Aug 23, 2016
@frizensami frizensami changed the title [T2A2][W15-A3]]Sriram Sami [T2A2][W15-A3]Sriram Sam Aug 23, 2016
@frizensami frizensami changed the title [T2A2][W15-A3]Sriram Sam [T2A2][W15-A3]Sriram Sami Aug 23, 2016
@@ -365,7 +365,8 @@ public static String executeCommand(String userInputString) {
*/
private static String[] splitCommandWordAndArgs(String rawUserInput) {
final String[] split = rawUserInput.trim().split("\\s+", 2);
Copy link

@okkhoy okkhoy Aug 25, 2016

Choose a reason for hiding this comment

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

split is not a particularly good variable name; you could have changed it to be more reflective of what it is
in any case, its an array, so by the standards, it should have been splits :-P

@okkhoy
Copy link

okkhoy commented Aug 25, 2016

@frizensami

  • some good refactoring done here; couple of good catches;
  • i believe you can do even better than this attempt.
  • in general, pay more attention to coding standards violation.
  • some useful ones include checking how conditional statements are written, naming conventions etc.,

@okkhoy okkhoy closed this Aug 25, 2016
@frizensami
Copy link
Author

frizensami commented Sep 8, 2016

Thanks for the comments! Much appreciated. Apologies - I just saw this.

@okkhoy
Copy link

okkhoy commented Sep 9, 2016

welcome! your ACK came in late ;-)

On Thu, Sep 8, 2016 at 10:43 PM, Sriram Sami [email protected]
wrote:

Thanks for the comments! Much appreciated.


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#90 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA-eBgtS21cQpDkQMrd90dQRLUGpUGSfks5qoB72gaJpZM4Jq2rx
.

@okkhoy
Copy link

okkhoy commented Sep 9, 2016

please close the PR if you haven't done yet.

On Thu, Sep 8, 2016 at 10:43 PM, Sriram Sami [email protected]
wrote:

Thanks for the comments! Much appreciated.


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#90 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA-eBgtS21cQpDkQMrd90dQRLUGpUGSfks5qoB72gaJpZM4Jq2rx
.

@frizensami
Copy link
Author

Yep I think it's closed already

yamgent pushed a commit to yamgent/addressbook-level1 that referenced this pull request Aug 5, 2018
…-AY1617S1#90)

The default storage file 'addressbook.txt' is not provided by default.
The program has to create a new storage file, and this process is 
written in the program's log.

Due to the additional information in the log, the first test run after 
cloning the master always result in failure, as the additional
information creates a difference between actual.txt and expected.txt,
causing the test to fail.

Let's add the default storage file 'addressbook.txt' into the test
folder so that the runtests.bat will stop failing on first run.
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.

2 participants