Skip to content

Komax codereview#847

Closed
komax wants to merge 17 commits intojruby:masterfrom
komax:komax_codereview
Closed

Komax codereview#847
komax wants to merge 17 commits intojruby:masterfrom
komax:komax_codereview

Conversation

@komax
Copy link
Member

@komax komax commented Jul 2, 2013

Finished my first code review:

  • checked classes for code style
  • simplified some boolean conditions
  • factorization of complex conditions into separate variables
  • addition of @OverRide
  • removed unnecessary imports
  • descriptive identifiers
  • refactoring of duplicated code patterns into helper private methods

komax added 17 commits June 27, 2013 09:56
…e files;

comparing Strings by equals not on == in Java
… help

method discardTopValue()); Addition of some @OverRide
String.equals(). Moved checks out of if conditions to extra variable, if it
used at least twice
@BanzaiMan
Copy link
Member

As I indicated on IRC, this PR is way too big, having too many disparate parts to be merged. Please separate them into smaller parts (one PR per one idea), and skip the stylistic changes (spaces, do/end vs. braces, @Override, etc.) for now.

Thanks.

@komax
Copy link
Member Author

komax commented Jul 2, 2013

Thanks for your feedback, I am new in contributing to open source projects. I split them into these pull requests

@BanzaiMan
Copy link
Member

Thanks. I'm going to close this one, then.

@BanzaiMan BanzaiMan closed this Jul 2, 2013
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.

2 participants