dolfin team mailing list archive

[Johan Hake <johan.hake@xxxxxxxxx>]

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