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):

  1. Single Layer of Abstraction violations.
  2. Intent not clearly expressed, leaving reader to deduce intent by themselves.
  3. Context not clearly written down in code, reader has to keep it in their head.

Less important negative code traits:

  1. Inconsistent style, violating principle of least surprise.
  2. DRY violatons.
  3. 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.

Site Footer

Web Analytics