
How A Pull Request Rocked My World - 10char
http://clayallsopp.com/posts/the-story-of-pull-request/
======
Xcelerate
Reading the comments in this thread is very interesting to me, because each
programmer seems certain of the superiority of his own programming style.

We have some comments that disparage the change, saying that it obfuscates the
code and makes it more difficult to understand.

We have others saying that this change is a basic technique and it's
"shocking" that the author wrote a book without this simple knowledge.

What I take from this is yet another situation of people arguing about
something highly subjective that is closely tied to their identity. Good
programming is not an objective concept. People respond differently to
different programming techniques and what works better for one person may
greatly confuse another.

~~~
pjungwir
Early in my career, a project manager said to me, "I've never heard one
programmer praise another programmer's code. They always criticize!" I think
that reinforced for me both that someone's approach might be good even if it
isn't my own, and also that it's important to say when something is done well.
Of course, there's also just a lot of bad code out there.... :-)

~~~
cubicle67
I spent a few years as the sole maintainer of a moderately complex enterprise
app written prettly much entirely in PLSQL. Hundreds of thousands of lines of
it. By the time I left a reasonable proportion of that was code I'd added.

A few months after I'd left I got a phone call from the guy who'd taken over
the project - he rang specifically to compliment me on the code and how easy
it was for him to understand what was going on and how much better that had
made his work. Didn't really know how to respond as I've never had that
before, but it was nice.

Not blowing my own trumpet, so to speak (although it is something I'm kinda
proud of) but to provide a counter data point to your PM's

~~~
enjoy-your-stay
Although I've never gone to the effort of actually calling somebody up to
thank them, I have gone out of my way to thank colleagues whose code I've been
maintaining if I have found it easy to work with.

Code that is: consistent in style and approach (even if it's different to
mine) easy to to reason about has clear comments detailing why something was
done the way it was

is hard to do consistently, and very often stands out from the spaghetti code
that's present in other parts of the system.

The reason I do this is firstly to let the person know I appreciate the effort
they have gone to, and secondly to signal to the other developers on the
project that this is something worth trying to attain, and at least one person
values these things.

------
lincolnq
I don't love this refactor. I especially don't like how it generates names
from other names -- e.g., given 'switch'/'submit', appends '_row', and then
converts it to CamelCase so you get 'SwitchRow'/'SubmitRow', then instantiates
the class named that.

Here's why: When I am maintaining somebody else's code, even if I guess that
this is going on, I rarely trust these kinds of name-generation tricks. When I
need to change some code, I'm going to grep the codebase for 'SwitchRow' and
add a new line in whatever conditional to instantiate my new class. When grep
can't find any instances of SwitchRow's constructor being called, I become
suspicious and it costs me more time, because I am making sure I properly
understand what is actually going on.

~~~
rosser
_...I rarely trust these kinds of name-generation tricks._

So you're probably not a big fan of Rails, then?

~~~
potatolicious
I'm actually not a huge fan of implicit name-generation tricks, but I'm also
not entirely opposed to them.

The trick here I think is consistency - is name-generated type dispatch the
standard across the entirety of the codebase? If it is, go wild. If it's not
then I'd be much more wary of it.

Consistency and building correct expectations for future maintainers is pretty
important. Doing a smart trick in one place but failing to do it in other
places where it makes sense IMO tends to create a bigger mess than doing it
the dumb-but-safe way everywhere.

~~~
SoftwareMaven
I _so_ agree with you. There are few programming mistakes that consistency in
use does ameliorate. On the other hand, no matter how "good" the code, if it's
inconsistent, it's bad.

~~~
Arelius
I disagree. Consistency is only valuable so long as it helps clarity and
understanding. While that is most often the case, there are exceptions.

------
jules
Novice programmer: Uses simple if/else.

Adept programmer: Discovers polymorphism. It's great, uses it for everything.

Master programmer: Realizes that in many cases, it makes the code less clear
and more verbose, reverts back to if/else until the flexibility is actually
needed.

Same story repeats itself many times on different fronts: metaprogramming,
recursion, exceptions, macros, etc.

~~~
dougk16
One more step: Master programmer realizes that they're still a Novice
programmer - repeats the cycle.

------
rachelbythebay
I tried to picture the solution before scrolling down past the initial block
with the nested ifs. It got me thinking about "switch()", as seen in C, C++,
and elsewhere. That in turn got me thinking about using an enum to pick up on
whatever it is he's trying to do here, and jump to the right place
accordingly.

Generically, I had something like this in my head:

    
    
        switch (row_type) {
          case submittable: ...; break;
          case checkable: ...; break;
          ...
        }
    

Then I started realizing that using a switch means you have isolated things
such that they can only be one of the n types. It won't ever be submittable
AND checkable, for instance. I assume that's what is desired here: mutual
exclusivity for all of these possibilities.

Looking back at the original code, I see that by using the nested if/else
construct, it effectively gives priority to them in the order in which they
are encountered. A row with a submit_button is going to fire off
make_submit_cell() whether or not it has checkable set. It trumps the possible
call to make_check_cell, in other words.

These two approaches solve two different problems. Without knowing more, it's
hard to say whether the "priority" approach makes sense here.

If I had to guess, based on the refactoring which turned it into a series of
distinct classes, that sounds like it should have been "pick 1 of n" (one-hot
code) all along. If that's so, the if block wasn't just ugly, but it was
actually covering up some real problems. It was just asking for trouble in
terms of bugs down the road since it allowed nonsensical settings.

But that's just a guess. I could be wrong.

~~~
potatolicious
Indeed, the original code was, IMO, incorrect.

It looks like the purpose is a pretty common one in iOS - you have a table
view populated by many rows, each of which can be a different class that
renders and behaves differently. It's a common need in iOS apps.

What he really needed was an enum, not flags. The choice of class is mutually
exclusive and flags are horrible for tracking this, and are great for hiding
bugs - one erroneous flag flip and suddenly your code is walking down a
completely different path that's non-obvious on first debug. Using a bunch of
bools to approximate state is error-prone and IMO just bad architecture.

------
sbuccini
To be honest, I'm very disappointed with the comments here.

Clay is obviously a very talented programmer, and just like most talented
programmers, he didn't start out that way. This article isn't about how he
learned OOP, it's about how someone opened his eyes to how code can be
_elegant_ instead of simply just functional.

This article hits home with me especially. I just learned Python last semester
in my intro to CS class. I had an eerily similar moment when I learned how to
use generators. "WOW! You can simplify this huge block of code into one line!
It's..beautiful." It's a really special moment, and I'm glad Clay shared his.

------
10char
I was confused as to how a lot of folks took the post to mean I didn't know
what polymorphism was...and then I experienced the whole Arrested Development-
esque "I've made a huge mistake," probably a little too late.

A lot of folks drew attention to "You can reuse code with inheritance.
Absolutely crazy. Brilliant." and took as equivalent to me saying "It's
absolutely crazy and brilliant you can reuse code with inheritance." I get
that both in- and especially out-of-context that's how it comes off, but it's
not what I meant. It was sloppy and I should've been more careful in writing.

I meant it to come off more like "...everything is dynamic and decided at run-
time, plus you get a great plugin architecture if you do some polymorphic
tricks with these RowType objects. And all of these benefits came from just
one simple refactoring. The power of small refactors is absolutely crazy.
Brilliant." (I've added this as an addendum to the post)

Like the point of the whole thing wasn't about the refactor itself and if/how
_the code_ "rocked my world", but the fact that _a refactor_ could have a
_seismic effect_ on the future of a project and the direction it takes.

The addition of many more row "types" wasn't even in the orbit of my thinking
without the refactor, and it really helped shape where the project went: there
were just those 4 branches in the `if`/`else` tree in this post, but there are
now 20-odd default row configurations.

Hope that helps some folks to add more context

------
mquander
I don't agree with this change. Based on the code, it looks like there should
be an enumeration of cell_types somewhere, a button should have a cell_type,
and then there's a map from cell_type to construction_fn (or a polymorphic set
of classes to serve as the map.)

Metaprogramming is something you do when you can't do it easily in the usual
way. If a dispatch table or inheritance takes care of something, use the
existing language features instead of building your own.

(EDIT: On examination, there already is a RowType, and the pull request is a
lot closer to my suggestion than I first thought, so mostly ignore this.)

------
guylhem
On the first glance it seems great, but much more complex to understand and
debug.

