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

Komax codereview #847

Closed
wants to merge 17 commits into from

Conversation

Projects
None yet
2 participants
@komax
Copy link
Member

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

Maximilian Konzack added some commits Jun 27, 2013

Maximilian Konzack
removal of not required imports; add of @override annotation for thes…
…e files;

comparing Strings by equals not on == in Java
Maximilian Konzack
refactored ASTCompiler checking for dropping last value on stack (new…
… help

method discardTopValue()); Addition of some @override
Maximilian Konzack
addition of many @override. Refactoring on some conditions using
String.equals(). Moved checks out of if conditions to extra variable, if it
used at least twice
Maximilian Konzack
Merge branch 'master' into komax_codereview
upstream from jruby master
@BanzaiMan

This comment has been minimized.

Copy link
Member

BanzaiMan commented Jul 2, 2013

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

BanzaiMan commented Jul 2, 2013

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
You can’t perform that action at this time.