You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@ranger.apache.org by Zsombor <gz...@gmail.com> on 2017/02/03 00:37:47 UTC

Code-style

Hi,

 After reviewing the code base, I have a couple of issues, which I would
like to ask:
* There are lot's of places, where the fields could be marked as private
and/or final, but it's omitted, why ? Is it a lack of time, or interest ?
Or the project prefer this style?
* Why the xml configuration files are used for spring? Using java is much
more safe
* There are lot of static variable used, instead of relying on Spring (or
even Guice) to inject the necessary dependencies. Or there is a
RangerDaoManagerBase, which is just for looking up DAO classes. Why ? Is it
because the springframework is too old for proper generic service injection
support? Why it can't be upgraded from 3.1.3 to a current - 4.3.x ?
* Lot's of places, where there are empty blocks, even empty methods - or
exception "handling" as 'e.printStackTrace()'
* And smaller issues, like initializing fields unnecessarily, or calling
x.equalsIgnoreCase("something") instead of "something".equalsIgnoreCase(x)

What do you think, is it worth working on these?

Best regards,
 Zsombor

Re: Code-style

Posted by Don Bosco Durai <bo...@apache.org>.

Zsombor

Thanks for going through the code. I don’t think we have any strong reasons for doing things one way or another. It is generally we are so caught up with our day to day to work, that you don't want to change anything that is working and risk breaking something else.

We have constantly refactoring code to improve the performance or add features. Generally, it is a good idea to review newer commits for these issues. We can maintain a to-do or not-to-do list as a guideline for reviewers.

For existing issues, it would good to create JIRAs to track them. If the issues are trivial, then it would be a good place for new members to contribute.

Thanks

Bosco

On 2/3/17, 7:41 AM, "Velmurugan Periasamy" <vperiasamy@hortonworks.com on behalf of vel@apache.org> wrote:

    +1
    
    My suggestion is to open separate JIRAs for addressing each of these items.
    Although they don¹t affect functionality these can be worked on as coding
    best practices.
    
    From:  Colm O hEigeartaigh <co...@apache.org>
    Reply-To:  "dev@ranger.apache.org" <de...@ranger.apache.org>,
    "coheigea@apache.org" <co...@apache.org>
    Date:  Friday, February 3, 2017 at 6:37 AM
    To:  "dev@ranger.apache.org" <de...@ranger.apache.org>, "gzsombor@gmail.com"
    <gz...@gmail.com>
    Subject:  Re: Code-style
    
    Hi,
    
    Contributions to the project, even minor ones, are always welcome. I've
    cherry-picked some of your comments below that you could work on patches
    for, some of the other suggestions are a bit more contraversial and could
    be considered later.
    
    
    On Fri, Feb 3, 2017 at 12:37 AM, Zsombor <gz...@gmail.com> wrote:
    
    > 
    >  * There are lot's of places, where the fields could be marked as private
    >  and/or final, but it's omitted, why ? Is it a lack of time, or interest ?
    > 
    
    No reason, please feel free to submit a patch.
    
    * Lot's of places, where there are empty blocks, even empty methods - or
    >  exception "handling" as 'e.printStackTrace()'
    > 
    
    Yes agreed this needs to be improved.
    
    
    >  * And smaller issues, like initializing fields unnecessarily, or calling
    >  x.equalsIgnoreCase("something") instead of "something".equalsIgnoreCase(x)
    > 
    > 
    Again, please submit a patch for this.
    
    Colm.
    
    
    >  What do you think, is it worth working on these?
    > 
    >  Best regards,
    >   Zsombor
    > 
    
    
    
    -- 
    Colm O hEigeartaigh
    
    Talend Community Coder
    http://coders.talend.com

Re: Code-style

Posted by Velmurugan Periasamy <ve...@apache.org>.

+1

My suggestion is to open separate JIRAs for addressing each of these items.
Although they don¹t affect functionality these can be worked on as coding
best practices.

From:  Colm O hEigeartaigh <co...@apache.org>
Reply-To:  "dev@ranger.apache.org" <de...@ranger.apache.org>,
"coheigea@apache.org" <co...@apache.org>
Date:  Friday, February 3, 2017 at 6:37 AM
To:  "dev@ranger.apache.org" <de...@ranger.apache.org>, "gzsombor@gmail.com"
<gz...@gmail.com>
Subject:  Re: Code-style

Hi,

Contributions to the project, even minor ones, are always welcome. I've
cherry-picked some of your comments below that you could work on patches
for, some of the other suggestions are a bit more contraversial and could
be considered later.

On Fri, Feb 3, 2017 at 12:37 AM, Zsombor <gz...@gmail.com> wrote:

> 
>  * There are lot's of places, where the fields could be marked as private
>  and/or final, but it's omitted, why ? Is it a lack of time, or interest ?
> 

No reason, please feel free to submit a patch.

* Lot's of places, where there are empty blocks, even empty methods - or
>  exception "handling" as 'e.printStackTrace()'
> 

Yes agreed this needs to be improved.

>  * And smaller issues, like initializing fields unnecessarily, or calling
>  x.equalsIgnoreCase("something") instead of "something".equalsIgnoreCase(x)
> 
> 
Again, please submit a patch for this.

Colm.

>  What do you think, is it worth working on these?
> 
>  Best regards,
>   Zsombor
> 

-- 
Colm O hEigeartaigh

Talend Community Coder
http://coders.talend.com

Re: Code-style

Posted by Colm O hEigeartaigh <co...@apache.org>.

Hi,

Contributions to the project, even minor ones, are always welcome. I've
cherry-picked some of your comments below that you could work on patches
for, some of the other suggestions are a bit more contraversial and could
be considered later.

On Fri, Feb 3, 2017 at 12:37 AM, Zsombor <gz...@gmail.com> wrote:

>
> * There are lot's of places, where the fields could be marked as private
> and/or final, but it's omitted, why ? Is it a lack of time, or interest ?
>

No reason, please feel free to submit a patch.

* Lot's of places, where there are empty blocks, even empty methods - or
> exception "handling" as 'e.printStackTrace()'
>

Yes agreed this needs to be improved.

> * And smaller issues, like initializing fields unnecessarily, or calling
> x.equalsIgnoreCase("something") instead of "something".equalsIgnoreCase(x)
>
>
Again, please submit a patch for this.

Colm.

> What do you think, is it worth working on these?
>
> Best regards,
>  Zsombor
>

-- 
Colm O hEigeartaigh

Talend Community Coder
http://coders.talend.com