The if tree was certainly not pretty, but straightforward - it did its thing.
It was something that could be given to an average developer to work on and
improve.

The refactored code will require someone that's not just an average developer
- if only because you need to read the article to understand it if you're not
familiar with javascript, while the initial code needed only a glance to
understand what it did.

IMHO, that's just complexity for complexity sake - unless you have plans to
take advantage of the new flexibility it provides.

(and if you have to find, hire and pay that 'better than average' person, it
might not be such a good thing.)

~~~
hnewser1
This is not written in javascript, it is ruby. And I think your developer is
way below average if he can't understand the pattern used in the refactored
code.

~~~
dubcanada
If you have a ruby programmer who can't understand this, fire them. And get a
new one. This type of stuff is all over ruby code.

------
leriksen
Those who know this technique either discover it themselves, through "reading,
learning, doing", or are shown,like this guy was. None of us was born knowing
this. Clay has blogged about his experience of discovering this - that in
itself is what makes this a great read for me. I wouldn't be surprised if
every single tech book author, at some point after their book has been
published, has learned something and thought "I wish I knew that when I wrote
my book !!" By their very nature books document the authors understanding at a
point in time. Would polymorphism have helped Clay's book ? Probably. Does not
knowing it ruin the book ? Probably not. If we waited 'til we knew everything
before we wrote a book about anything, nothing would ever get written.

~~~
AlexeyBrin
Writing a book is also a great way to discover new things about your subject.

~~~
nirvdrum
While true, when I purchase a book I'm trusting that the author is an expert
in the topic. Maybe my expectations are too high.

~~~
chris_wot
You may never know - if it teaches you some things and makes you more
productive, then what of it?

~~~
nirvdrum
My concern is the inverse of that: recommending something you either don't
fully understand or haven't used long enough to speak to either its pros or
cons. Being in a book usually is an indicator that something is a best
practice or how the way things should be done, ostensibly being put forth by
an expert. Perhaps we shouldn't hold such content to such a standard.

~~~
danneu
I checked out the self-published Object-Oriented Design Anthology that this
Clay guy ostensibly wrote judging by the discourse in this thread.

Instead, I found that he wrote from what I can tell the _only_ book that
introduces RubyMotion, published by PragProg, and endorsed by other authors.

His blog post shows humility, an ability to praise the work of others, and an
effort to add value to the world.

All things that these HN comments distinctly lack and, ultimately, punish.

~~~
nirvdrum
While I understand the point you're trying to make, OOP is a pervasive concept
that should factor into building Ruby and RubyMotion apps. My point is if he
opts not to use polymorphism in a RubyMotion app, it's an indication from him
as the author of a RubyMotion book that polymorphism should be avoided for
whatever reason (maybe the dynamic dispatch is too expensive). My assumption
isn't that the author simply doesn't understand OOP, but rather that this was
a deliberate decision. But, now a hole in his knowledge becomes best practice
for many, because as you pointed out, it's the only book on RubyMotion.

I appreciate the humility. I just wish it manifested in a crawl before you can
run mentality. There is actual value in understanding the fundamentals.

------
robbles
Can you imagine how much more pleasant participating in HN, GitHub, etc. would
be if all programmers were this pleasant and open-minded?

Regardless of the particular merits of the technique he talks about here, I
think we can all learn a lesson from this guy about appreciating the work and
ideas of others.

~~~
gruseom
I couldn't agree more. The ability to appreciate others' work is a sign of
intelligence.

------
stickydink
The article is very humble, but in my view, seems to go a little too far.

"You can reuse code with inheritance. Absolutely crazy. Brilliant.

All of this blew me away."

Honestly?

~~~
revelation
Thats a very critical point that you just have to understand, to "get". Many
people know and learn about object oriented design, but when tasked to
actually write a program based on these principles they will often revert to
code where they make distinctions based on the type of what they have been
handed.

TLDR: dynamic_cast is bad design

~~~
mylittlepony
I guess he's gonna come back with another article when he realizes composition
> inheritance.

~~~
alttab
You'll need to wait like 5 years if he was anything like me

------
chaz
Critiquing these particular lines of code or the pull request is unimportant.
My takeaway was that it's hugely valuable to simply make your code available
and have other people look at it. Not only can it improve your own code, but
you can personally learn something new from other people. You can be sure

