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

Code cleanup in utility classes #1233

Conversation

@findepi
Copy link
Member

commented Aug 1, 2019

No description provided.

@cla-bot cla-bot bot added the cla-signed label Aug 1, 2019

@dain

dain approved these changes Aug 1, 2019

Copy link
Member

left a comment

One comment

{
private static final Logger LOG = Logger.get(S3SelectPushdown.class);
private S3SelectPushdown() {}

This comment has been minimized.

Copy link
@dain

dain Aug 1, 2019

Member

Normally the constructor is placed after the constants.

This comment has been minimized.

Copy link
@findepi

findepi Aug 1, 2019

Author Member

Indeed.... although it's weird to me. This constructor is just utility class marker, so having is somewhere in the middle is not useful.

This comment has been minimized.

Copy link
@dain

dain Aug 1, 2019

Member

Understood, but normally we prefer consistency in cases like this.

@findepi findepi force-pushed the findepi:findepi/master/code-cleanup-in-s3selectpushdown-65869d branch from 8db9677 to 8aecd45 Aug 1, 2019

@findepi findepi changed the title Code cleanup in S3SelectPushdown Code cleanup in utility classes Aug 1, 2019

@findepi findepi requested a review from dain Aug 1, 2019

@dain
Copy link
Member

left a comment

Couple of minor comments. This seems file to me other than a final decision on where to place the private constructor.

findepi added some commits Aug 2, 2019

@findepi findepi force-pushed the findepi:findepi/master/code-cleanup-in-s3selectpushdown-65869d branch from 8aecd45 to c5fe10f Aug 2, 2019

@findepi

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

This seems file to me other than a final decision on where to place the private constructor.

I moved it to where it was for now, but i think in utility classes it could be at the top.
My reasoning is -- the only role of this constructor is "to be" and the only role is "to convey it's utility class".
When it is in between, it doesn't play it's role.

@electrum electrum merged commit ab0382c into prestosql:master Aug 16, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
verification/cla-signed
Details

@findepi findepi deleted the findepi:findepi/master/code-cleanup-in-s3selectpushdown-65869d branch Aug 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.