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, nore...@launchpad.net 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@lists.launchpad.net
>>>>>> Unsubscribe : https://launchpad.net/~dolfin
>>>>>> More help : https://help.launchpad.net/ListHelp
>>>>>
>>>>> _______________________________________________
>>>>> Mailing list: https://launchpad.net/~dolfin
>>>>> Post to : dolfin@lists.launchpad.net
>>>>> Unsubscribe : https://launchpad.net/~dolfin
>>>>> More help : https://help.launchpad.net/ListHelp
>>>>
>>>> _______________________________________________
>>>> Mailing list: https://launchpad.net/~dolfin
>>>> Post to : dolfin@lists.launchpad.net
>>>> Unsubscribe : https://launchpad.net/~dolfin
>>>> More help : https://help.launchpad.net/ListHelp
>>>
>>> _______________________________________________
>>> Mailing list: https://launchpad.net/~dolfin
>>> Post to : dolfin@lists.launchpad.net
>>> Unsubscribe : https://launchpad.net/~dolfin
>>> More help : https://help.launchpad.net/ListHelp
>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~dolfin
>> Post to : dolfin@lists.launchpad.net
>> Unsubscribe : https://launchpad.net/~dolfin
>> More help : https://help.launchpad.net/ListHelp
_______________________________________________
Mailing list: https://launchpad.net/~dolfin
Post to : dolfin@lists.launchpad.net
Unsubscribe : https://launchpad.net/~dolfin
More help : https://help.launchpad.net/ListHelp
