recent comments

[aram harrow]

A sidebar displaying recent comments is now up.
Let me know if this broke anything.

aram

[Bill Rosgen]

On 2012-07-02, at 17:40 , aram harrow wrote:

> A sidebar displaying recent comments is now up.
> Let me know if this broke anything.

While nothing seems broken, I would implement things differently in two ways.

I don't think it's a good idea to call Paper.find and User.find from the view. If we need to do that, we should probably be doing it in the controller. In this case there's no need, since given a comment c, c.user and c.paper return the relevant user or paper. I'd prefer in general if we did very little in views other than handle formatting and displaying information, as they're not easy to read with all the ruby embedded into the HTML.

This doesn't solve my other problem: when we start from a list of comments and then find the user and paper associated with each comment individually, it requires 2n database queries to find the information we want. This is pretty much the classic 'n+1' problem and is usually solvable by telling active record what information to fetch with the first query.

I've made changes that make me happier and committed them -- hopefully you don't mind if I edit your code.

There are also a bunch of test cases now failing (at least on my machine) relating to displaying papers. Nothing appears broken, so it may just be that the test cases can't handle the recent formatting changes, but I'll try to have a look and figure things out. I'll also plan to add test cases to cover recent comments, author links, etc.

Bill

[aram harrow]

I should've added: I'm sorry for not knowing rails, ruby or the whole
model-view-controller thing. Even CSS I just used for the first time.
So I hope my appalling programming style isn't too annoying, and I'm
happy for my code to be rewritten. Probably Ishould either soon learn
what I am doing, or stop touching the codebase.

Anyway, thanks for fixing things up this time.

aram

[Bill Rosgen]

Aram,

I certainly don't mean to suggest that you shouldn't make changes to the codebase -- I am no expert either. Please feel free to keep making changes. If I see anything that I think is done "wrong", I'll point it out, but you and everyone else should feel free to disagree with me. I'm sure that I've done many things incorrectly when setting up the current code -- I'd love to know what they are so that I can avoid making the same mistakes later.

I've had a look at the tests: the failures were due to the fact that I was testing for papers expecting to see a link to the id, but for a while now the site has been linking to the title instead (which is a change that I agree with). I've updated the tests -- I've put the paper testing code into a helper that should be easier to change in the future. The test suite is a bit of a disaster at the moment -- I think it really needs to be broken up into smaller modules, but I haven't found the time to do this.

If you and the other developers don't want the hassle of fighting with the test suite, I can be persuaded to drop it. For the time being I'll try to keep it updated to cover new functionality: it has saved me from subtle bugs on several occasions in the past, but I agree that maintenance can be a lot of work -- there are currently more lines of code in the test suite than the rest of the site.

Bill

[aram harrow]

Well, I just ignored the test suite because I hadn't read that part of
the ruby tutorials. :)
If you don't mind me coding like a barbarian, I'll keep on adding
features. Hopefully it's quicker for you to refactor my messes than
coding these things in the first place? If not, I'll reassess my
approach.

aram