killswitch0 / goal_tracker Goto Github PK

Reviewing tasks_controller.rb (https://github.com/Killswitch0/goal_tracker/blob/main/app/controllers/tasks_controller.rb), we could notice we use the same code block 5 times in different actions:

@goal = Goal.find(params[:goal_id])

Another one 4 times:

@task = Task.find(params[:id])

Suggestion:
We could move both of them into separate setter methods that are using conditional variable initialization ||= (https://rubystyle.guide/#double-pipe-for-uninit):

private def goal
  @goal ||= Goal.find(params[:goal_id])
end

private def task
  @task ||= Task.find(params[:id])
end

We can utilize those methods instead of repeating the same code multiple times. Additionally, we can place them in before_action callbacks to set the variables automatically without explicitly invoking those methods in the actions. Additionally, we could make them helper methods, allowing access to them from the views.

We can apply this concept across all controllers within the project.

As another suggestion, it would be very helpful for you to write a documentation for each code unit.
You could use something like https://github.com/lsegal/yard to systematize it.

For example your actions would look like this:

  # Checks if the habit was completed today.
  #
  # @return [Boolean]
  def completed_today?
    completion_dates.created_today.exists? && self.completed?
  end
  ...
  # GET /goals/:goal_id/habits/new
  def new
    @habit = Habit.new
  end

  # GET /goals/:goal_id/habits/:id/edit
  def edit
    @habit = Habit.find(params[:id])
  end
  
  # POST /goals/:goal_id/habits
  def create
    @habit = @goal.habits.build(habit_params)
    @habit.user = current_user
  ...

Code documentation is crucial for ensuring clear understanding, promoting collaboration, and simplifying maintenance (it just helps everyone).

We have two exactly same methods in habit.rb and goal.rb:

def self.search(search, user)
  where('lower(name) LIKE ? AND user_id = ?', "%#{search.downcase}%", "#{user.id}") if search
end

To follow the DRY principle, we could move this definition to the searchable.rb concern (app/models/concerns), which would look like that:

# frozen_string_literal: true

module Searchable
  extend ActiveSupport::Concern

  class_methods do
    def search(search, user)
      return unless search
    
      where('lower(name) LIKE ? AND user_id = ?', "%#{search.downcase}%", user.id)
    end
  end
end

Also, please review the refactored method using the "GuardClause" (https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/GuardClause)