Making some Discourse code a little better | Hacker News Reader

Making some Discourse code a little better

grantammons.me

123 pointsjanerik19 comments

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!

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

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.

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.")

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

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.

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

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

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?

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.

I would love to see that implemented! I wonder if it could be an emacs mode or vim plugin with split views or doxygen-style inline comments?

Have you seen Docco?

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

My intent was to make the show method read almost like pseudocode when I was done with it, so the reader could easily see, at a glance, a high-level overview of what was happening with that method.

I tried to make the experience of seeing that code sort of like reading the front page of a newspaper. You see the headlines, but if you want to drill down, you gotta go more in depth. It also allowed me to keep the size of the methods down.

This entire refactor was meant to make the code more readable, which is why I made some of the decisions I made.

Ah, I see what you mean. I've never really thought about how one approaches methods from a structural perspective (especially controller methods).

I suppose for something like the respond_to block, which is shared to pretty much every controller method (and unique to that method, most likely), my thought was that its absence makes the method slightly more confusing for the experienced Rails developer.

So, would you extend this reasoning to every controller method? ('perform_index_response', 'perform_edit_response', etc.)

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.

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.

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.

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.

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.

Reply on news.ycombinator.com