← Back to Upcase

Concern or new class which inherits


(Nicolo) #1

I’m trying to make a design choice on some code I’m refactoring.

In my application I have a ActiveRecord model ContestEntry. After a certain time, the entries are considered live. I need to display these entries in a table which I populate via json. There are methods that I would call on the entries only when they are live. I’m debating using a concern or a new class which inherits from ContestEntry. Feel free to suggest something completely different.

Example new class:

class LiveContestEntry < ContestEntry
 
##some methods

## override as_json

end

Example concern:

module Concerns
	module Live
		extend ActiveSupport::Concern 

            ##some methods
        
           ##live_to_json
        end
end

(Greg Lazarev) #2

It is recommended to use composition over inheritance. Mixins are a form of inheritance. With composition you can structure your code so LiveContestEntry has a ContestEntry. Then you can delegate certain methods to the ContestEntry instance.

Example:

def show
  contest_entry = ContestEntry.find(params[:id])
  @entry = derive_entry(contest_entry)

  render json: @entry
end

private

def derive_entry(contest_entry)
  if contest_entry.live?
    LiveContestEntry.new(contest_entry)
  else
    SomeOtherContestEntry.new(contest_entry)
  end
end
class LiveContestEntry
  delegate(
    :name,
    :description,
    to: :contest_entry
  )  

  def initialize(contest_entry)
    @contest_entry = contest_entry
  end

  def to_json
    ...
  end

  private

  attr_reader :contest_entry
end

(Holger Frohloff) #3

I like the proposed solution. Only thing I would want to add is a change to derive_entry using tell, don’t ask:

def show
  contest_entry = ContestEntry.find(params[:id])
  @entry = contest_entry.derive_entry

  render json: @entry
end

class ContestEntry
  def derive_entry
    if self.live?
      LiveContestEntry.new(self)
    else
      SomeOtherContestEntry.new(self)
    end
  end
end

This encapsulates the logic when to derive into the Entry class. I think the entry should know what to do, if it’s live. It also makes it easy to derive new classes, if you need any other classes in the future.


(Nicolo) #4

I see the value of composition over inheritance here. This will not be a show action but instead be an index action with between 1 - 300+ entries. The query to get the live contests will be involve multiple includes.

Will the efficiency of the includes hold in this situation?

After the query am I going to have to loop over the all the entries and convert them to a LiveContestEntry?


(Holger Frohloff) #5

I don’t have enough experience in this situation to give you a proper advice. One thing I know: If you wonder wother something is fast anough you have two choices:
a) try and get along with it until you experience the limitations, then change it
b) measure beforehand and decide with real data