Skip to content

Instantly share code, notes, and snippets.

@monfresh

monfresh/code_smells.md

Last active May 31, 2023 18:54
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save monfresh/cbf1504ffe88f3a95e73eb0f43fb73ea to your computer and use it in GitHub Desktop.
Save monfresh/cbf1504ffe88f3a95e73eb0f43fb73ea to your computer and use it in GitHub Desktop.
Download ZIP
Wisdom Wednesday Code Smells
Raw
code_smells.md

PDF

PAIN

DREAM

  • Wouldn't it be nice to be able to quickly make changes and add new features?
  • Wouldn't it be nice for new team members to jump in to the code faster?

FIX

  • There is a proven system that works. Learn code smells and their recipes.
  • Code smells indicate that there might be a problem. Forces us to think about whether or not there is a problem.
  • It's normal to have different levels of expertise. We also aren't requiring Ruby experience specifically when hiring (not sure why), so to level up the team, we need to include education as part of onboarding. Examples: Facebook's bootcamp.
  • Good news is that even without deep knowledge of code smells and OO patterns, there are tiny changes we can make that can have a big impact: extracting lines from long methods into smaller well-named methods, or just renaming methods in general. Example: distribute_priority_genpop_hearing_appeals instead of distribute_appeals(:hearing, priority: true, genpop: "only_genpop").
  • Start small. One small change at a time.
  • Advantage of learning code smells: it applies to many programming languages. It's core knowledge you can carry around from project to project.

STORY

  • Tiny Habits: from 2 pushups to 11 since October
  • Build habit of Continuous improvement. Why? 1% every day means 37% after a year.

99 bottles of oop

The lessons in the book have been found useful by programmers with a broad range of experience, from rank novice through grizzled veteran. Despite what one might predict, novices often have an easier time with this material. As they are unencumbered by prior knowledge, their minds are open, and easily absorb these ideas.

It’s the veterans who struggle. Their habits are deeply ingrained. They know themselves to be good at programming. They feel quick, and efficient, and so resist new techniques, especially when those techniques temporarily slow them down.

This book will be useful if you are a veteran, but it cannot be denied that it teaches programming techniques that likely contradict your current practice. Changing entrenched ideas can be painful. However, you cannot make informed decisions about the value of new ideas unless you thoroughly understand them, and to understand them you must commit, wholeheartedly, to learning them.

Therefore, if you are a veteran, it’s best to adopt the novice mindset before reading on. Set aside prior beliefs, and dedicate yourself to what follows. While reading, resist the urge to resist. Read the entire book, work the problems, and only then decide whether to integrate these ideas into your daily practice.

Why do we care about code quality?

  • Code is read much more often than it is written, therefore we must prioritize readability. "The reason we cost money is the time spent reading code" - Sandi Metz.
  • It currently takes a long time to understand how certain parts of the app work due to the way they were written
  • The longer it takes to understand code, the longer it takes to build new features
  • The harder the code is to read and change, the more prone it is to bugs - SHOW STATS
  • We want to make it easier for future developers to understand and maintain the code

Concerns and questions from previous WW on code quality

  • We spend too much time dealing with offenses
  • It's not worth it
  • Do we follow style guide?
  • It's too big of an effort. Example: yeah, I know that such and such class is too big.
  • How do we check locally?
    • editor plugins
  • Writable attribute doesn't apply to us because it's not a gem that other people will use
    • Writable from the outside doesn't mean outside our team. It means outside the class itself. When write Caseflow code, we could easily introduce a bug by using if x = instead of if x ==.

Continuous improvement

  • "If something seems hard to do, try it a little bit at a time" - Daniel Tiger
  • A dedication to making small changes and improvements every day, with the expectation that those small improvements will add up to something significant.
  • What is the smallest change we can make to improve the code?
  • 1% improvement every day compounds to 37% improvement after 1 year (1.01^365).
  • Trying to tackle a big refactor all at once is too overwhelming. Just like habits, start small.
  • Only refactor as you touch the code. No need to refactor code that never changes.
  • https://jamesclear.com/continuous-improvement
    • Do more of what already works
    • Avoid tiny losses
    • Improvement is not about doing more things right, but about doing fewer things wrong.
  • We have been improving since introducing Code Climate: https://codeclimate.com/github/department-of-veterans-affairs/caseflow/trends/technical_debt

After I.., I will...

  • Effective Engineer
  • Heidi Halverson

Examples

def available_actions(user)
  return [] unless user

  if assigned_to == user
    return [
      Constants.TASK_ACTIONS.ASSIGN_TO_TEAM.to_h,
      Constants.TASK_ACTIONS.REASSIGN_TO_PERSON.to_h,
      appropriate_timed_hold_task_action,
      Constants.TASK_ACTIONS.MARK_COMPLETE.to_h,
      Constants.TASK_ACTIONS.CANCEL_TASK.to_h
    ]
  end

  if task_is_assigned_to_user_within_organization?(user)
    return [
      Constants.TASK_ACTIONS.REASSIGN_TO_PERSON.to_h
    ]
  end

  if task_is_assigned_to_users_organization?(user)
    return [
      Constants.TASK_ACTIONS.ASSIGN_TO_TEAM.to_h,
      Constants.TASK_ACTIONS.ASSIGN_TO_PERSON.to_h,
      Constants.TASK_ACTIONS.MARK_COMPLETE.to_h,
      Constants.TASK_ACTIONS.CANCEL_TASK.to_h
    ]
  end

  []
end
  • Extract method with better name: makes it more readable and signals that the implementation is not important. Might also surface a missing abstraction

Data Clump

  • Search for "start_date" in Caseflow: tons of start_date, end_date data clumps

Feature Envy vs Replace Temp with Query

class DocketRangeJob < ApplicationJob
  queue_as :low_priority

  def perform
    for_month = Time.zone.now + 1.month
    days_in_month = Time.days_in_month(for_month.month, for_month.year)
    appeals_to_mark(days_in_month).update(docket_range_date: for_month.end_of_month)
  end

  def appeals_to_mark(days_in_month)
    dc = DocketCoordinator.new
    target = dc.target_number_of_ama_hearings(days_in_month.days)
    dc.dockets[:hearing].appeals(priority: false).where(docket_range_date: nil).limit(target)
  end
end

God Object

cd app/models
wc -l * | sort
508 end_product_establishment.rb
725 appeal.rb
787 request_issue.rb
1000 legacy_appeal.rb
1398 regional_office.rb

Other points of view

  • Discussion should only happen once in order to reach a consensus and we use that going forward
  • Stop and think about the offense. Example above: why not use string interpolation?

Code Smells:

Further Reading and viewing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment