Redmine technical debt audit
Redmine is a project management Rails web app.
I’ve analysed app/models/issue.rb, the file changed most often in the past year.
Techical debt size: medium.
You can expect medium speedup after repaying the technical debt.
Most important negative code traits (fix these for most impact):
- Single Layer of Abstraction violations.
- Intent not clearly expressed, leaving reader to deduce intent by themselves.
- Context not clearly written down in code, reader has to keep it in their head.
Less important negative code traits:
Fixing the above traits will improve team happiness and feature development speed.
Single Layer of Abstraction
Can be fixed with Extract Method refactoring.
Reader has to remember that
nil when it’s a new record.
Not clear why
@status_was is assigned
nil. Extract Method is recommended to fix that.
base is a very generic term, I take it that it means
ActiveRecord::Base, but it’s easy to confuse it with the other meaning of
base. Rename Method is recommended to fix that.
copy_from is misleading as
author isn’t copied from another issue. Rename Method is recommended to fix that.
It’s not clear at all, why
to_s is required.
status is used as a temporary variable, assigned to twice. Split Temporary Variable might give you some ideas. Don’t use an instance variable as a temp.
It’s really a cache for current user, but it’s not easy to read. It also complicates the method. Cache functionality is probably better off outside of this method (and then, it can be scaled to have cache for all users).
Context not clearly written down in code
Have to remember that
arg means issue or issue id. Code is always better than comments, so why not code that?
Juggling a couple of variables named
status_id is not easy. Write down context, use
Same as case 2.
It may seem that
wp variables are only used on 4 lines, but you still have to have internal mapping for their meaning. And they’re not like a brand name, that you will encounter them everywhere. You need to map them every time you read this code.
Some method arguments use full name, like
status_id, some use abbreviated names like
pid, and some use just
Also, sometimes guard clause is used, and sometimes not.
Extract Method is recommended to fix that.
Rubocop is highly recommended to suss out thigs like this.
I don’t know enough, but I’d not pass
user to the block. Pass
_ if you must pass something.
Controller passing attributes to
safe_attributes would fail silently, if passed
attrs isn’t a
Hash. Not nice to debug. I’d use
It’d be easier to read code that says that we’re selecting read-only attributes. E.g.
Array#concat, it doesn’t create a new array, as
Use a temp to store
Suppose the second call to
In Ruby, a question mark in the method name relays that it returns a boolean.
is_ is completely redundant here.
Reduce technical debt by refactoring
Give your team time to refactor. In his book Refactoring: Ruby edition, Martin Fowler suggests to refactor all the time in little bursts, instead of setting aside time to do it. Refactoring the code that was just written, refactoring to understand, refactoring before adding a feature (to make it easy adding it) are some good reasons to refactor (read the book for more). Also, there’s “Boy Scout Rule”, telling us to always leave the code behind in a better state than you found it.