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, 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.
>
>
> 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@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
>
>
> _______________________________________________
> 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
