Redmine technical debt audit
Redmine is a project management Rails web app.
Summary
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:
- Inconsistent style, violating principle of least surprise.
- DRY violatons.
- Redundant "self".
Fixing the above traits will improve team happiness and feature development speed.
Recommendations
Style Consistency
Consistent code style is important for team happiness, and a great way to do it is to use Rubocop. It picks up redundant self, suggests to use guard clause, among other things.
Single Layer of Abstraction
Can be fixed with Extract Method refactoring.
Unclear intent
Case 1
Reader has to remember that ActiveRecord#id is nil when it's a new record.
Case 2
Not clear why @status_was is assigned nil. Extract Method is recommended to fix that.
Case 3
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.
Case 4
copy_from is misleading as author isn't copied from another issue. Rename Method is recommended to fix that.
Case 5
It's not clear at all, why to_s is required.
Case 6
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.
Case 7
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
Case 1
Have to remember that arg means issue or issue id. Code is always better than comments, so why not code that?
Case 2
Juggling a couple of variables named status_id is not easy. Write down context, use new_status_id.
Case 3, Case 4
Same as case 2.
Case 5
It may seem that h and 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.
Inconsistent style
Case 1, Case 2, Case 3
Some method arguments use full name, like user and status_id, some use abbreviated names like pid, and some use just arg.
Also, sometimes guard clause is used, and sometimes not.
DRY violations
Case 1, Case 2
Extract Method is recommended to fix that.
Redundant "self"
Case 1
Rubocop is highly recommended to suss out thigs like this.
Misc
Variable name reused
I don't know enough, but I'd not pass user to the block. Pass _ if you must pass something.
Silent fail
Controller passing attributes to safe_attributes would fail silently, if passed attrs isn't a Hash. Not nice to debug. I'd use raise.
Double negative
It'd be easier to read code that says that we're selecting read-only attributes. E.g. find_all.
Performance hit:
Case 1: Array#+
Use Array#concat, it doesn't create a new array, as #+ does.
Case 2: A calculating method called many times
Use a temp to store read_only_attribute_names result.
Difficult to read, possible race
Suppose the second call to super raises ActiveRecord::RecordNotFound?
Javaspeak (is_foo)
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.
