dolfin team mailing list archive

["Garth N. Wells" <gnw20@xxxxxxxxx>]

On 08/03/11 23:17, Johan Hake wrote:
> On Tuesday March 8 2011 14:47:00 Anders Logg wrote:
>> On Mon, Mar 07, 2011 at 07:46:54AM +0000, Garth N. Wells wrote:
>>> On 07/03/11 07:40, Marie E. Rognes wrote:
>>>> On 03/07/2011 08:37 AM, Garth N. Wells wrote:
>>>>> On 06/03/11 22:23, Marie E. Rognes wrote:
>>>>>> On 03/06/2011 10:41 PM, Garth N. Wells wrote:
>>>>>>> On 06/03/11 21:29, Marie E. Rognes wrote:
>>>>>>>> On 03/06/2011 10:22 PM, Anders Logg wrote:
>>>>>>>>> On Sun, Mar 06, 2011 at 09:08:10PM +0000, Garth N. Wells wrote:
>>>>>>>>>> On 06/03/11 21:02, Marie E. Rognes wrote:
>>>>>>>>>>> On 03/06/2011 09:30 PM, noreply@xxxxxxxxxxxxx wrote:
>>>>>>>>>>>>        if (parameters["max_dimension"].change_count()>     0
>>>>>>>>>>>>        
>>>>>>>>>>>>            &&     V.dim()>     max_dimension)
>>>>>>>>>>>>
>>>>>>>>>>>> +  {
>>>>>>>>>>>>
>>>>>>>>>>>>          return true;
>>>>>>>>>>>>
>>>>>>>>>>>> -
>>>>>>>>>>>> -  // Otherwise, not done.
>>>>>>>>>>>> -  return false;
>>>>>>>>>>>> +  }
>>>>>>>>>>>> +  else
>>>>>>>>>>>> +    return false;
>>>>>>>>>>>>
>>>>>>>>>>>>      }
>>>>>>>>>>>
>>>>>>>>>>> I notice that my early returns keep getting moved into else
>>>>>>>>>>> clauses... I
>>>>>>>>>>> find this approach less readable, especially when there are
>>>>>>>>>>> nested ifs.
>>>>>>>>>>> Why is it the preferred way?
>>>>>>>>>>
>>>>>>>>>> Because your comment basically says else, so I'd say it's better
>>>>>>>>>> to have
>>>>>>>>>> the code say it consistently.
>>>>>>>>>>
>>>>>>>>>> I find it easier to follow, because it's clear that the function
>>>>>>>>>> exits
>>>>>>>>>> from the conditional block. The return value is either true or
>>>>>>>>>> false depending on the one true/false evaluation.
>>>>>>>>
>>>>>>>> The code is an if -- else if -- else. I don't see how moving that
>>>>>>>> into an if, if -- else increases consistency.
>>>>>>>
>>>>>>> It was an if -- else.
>>>>>>
>>>>>> No, it was not. (It was an "done if A", "done if B", otherwise "not
>>>>>> done")
>>>>>
>>>>> Looks to me like an if - else structure. It was
>>>>>
>>>>>    if (parameters["max_dimension"].change_count()>  0
>>>>>    
>>>>>        &&  V.dim()>  max_dimension)
>>>>>      
>>>>>      return true;
>>>>>    
>>>>>    // Otherwise, not done.
>>>>>    return false;
>>>>
>>>> Only if you ignore the first if ;-) The original read as:
>>>>   // Done if error is less than tolerance
>>>>   if (std::abs(error_estimate) < tolerance)
>>>>   
>>>>     return true;
>>>>   
>>>>   // Or done if dimension is larger than max dimension (and that
>>>>   // parameter is set).
>>>>   const uint max_dimension = parameters["max_dimension"];
>>>>   if (parameters["max_dimension"].change_count() > 0
>>>>   
>>>>       && V.dim() > max_dimension)
>>>>     
>>>>     return true;
>>>>   
>>>>   // Otherwise, not done.
>>>>   return false;
>>>
>>> This for me is now an even better example :) of why we should use
>>>
>>>   if - else if - else
>>>
>>> for these simple cases (in which nothing is done between statements). It
>>> would have much clearer immediately how the return value is being
>>> computed.
>>
>> I still think the else is unecessary! :-P
> 
> For what it is worth I agree with Anders and Marie. I cannot find it right 
> now, but reducing the amount of indented code using return statements from a 
> function is considered good programing (by some guidlines at least) as it 
> makes the code easier to read. I always seek to flesh out the simples options 
> first and return the values for these from any function.
> 

