dolfin team mailing list archive

[Anders Logg <logg@xxxxxxxxx>]

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

--
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