Now I'm feeling a bit guilty for not having open sourced any of the code I've
ever written, and I'm wondering what kinds of lessons I could have learned
earlier on.

------
herge
The classic "Replace Conditional with Polymorphism" design pattern, which took
me way to long to realize that it also applies to Python.

~~~
jonathanberger
Yes, it's cataloged in Martin Fowler's excellent book Refactoring, Improving
the Design of Existing Code in Chapter 9.

I'd recommend the book strongly.

~~~
herge
Link for the lazy: <http://www.refactoring.com/catalog/>

------
wcoenen
This google tech talk is basically about the same thing, eliminating
conditionals with polymorphism:
[https://www.youtube.com/watch?feature=player_embedded&v=...](https://www.youtube.com/watch?feature=player_embedded&v=4F72VULWFvc)

------
jamieb
This person found something that they didn't know they didn't know and were so
happy they blogged about it. Regardless of what I may personally think about
the merits of the code before or after, it makes me happy that there is
another human being who is thrilled by learning something new about
programming.

------
rckrd
I would like to point out that he was able to describe his original
implementation in a few sentences. This is important.

------
laureny
tl;dr: polymorphism is usually better than if/else.

~~~
bfwi
I like this explanation: [http://stackoverflow.com/questions/234458/do-
polymorphism-or...](http://stackoverflow.com/questions/234458/do-polymorphism-
or-conditionals-promote-better-design)

------
jtchang
Refactors like this are super cool and often needed but I sometimes have to
think why we don't store the easier to read code as well.

I know we have git and we can always do a diff to see what changed. What I am
talking about is a more holistic view. Being able to search a code base for
SwitchRow and then somehow the code comes up where it was refactored into this
dynamic programming style.

Git can do this but only if you are actively looking for it. I wish you could
do a metacomment on the refactor without cluttering up the code.

~~~
shocks
Because if you understand the new code you don't need the "easier to read"
code.

Constraining yourself to only writing code a beginner can understand is
stupid. Maintaining good code and "easier to read for a beginner" code in
tandem is a waste of man hours.

------
vemv
Fabio's approach does the job, but it fails at genericity - its usefulness is
limited to the scope of the Formotion project. Which is unfortunate because
something actually simple is going on - a mapping of keywords to functions.

What you really want is multimethods - they provide all the goodness of
classic polymorphism, without forcing you to model intrincate, rigid class
hierarchies/relationships.

Some pseudocode illustrating the concept:

    
    
        # declaration. the 'dispatch function' we pass can build an arbitrary value out of the args it receives.
        defmulti build_cell lambda { |args| args[1].tag }
        
        # if the value the dispach function generates equals :submit, this body is gonna be called
        defmethod build_cell :submit do
            # implementation...
        end
        
        # more defmethods for :switch, :check, :edit etc
        
        build_cell(row, cell)
        # our lambda gets invoked, which re-passes the arguments it receives to the corresponding implementation
    

Multimethods doesn't seem to be have been adopted at all in the Ruby
community. A quick googling reveals a couple implementations though.

In the Lisp world they are first-class, even though they aren't used all the
time. <http://clojure.org/multimethods> might be worth a read.

------
roestava
Ruby is so unique. Back when I was also surprised by Ruby's reflection and
metaprogramming features. So much easier than it was in the languages I had
used before.

Nowadays I see people wanting to change Ruby to be more like those other
languages. Needless to say, Ruby is best when all of Ruby is available.
Putting restrictions on it wouldn't begin to turn it into what some people
want.

On the server-side, people can use all kinds of languages, with no
restrictions on choices. That's why these languages can flourish. On the
client-side we are more restricted. The industry gets to dictate what goes on
the client-side more.

Sigh.

I'm trying to use Dart lately which compiles to Javascript. While it can seem
a bit like Ruby in OO and dynamic typing, it's a world of difference in other
regards. Languages like Ruby that can evolve while breaking backward
compatibility have more chance to be useful out of the box.

------
greenyoda
It's scary to think that this guy who was just shocked to discover a very
basic concept of object oriented programming (polymorphism) has written his
own book on iOS programming:

<http://clayallsopp.com/posts/writing-a-programming-book>

~~~
AlexeyBrin
How many books did you wrote ?

~~~
martinced
I did write about twelve books and I still consider OP's point to be very
valid.

The blog entry is great in that it shows how collaborating over the Internet
can lead to improvement but it's indeed a bit concerning that someone who
didn't know what polymorphism was had been publishing a programming book.

~~~
AlexeyBrin
Clay wrote a book about RubyMotion (a Ruby compiler for iOS), not a book that
teaches OOP. His knowledge about polymorphism is not so relevant for this book
(I think he "knows" what polymorphism is but he was genuinely amazed by the
way you can use it for this particular case).

------
rasur
IIRC, Fabio works at the Swiss Ruby shop Simplificator, who are all very nice
chaps indeed! Glad he helped you along.

EDIT: I just checked, yes he does (well, unless we're talking about another
Fabio of course, which is possible...)

------
jakejake
I liked the article because I feel like I'm at the stage of my career that I
need to be a mentor. But at the same time I don't feel like I know enough
about everything.

Here is someone writing books and programming iOS apps who can still have his
mind blown by, what seems to me, some fundamental concepts. I think there are
a lot of people out there doing real work - who can benefit from a little
push. that makes me feel that a) I have things I can teach and b) that its ok
to not know everything - even what others may think is "basic" stuff.

