Re: [PATCH 2/2] staging: rtl8192u: Corrects 'Avoid CamelCase' for variables

[Dan Carpenter <dan.carpenter@oracle.com>]

On Tue, Mar 17, 2020 at 08:51:30AM +0000, Camylla Goncalves Cantanheide wrote:
> The variables of function setKey triggered a 'Avoid CamelCase'
> warning from checkpatch.pl. This patch renames these
> variables to correct this warning.
> 
> Signed-off-by: Camylla Goncalves Cantanheide <c.cantanheide@gmail.com>
> ---
>  drivers/staging/rtl8192u/r8192U_core.c | 52 +++++++++++++-------------
>  1 file changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
> index 93a15d57e..fcfb9024a 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -4877,50 +4877,50 @@ void EnableHWSecurityConfig8192(struct net_device *dev)
>  	write_nic_byte(dev, SECR,  SECR_value);
>  }
>  
> -void setKey(struct net_device *dev, u8 EntryNo, u8 KeyIndex, u16 KeyType,
> -	    u8 *MacAddr, u8 DefaultKey, u32 *KeyContent)
> +void setKey(struct net_device *dev, u8 entryno, u8 keyindex, u16 keytype,
> +	    u8 *macaddr, u8 defaultkey, u32 *keycontent)
>  {
> -	u32 TargetCommand = 0;
> -	u32 TargetContent = 0;
> -	u16 usConfig = 0;
> +	u32 target_command = 0;
> +	u32 target_content = 0;
> +	u16 us_config = 0;

Use these renames to think deeply about naming.

I don't like "entryno".  I would prefer "entry_no".  Use the same
underscore for spaces rule for key_index, mac_addr and all the rest.  Is
"key_idx" better or "key_index"?

What added value or meaning does the "target_" part of "target_command"
add?  Use "cmd" instead of "command".  "target_command" and
"target_content" are the same length and mostly the same letters.  Avoid
that sort of thing because it makes it hard to read at a glance.  The
two get swapped in your head.

What does the "us_" mean in us_config?  Is it microsecond as in usec?
Is it United states?  Actually it turns out it probably means "unsigned
short".  Never make the variable names show the type.  If you have a
good editor you can just hover the mouse over a variable to see the
type.  Or if you're using vim like me, then you have to use '*' to
highlight the variable and scroll to the top of the function.  Either
way, never use "us_" to mean unsigned short.

What does the "config" part of "us_config" mean?  What does the "content"
part of "target_content" mean?  Always think about that.  Variable names
are hard and maybe "config" and "content" are clear enough.  But at
think about it, and consider all the options.

Anyway, the reason that this patch needs to be re-written is because
we want underscores in place of spaces for "key_type" and because
"us_config" is against the rules.  The rest is just something to
consider and if you find better names, then go with that but if you
don't just fix those two things and resend.

regards,
dan carpenter