I don't know what there is to agree with, because for the actual code in
question indentation is *not* an issue. I suspect that Marie and myself
are the only ones that have looked at the code in question. It is:

    // Done if error is less than tolerance
    if (std::abs(error_estimate) < tolerance)
      return true;

    // Or done if dimension is larger than max dimension (and that
    // parameter is set).
    const uint max_dimension = parameters["max_dimension"];
    if (parameters["max_dimension"].change_count() > 0
          && V.dim() > max_dimension)
    {
      return true;
    }

    // Otherwise, not done.
    return false;

My preference is change it to:

    if (std::abs(error_estimate) < tolerance)
      return true;
    else if (parameters["max_dimension"].change_count() > 0
        && V.dim() > max_dimension)
    {
      return true
    }
    else
      return false;

which I find clearer.

Related to an earlier post, while 'else' can be avoided in the above, I
don't think that it's 'silly' to use a language element that enhances
readability. If a language provides syntax that self-documents code, I
would prefer this over user code comments.

Garth

> I see the point of premature return, with all kindoff error that comes with. 
> But at the end of the day, I think readable and logical built code with small 
> and consice functions, are the solution to most of these troubles anyway. 
> 
> Johan
> 
>> --
>> Anders
>>
>>> Garth
>>>
>>>>> and I changed  it to
>>>>>
>>>>>    if (parameters["max_dimension"].change_count()>  0
>>>>>    
>>>>>        &&  V.dim()>  max_dimension)
>>>>>    
>>>>>    {
>>>>>    
>>>>>      return true;
>>>>>    
>>>>>    }
>>>>>    else
>>>>>    
>>>>>      return false;
>>>>>
>>>>> Garth
>>>>>
>>>>>> The example in question was pretty trivial, and its precise form not
>>>>>> a big deal. However, I think having a common policy would be
>>>>>> beneficial.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> Mailing list: https://launchpad.net/~dolfin
>>>>>> Post to     : dolfin@xxxxxxxxxxxxxxxxxxx
>>>>>> Unsubscribe : https://launchpad.net/~dolfin
>>>>>> More help   : https://help.launchpad.net/ListHelp
>>>>>
>>>>> _______________________________________________
>>>>> Mailing list: https://launchpad.net/~dolfin
>>>>> Post to     : dolfin@xxxxxxxxxxxxxxxxxxx
>>>>> Unsubscribe : https://launchpad.net/~dolfin
>>>>> More help   : https://help.launchpad.net/ListHelp
>>>>
>>>> _______________________________________________
>>>> Mailing list: https://launchpad.net/~dolfin
>>>> Post to     : dolfin@xxxxxxxxxxxxxxxxxxx
>>>> Unsubscribe : https://launchpad.net/~dolfin
>>>> More help   : https://help.launchpad.net/ListHelp
>>>
>>> _______________________________________________
>>> Mailing list: https://launchpad.net/~dolfin
>>> Post to     : dolfin@xxxxxxxxxxxxxxxxxxx
>>> Unsubscribe : https://launchpad.net/~dolfin
>>> More help   : https://help.launchpad.net/ListHelp
>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~dolfin
>> Post to     : dolfin@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~dolfin
>> More help   : https://help.launchpad.net/ListHelp