
Making some Discourse code a little better - janerik
http://grantammons.me/2013/02/09/making-some-discourse-code-a-little-better/
======
EvilTrout
A couple of weeks ago, I was talking to a friend about the impeding launch of
Discourse. She asked me if I was worried that people would judge me for the
code I'd written (although to be honest, you can't tell who wrote what since
we reset our commit history when we launched).

The truth is releasing your code is a very scary thing to do. I knew some
parts were good, some parts less so.

Posts like this (and their pull requests) have floored me. Grant did a
fantastic job refactoring a large chunk of technical debt we'd acquired, and
then took the time to write up why he did it in detail.

I really hope this trend of improving something and writing how you did it
catches on. I want to see it in other people's code bases too!

~~~
sanderjd
Out of curiosity - was there any particular reason you reset your commit
history at launch? I feel somewhat impotent when I come across a chunk of code
I don't understand the reasoning of and discover that its history dead-ends at
the initial release commit.

If it is a clever way to get a bunch of people using Discourse right off the
bat by asking questions in the meta forum [1], then it is ingenious and I
applaud you!

1\. [http://meta.discourse.org/t/would-it-be-possible-to-see-
the-...](http://meta.discourse.org/t/would-it-be-possible-to-see-the-full-
history-of-the-repo/2610/2)

~~~
EvilTrout
Honestly it came down to there being private keys and other shifty things in
there at various points. Also there was a lot of old crap as we evolved the
prototype closer to what we released... We just thought it was a good idea.

~~~
patio11
_private keys and other shifty things_

This was a SOP at a company I worked at, for any repository, either for OSS or
when delivered as work product to customers. We just didn't want to have to do
a commit-by-commit, line-by-line audit for any of an unbounded universe of
Things That Would Be Very Bad If They Leaked. A surprising amount of them are
not things that are technical in nature -- for example, many developers treat
commit messages as watercooler talk between themselves rather than something
which could potentially be reviewed in a courtroom. (Guess the precipitating
event: "Memo to all developers: One should never, ever, ever, ever, ever
mention a developer who is no longer with the firm in a commit message.")

~~~
mryan
> (Guess the precipitating event: "Memo to all developers: One should never,
> ever, ever, ever, ever mention a developer who is no longer with the firm in
> a commit message.")

Is the purpose here to limit the amount of material that might show up in
discovery, and therefore potentially be made public?

~~~
rmc
If X was made redundant rather than fired for incompetence, and then commits
were to mention that X was a terrible developer, then if X sues for unfair
dismissal, then you've just given X evidence that the company lied.

~~~
danielweber
"They said I was redundant, but internal proof shows they think I sucked."

Could you draw that out for me a little more? Because to me it seems
backwards.

~~~
rmc
It's a non-perfect example. IANAL, but I think the main problem is that the
company is caught lying.

------
octernion
I really enjoyed reading this!

When Discourse was originally announced, I read this file (quite randomly),
and was distressed at the complexity and length (even though I totally
understand time constraints and the need to 'get it to work').

I had never heard of the Law of Demeter
(<http://en.wikipedia.org/wiki/Law_of_Demeter>), nor had I heard of Sandi
Metz's rules for Rails development (<http://gist.io/4567190>), both of which
put in concrete terms some of the lessons I've learned when maintaining Rails
applications.

I'd love to see more of these!

Edit: After reading over the pull request, I admit I'm a little confused why
the response code was refactored into 'perform_show_response' - this seems
like the direct responsibility of this controller method (and isn't common or
otherwise misplaced). Is there a reason why you moved it out?

~~~
city41
One idea I've always had in the back of my head but have never pursued was
plugins for text editors/IDEs that allow developers to quickly and easily make
commentary on the code they are writing, and the end result would be blog
posts formatted much like this one.

~~~
philfreo
Have you seen Docco?

<http://jashkenas.github.com/docco/>

------
tinco
This post is great, I'd love to see more blog posts about refactorings of open
source code on Hacker News.

It's good to be reminded of refactoring techniques, and to be reminded of what
neatly refactored complex code looks like.

I think being able to refactor code, and make it look that good is a hallmark
of a great programmer.

~~~
jevinskie
I really liked how the author incorporated gists of the intermediate code
throughout the post. I will definitely follow their example if I pursue a
similar endeavor.

------
ericb
Good overall, but is there justification for having
consider_user_for_promotion called through a before_filter? It doesn't filter
anything or redirect the request. It also doesn't retrieve instance variables
for the request to process. You could theoretically put the whole method being
refactored into a before_filter, but that's just a shell game.

I agree with making consider_user_for_promotion a standalone function. I like
that because it is reusable, and self-comments the code.

~~~
spohlenz
A before_filter is just a method that runs before an action - there's no
requirement for it to set an instance variable or redirect. In Rails 4, this
method has been aliased as before_action to make this clearer.

Since the behavior of this method doesn't have too much to do with showing a
topic, I think it makes sense. Extracting it also makes it possible to move it
up to the ApplicationController for wider use.

~~~
ericb
Extracting it into a separate method is what makes it available for wider use
--not the before_filter.

That said, you provide a justification--the topicality of the code itself to
the idea of "show." It is a decent point that I missed in the article. I'll
have to think on that.

I question this logic living in the controller at all, though. It looks like a
model logic that should, from the controller perspective, look like
current_user.review_for_promotion.

