On Wed, 06 Oct 2010 10:32:45 -0700, Markus Roberts wrote: > On Mon, Oct 4, 2010 at 10:24 AM, Paul Berry <[email protected]> wrote: > > Since this patch has been sitting on the list with no comments for a while, > > Jesse and I took a stab at reviewing it last week. Comments below: > > > > On Wed, Sep 8, 2010 at 11:53 AM, Paul Berry <[email protected]> wrote: > >> @@ -381,6 +314,14 @@ class Puppet::SimpleGraph > >> predecessor > >> end > >> > >> + def downstream_from_vertex(v) > >> + �...@downstream_from[v] || > >> @out_from[v].keys.inject(@downstream_from[v] = > >> {}) { |result,node| result.update(node=>1).update > >> downstream_from_vertex(node) } > >> + end > >> + > >> + def upstream_from_vertex(v) > >> + �...@upstream_from[v] || @in_to[v].keys.inject( @upstream_from[v] > >> = > >> {}) { |result,node| result.update(node=>1).update > >> upstream_from_vertex(node) > >> } > >> + end > >> + > > > > These two methods are very difficult to read. Is there a good reason not to > > do something like this? > > def downstream_from_vertex(v) > > return @downstream_from[v] if @downstream_from[v] > > result = @downstream_from[v] = {} > > �...@out_from.keys.each do |node| > > result[node] = 1 > > result.update(downstream_from_vertex(node)) > > end > > result > > end > > Because I find early returns & mucking about with temporary variables > like "result" much more difficult to read? The first one tells you > explicitly what the result is and the second just tells you that it's > what you get if you follow some procedure. Also, the second form is > longer and, I suspect, slower. > > Taking the idioms individually: > > def foo > @foo || (exp) > end > > is idiomatic ruby; the alternative: > > def foo > return @foo if @foo > result = @foo = (exp) > result > end > > doesn't look like ruby at all, and has way too many moving parts. > > As for using inject to build a hash in a manner analogous to the way > collect builds an array, it isn't a common but it's still (IMHO) > better than the procedural version. I'd be thrilled if ruby had hash > analogs for collect, flatten, etc. but I don't think using inject is > that much less readable than using map instead of collect for arrays. > > The present version (with "node" changed to "x" to avoid a grammatical issue): > > def downstream_from_vertex(v) > @downstream_from[v] || > @out_from[v].keys.inject(@downstream_from[v] = {}) { |result,x| > result.update(x=>1).update downstream_from_vertex(x) } > end > > can be read off in English as: the nodes downstream from vertex v > (memoized) is a hash containing x and the nodes downstream from x for > each node x on an edge out from v. >
While I agree that early returns, and temp variables _can_ be harder to read, I think they have the opposite effect in this case. The original lines are long enough and complex enough that I think they deserve to be broken up, and I think the usage of early returns, and a temp variable is a perfectly valid way to do so (though you should still be careful to not go too far in that direction). I'm +1 for Paul's suggestion here. -- Jacob Helwig
signature.asc
Description: Digital signature
