Some Small things

[Sandro Martini <sa...@gmail.com>]

Hi to all,
i think to have just finished to add missing @Override in pivot
sources (i hope :-) ).

I've seen some small things:
- in TerraTreeViewSkin, there are a warning on two empty for, in
addVisibleNode() and in removeVisibleNodes() ... as seen before for
other warnings on while empty I think should be modified, for better
readability, and to avoid warnings ...
- in TerraTableViewHeaderSkin, i got this warning "Exporting
non-public type through public API"
    protected SortIndicatorImage sortAscendingImage = new
SortIndicatorImage(SortDirection.ASCENDING);
    protected SortIndicatorImage sortDescendingImage = new
SortIndicatorImage(SortDirection.DESCENDING);
  changing them to private solve ... i make the change, right ?

- there are many warnings on "Local variable hides a field" (in many
sources), and i know the the code works good also with this, but what
do you think on changing a little these variables, to avoid this ?
Using Best practices always help ...

Tell me.

Sandro

Re: Some Small things

[Greg Brown <gk...@mac.com>]

>> - there are many warnings on "Local variable hides a field" (in many
>> sources), and i know the the code works good also with this, but what
>> do you think on changing a little these variables, to avoid this ?
>> Using Best practices always help ...
>>
>
> I actually think it's ok to have local variables hide a field.   
> Otherwise,
> you end up naming your local variables with contrived names, and in  
> most (if
> not all) cases where we do this, our local variable is a view (or  
> decorator,
> in design pattern terminology) of our field, so the naming parity is
> intentional.

Yeah. That *is* our recommended "best practice".  :-)

Re: Some Small things

[Todd Volkert <tv...@gmail.com>]

> > No, the fields are protected, and the class is private, which is what is
> > generating the warning.  If the class were protected as well as the
> fields,
> > all would be well :)
> Moving the fields to private solve the warning, without changing the class,
> Ok ?
> Or do you think it's better to change the class to protected ?
>

Either way, but you're probably right -- let's make the fields private since
subclasses shouldn't need to paint the sort indicator images.

Re: Some Small things

[Sandro Martini <sa...@gmail.com>]

Hi Todd,

> Yep.  You want me to make the change or you?
I can do it ...

> No, the fields are protected, and the class is private, which is what is
> generating the warning.  If the class were protected as well as the fields,
> all would be well :)
Moving the fields to private solve the warning, without changing the class, Ok ?
Or do you think it's better to change the class to protected ?

Bye

Re: Some Small things

[Todd Volkert <tv...@gmail.com>]

> > In those cases, there are comments right before the loops explaining
> what's
> > going on, and they're just scanning loops (used to position an int
> > variable).  Can you suggest a different phrasing so we can see the
> options?
> Very simple ... instead of the ";", use an empty { } ... ok, minimal
> but at least the warnings are gone.
> Ok ?
>

Yep.  You want me to make the change or you?


>
> > I'd make the SortIndicatorImage class protected instead.
> private (they are already protected) ...
>

No, the fields are protected, and the class is private, which is what is
generating the warning.  If the class were protected as well as the fields,
all would be well :)

Re: Some Small things

[Sandro Martini <sa...@gmail.com>]

> Thanks!  Can you resolve the JIRA ticket?
Ok, fixed.


> In those cases, there are comments right before the loops explaining what's
> going on, and they're just scanning loops (used to position an int
> variable).  Can you suggest a different phrasing so we can see the options?
Very simple ... instead of the ";", use an empty { } ... ok, minimal
but at least the warnings are gone.
Ok ?


> I'd make the SortIndicatorImage class protected instead.
private (they are already protected) ...


> I actually think it's ok to have local variables hide a field.  Otherwise,
> you end up naming your local variables with contrived names, and in most (if
> not all) cases where we do this, our local variable is a view (or decorator,
> in design pattern terminology) of our field, so the naming parity is
> intentional.
Ok, but in this case we couldn't have the support of compiler and
tools, to check that the code inside is right ...
Usually I use a different variable name, maybe with only a little
difference in name, and but for me it's Ok the same.

Bye

Re: Some Small things

[Todd Volkert <tv...@gmail.com>]

> i think to have just finished to add missing @Override in pivot
> sources (i hope :-) ).
>

Thanks!  Can you resolve the JIRA ticket?


> I've seen some small things:
> - in TerraTreeViewSkin, there are a warning on two empty for, in
> addVisibleNode() and in removeVisibleNodes() ... as seen before for
> other warnings on while empty I think should be modified, for better
> readability, and to avoid warnings ...
>

In those cases, there are comments right before the loops explaining what's
going on, and they're just scanning loops (used to position an int
variable).  Can you suggest a different phrasing so we can see the options?


> - in TerraTableViewHeaderSkin, i got this warning "Exporting
> non-public type through public API"
>    protected SortIndicatorImage sortAscendingImage = new
> SortIndicatorImage(SortDirection.ASCENDING);
>    protected SortIndicatorImage sortDescendingImage = new
> SortIndicatorImage(SortDirection.DESCENDING);
>  changing them to private solve ... i make the change, right ?
>

I'd make the SortIndicatorImage class protected instead.


> - there are many warnings on "Local variable hides a field" (in many
> sources), and i know the the code works good also with this, but what
> do you think on changing a little these variables, to avoid this ?
> Using Best practices always help ...
>

I actually think it's ok to have local variables hide a field.  Otherwise,
you end up naming your local variables with contrived names, and in most (if
not all) cases where we do this, our local variable is a view (or decorator,
in design pattern terminology) of our field, so the naming parity is
intentional.

-T