Re: [Outreachy][PATCH v3] Staging: vt6655: Refactor switch case statement.

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Mar 26, 2020 at 07:53:01PM -0700, Briana Oursler wrote:
> Pull some logic branches above switch case. Create variables to reduce
> code duplication and improve readability.
> 
> Signed-off-by: Briana Oursler <briana.oursler@gmail.com>
> ---
> 
> Changes in v3:
> 	- Update grammar and meaning in patch description.
> 
> Changes in v2:
> 	- Remove ternary logic in switch case.
> 	- Introduce 'flag' bool.
> 	- Move len definition above switch case.
> 
>  drivers/staging/vt6655/rxtx.c | 128 ++++++++++++++++++----------------
>  1 file changed, 66 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
> index 37fcc42ed000..5b45d66b7f39 100644
> --- a/drivers/staging/vt6655/rxtx.c
> +++ b/drivers/staging/vt6655/rxtx.c
> @@ -239,11 +239,21 @@ s_uGetDataDuration(
>  )
>  {
>  	bool bLastFrag = false;
> +	bool flag = false;
>  	unsigned int uAckTime = 0, uNextPktTime = 0;
> +	unsigned int len;
>  
>  	if (uFragIdx == (uMACfragNum - 1))
>  		bLastFrag = true;
>  
> +	if (uFragIdx == (uMACfragNum - 2))
> +		len = cbLastFragmentSize;
> +	else
> +		len = cbFrameLength;
> +
> +	if (byFBOption == AUTO_FB_0)
> +		flag = true;

"flag" is horrible to try to remember what it really is for.

You already know this, because you say so in your code change:

> +			 /* flag true if (byFBOption == AUTO_FB_0) */
> +			if (flag) {

Why not just test for the == statement here?

Also, your comment is indented incorrectly, didn't checkpatch catch that
when you ran it against your patch?

> +			 /* flag true if (byFBOption == AUTO_FB_0) */
> +			if (flag) {

Same goes here as well.

I don't see why adding that new variable saved any space or logic, it
only made the code harder to follow and forced you to write the logic
out anyway, in the comment :)

thanks,

greg k-h