dolfin team mailing list archive

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

On 09/03/11 07:08, Marie E. Rognes wrote:
> On 03/09/2011 08:04 AM, Garth N. Wells wrote:
>>
>>
>> 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.
> 
> 
> Now, it wasn't changed to that, and that is the reason why I was
> puzzled. 

That's because I didn't see at first that the top part of the function
had a return statement, which is why for this example I prefer to use
just one conditional block.

Garth

> The above version makes sense, although I prefer the other style.
> 
> Summarizing, I now know what people's views and motivations are, and I
> think I'll agree to disagree with regard to style. Let's leave it at
> that :-)
> 
> -- 
> Marie
> 
> 
> 
>>
>> 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
>>
>> _______________________________________________
>> 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