1031565 - Don't use m prefix for member variables

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[Eric Edens [:eedens]]

Moved from https://github.com/ericedens/FirefoxSearch/issues/8

Comment 1

[Eric Edens [:eedens]]

Attachment: bug-1031565-fix.patch

The patch is from https://github.com/ericedens/FirefoxSearch/pull/13

Comment 2

[:Margaret Leibovic]

Comment on attachment 8447462 [details] [diff] [review]
bug-1031565-fix.patch

Review of attachment 8447462 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good, I just have some comments suggesting additional improvements. Yay patches!

::: app/src/main/java/org/mozilla/search/autocomplete/AutoCompleteAgentManager.java
@@ +22,5 @@
>   */
>  class AutoCompleteAgentManager {
>  
> +    private final Handler mainUiHandler;
> +    private final Handler handler;

This member is confusingly named, since there's another Handler member in here. Can we give it a more specific name? (I know this is scope creep for this bug, but it would just be one tiny change :)

::: app/src/main/java/org/mozilla/search/autocomplete/AutoCompleteFragment.java
@@ +130,5 @@
>      @Override
>      public void onDestroyView() {
>          super.onDestroyView();
> +        if (null != inputMethodManager)
> +            inputMethodManager = null;

Nit: Brace single-line if statements (also not directly in the description of the bug, but this would be easy enough to do here).

Actually, why do we even need some of these if statements? We could just always set the variable to null, and if it was already null, so what?

@@ +197,1 @@
>              return;

More single-line if statements. Just brace them all! :)

::: app/src/main/java/org/mozilla/search/autocomplete/AutoCompleteRowView.java
@@ +40,5 @@
>  
>          findViewById(R.id.auto_complete_row_jump_button).setOnClickListener(new OnClickListener() {
>              @Override
>              public void onClick(View v) {
> +                if (null == onJumpListener) {

Nit: I noticed this in other places as well - normally we do |if (foobar == null)| instead of the other way around like you've done here. It doesn't really matter much, but I find it easier to read if we just stay consistent.

Comment 3

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic (Away June 28 - July 13) from comment #2)

> > +    private final Handler mainUiHandler;
> > +    private final Handler handler;
> 
> This member is confusingly named, since there's another Handler member in
> here. Can we give it a more specific name? (I know this is scope creep for
> this bug, but it would just be one tiny change :)

Also I have a general preference for inline acronyms staying capitalized: mainUIHandler, externalURL, etc.

We don't have a consistent style for this in Fennec, so consider this comment my attempt to make one and have it agree with my personal preferences ;)


> Nit: I noticed this in other places as well - normally we do |if (foobar ==
> null)| instead of the other way around like you've done here. It doesn't
> really matter much, but I find it easier to read if we just stay consistent.

I'm sure you know this, margaret, but for Bugzilla posterity: the reason to do that is that this:

  if (foobar = null) {

is a hidden bug in lots of C-like languages, but

  if (null = foobar) {

is a compile error. Most compilers warn about the former case, requiring you to type

  if ((foobar = null)) {

if you really want the assignment-as-truth behavior, so the need for this is somewhat diminished, but it's still a reasonable style.

Comment 4

[Eric Edens [:eedens]]

https://github.com/ericedens/FirefoxSearch/pull/13