best practicimando

[nx]

howdy fellers and felletes:

I recently made a pull request on a client project. They have an existing dev team that I work with.

The pull request was for a class and some component classes totaling several hundred LOC that generates a complex report. I really like making functions to break code up into bits:

def render

  rows = big_things.inject([]) { |rows, t| append_little_things(t, rows) }.flatten

  handle(rows)

end

def big_things

  BigThing.where(...).all

end

def append_little_things(thing, rows)

  group = ThingGroup.new(thing)

  rows.push(group.to_a)

end

The dev team said my code was too complex, difficult to follow and was contrary to the current pattern for writing report generation code. After changing it and resubmitting the pull request, this was preferred to them:

def render

  # Get the things

  big_things = BigThing.where(...).all

  # Track rows

  rows = []

  # Function for Building a row

  build_row = ->(x, y, z, e, m, c) { [....] }

  big_things.each do |t|

    # Build a row for each child, otherwise

    # render a single row

    unless t.children.empty?

      t.children.each do |c|

        rows << build_row.call(t.somethin, t.yep, t.mhm, c.x, c.y, c.z)

      end

    else

      rows << build_row.call(t.somethin, t.yep, t.mhm)

    end

  end

  handle(rows)

end

Was I wrong in thinking that way when I made the first PR? I'm all about code-hiding, breaking out logic into separate classes, bringing deeply nested statements out into 2-space indented methods and writing many functions with a small LOC /w descriptive names. Is that silly?

[Blake Hall]

I definitely prefer small methods. It makes it easier to debug and, in my opinion, read. I’ve always like Thoughtbot’s coding guidelines, specifically: https://github.com/thoughtbot/guides/tree/master/best-practices. I’ve kinda assumed that small methods were the most popular design style.

--
Blake Hall
@blakeshall

--
You received this message because you are subscribed to the Google Groups "Kentucky Ruby user group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kyrug+un...@googlegroups.com.
To post to this group, send email to ky...@googlegroups.com.
Visit this group at http://groups.google.com/group/kyrug.
For more options, visit https://groups.google.com/groups/opt_out.

[Chase Southard]

Not sure about code-hiding that sounds like a bad thing. But small methods, discrete classes, move shared logic into libs - that sorta stuff is good, IMHO. 

Need to continue chewing my way through: http://www.poodr.com/ book. Might be old hat to some of you, but it's good so far. Book club, anyone?

Also, good on ya for using the google group. More ruby. More Ruby. More RUBY.

Chase

--

You received this message because you are subscribed to the Google Groups "Kentucky Ruby user group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kyrug+un...@googlegroups.com.
To post to this group, send email to ky...@googlegroups.com.
Visit this group at http://groups.google.com/group/kyrug.
For more options, visit https://groups.google.com/groups/opt_out.

--

R. Chase Southard

@southard

http://southard.co

[nx]

Here's another example of breaking huge blocks of code into helper functions: http://www.gar1t.com/blog/solving-embarrassingly-obvious-problems-in-erlang.html. The language is Erlang, but it applies to any language.

I guess its really a culture thing and you just have to adapt your style to whatever tribe you work with.

[Todd Willey]

In general, team performance trumps everything else.

Specifically, you're not wrong, but there isn't much point in being
not-wrong if warps the minds of your reviewers. FWIW, I also prefer
the smaller, object oriented style to the procedural style. What's the
point in using ruby if you don't take advantage of what it offers and
just end up writing PERL?

I've shared our styleguide:
http://cirrusmio.github.io/learning-things/styleguide.html -- perhaps
your team should develop a similar set of standards to point to, so
contractors know the expectations?

On Tue, Oct 29, 2013 at 2:47 AM, nx <n...@nu-ex.com> wrote:

[timk]

Your longer single method just looks like logic that hasn't hit the "refactor" stage in TDD's "red, green, refactor".

It's much harder to read lots of tiny methods when you're not already familiar with the code. You need to accumulate a lot of state in your mind while you're studying new code, and tiny methods are basically GOTOs. Each time you see a new method call, you have to set a mental bookmark, engage your knowledge of scoping rules to go find the method, analyze its signature, trace its operation, and determine its return value. If the code uses implicit returns like your small-methods example, you may also need to recall less-used knowledge of mutator return values. Then you need to cache all that logic in your brain and substitute it wherever you see the method call.

That said, I'm with you on pulling out and naming complex logic so that top-level methods have clear logic that doesn't get caught up in implementation details. And once the reader gets past the head-banging frustration of accumulating enough mental state to trace execution in wetware, small methods can make the code really expressive. Like learning a new library API. It may also decrease coupling and open up opportunities for dependency injection, too.

-Tim

[nx]

I agree. I'd rather be flexible than fight for my coding dogma.

Proposing they make a style guide is a good idea. I'll do that.

[Chuck Fouts]

Smaller methods are preferred for one reason which is readability. Also, in my experience, large methods are prone to growing larger because they are harder to understand and it is easier to just add one more if statement.

That said what the others said about fitting in with the current team culture is correct. Even though I think it is pretty obvious that smaller methods are better other people may not have reached that conclusion yet. :)