------
tylerc230
Sounds like the open/closed principle of SOLID development. "Objects should be
open to extension but closed to modification". This basically means that users
of your code shouldn't have to modify your classes to add functionality. It
should be done through inheriance and adding new types.
<http://en.wikipedia.org/wiki/Open/closed_principle>

------
campnic
I like Martin Fowler's Refactoring. This is called 'Replace conditional with
subtype' explained here:
[http://www.refactoring.com/catalog/replaceConditionalWithPol...](http://www.refactoring.com/catalog/replaceConditionalWithPolymorphism.html)

The book is a good read and a fantastic reference.

------
svachalek
In terms of Gang of Four patterns, I believe this would be Strategy + Factory
Method. The book is a little antiquated (1994) but it or a better successor
should be required reading for any programmer.

~~~
chris_wot
I don't believe that its terribly antiquated. Pretty much everything bases
itself off of this book.

 _Edit:_ drafted iPad - fixed spelling. Trilby - does anyone even use that
word anymore?!?

~~~
martinced
_"I don't believe that it's the trilby antiquated. Pretty much everything
bases itself off of this book."_

Hardly.

The GoF book is sweet if in this day and age you're still doing Java/C# but if
you're programming in a language like, say, Clojure you can throw about 75% of
the GoF book into the trash.

pg said to use a Lisp to beat the average and that's what I'm doing. I'm
certainly not accepting the status-quo and considering that, say, the "visitor
pattern" is a godsend. Most of these "patterns" only make sense in a non-
functional and OO language and are only needed because of missing languages
concepts (like the lack of high-order function and/or the lack of real
macros).

~~~
svachalek
I don't think Clojure is really object-oriented, and therefore wouldn't
recommend the use of object-oriented design patterns at all. But as the
dominant paradigm of our age, I think it's worth understanding OOP even if the
final decision is to reject it.

------
juddlyon
Evocative writing for a technical post "serpentine" if/else, "rickety door" -
I know nothing about iOS and enjoyed reading it.

------
davidrudder
I like it when developers praise each other.

------
reyan
I would not go for polymorphism (specially with all that meta-programming)
with this simple conditional.

------
jQueryIsAwesome
I do something a little bit similar with DOM interaction in JS, for example
button ".btn_pdf_docs" shows all ".pdf_docs" elements. I consider the code to
implement this more readable than a bunch of events binding.

------
iamdave
I'm not a developer, but I read this article anyway.

 _All of this blew me away. Like this "@mordaroso" fellow strolled on up,
cocked his knee skyward, and jettisoned his leg into the rickety wooden door
that previously sheltered my mind._

That was just great writing.

~~~
mikeklaas
I have to disagree. "Jettison" does not fit the simile he was going for, at
all.

~~~
alecdbrooks
It's not great, but it could be good. Replace "jettison" and cut the cliched
"All of this blew me away" and the resulting paragraph is descriptive and
fairly tight.

