1
\$\begingroup\$

My app is setup where there are musical artists and albums have message boards through a has_one polymorphic association.

class Artist < ApplicationRecord  
  has_one :board, as: :boardable, dependent: :destroy

  after_create -> (artist) { BoardGenerator.create(artist) }
  after_update -> (artist) { BoardGenerator.update(artist) }
end

class Album < ApplicationRecord  
  has_one :board, as: :boardable, dependent: :destroy

  after_create -> (album) { BoardGenerator.create(album) }
  after_update -> (album) { BoardGenerator.update(album) }
end

class Board < ApplicationRecord
  belongs_to :boardable, polymorphic: true, optional: true
end

For every artist and album created, I'd like to create a message board associated to it. I decided to accomplish this using the after_create/after_udpate callbacks where business logic is extracted to a service object:

class BoardGenerator
  def self.create(boardable)
    boardable.board = Board.create(name: boardable.name)
  end

  def self.update(boardable)
    boardable.board.update_attribute(:name, boardable.name)
  end
end

One of my main concerns is that the create/update methods are dependent on both models having a name attribute. Is there any way to mitigate the risk of this ever changing between the two?

I'm trying to get in the habit of creating single responsibility models (hence the service object). Still new to the concept though. Can someone tell me if I'm on the right track and/or how to improve the code I have now?

\$\endgroup\$
  • \$\begingroup\$ This would be easy to do if you could override the constructor for Album and Artist. ActiveRecord tends to be picky about this. \$\endgroup\$ – Greg Burghardt Jun 22 '18 at 17:55
0
\$\begingroup\$

Updated Answer: It's funny, because now that I've revisited this since your comment, I've flip-flopped my thinking a little. I still feel like the BoardGenerator class is unnecessary.

Revisiting this, I noticed you mentioned the BoardGenerator being unnecessary. If that was the case how would a board know when to be created or updated without having some sort of link to a boardable?

The real issue is keeping the board name synchronized with the album or artist, and coordinating the creation of these dependent objects. The "boardable" should be the one responsible for this synchronization operation.

First, let's think about this problem from the standpoint of code using the Artist class.

When creating a new "boardable" I would expect this:

artist = Artist.create(name: "Prince")
puts artist.board.name # Prints "Prince"

The board is created automatically and gets the same name as the artist. Your code accomplishes this through the use of a separate class. Since it is the Artist that needs the board, and the Artist needs the board to have the same name, this behavior should be encapsulated within the Artist class completely. You can accomplish this by utilizing the after_initialize callback in ActiveRecord:

class Artist < ApplicationRecord  
  has_one :board, as: :boardable, dependent: :destroy

  after_initialize -> (artist) { create_board(name: artist.name) unless artist.persisted? }
end

Now there is the issue of updating the name of the board when a "boardable" updates its name. Again encapsulation is our friend. Overriding the name= method on a "boardable" allows the names to be synchronized:

class Artist < ApplicationRecord  
  has_one :board, as: :boardable, dependent: :destroy

  after_initialize -> (artist) { create_board(name: artist.name) unless artist.persisted? }

  def name=(new_name)
    super.name = new_name
    board ||= create_board(name: new_name)
    board.name = new_name
  end
end

Now when we get proper synchronization on update as well, without this logic leaking to other classes, or requiring another class that has little logic in it at all:

artist = Artist.find 23
artist.name = "The Artist Formerly Known As Prince"
puts artist.board.name # Prints "The Artist Formerly Known As Prince"

It's been a while since I've written much Ruby, but I feel like this behavior could be pulled out into its own Module and included as a mixin to both the Artist and Album classes.


Original Answer: I used to think the Board should be responsible for this stuff, but I've since reconsidered (see above).

I don't see an issue with your design, other than the BoardGenerator seems unnecessary. I would just go with static methods on the Board class itself. No need to separate this logic.

Responses to comments:

Carl Edwards asked:

Creating static methods wouldn't violate the principle of single mode responsibility?

The Single Responsibility Principal states that a class or method should only have one reason to change.

The rule is, if a Board is attached to a Boardable, keep the names of the two things in sync. The name of the Board is what gets affected, and the Board class should be responsible for that.

Encapsulating this logic in the Board class is much more important that tightly adhering to the SRP for one simple reason: encapsulation.

Encapsulating data and behavior is far more important in this case. I also don't see how it violates the SRP. If it does violate the SRP, encapsulation is far more important than a "principal," because encapsulation is one of the foundational aspects of good object oriented code.

Wow. I've written "encapsulation" a lot in the last two paragraphs, but I hope it really drives the point home that encapsulating the data and behavior of a Board is the most important aspect here.

\$\endgroup\$
  • \$\begingroup\$ Creating static methods wouldn't violate the principle of single mode responsibility? \$\endgroup\$ – Carl Edwards Jun 22 '18 at 18:07
  • \$\begingroup\$ @CarlEdwards: I updated my answer in response to your question. \$\endgroup\$ – Greg Burghardt Jun 22 '18 at 18:18
  • \$\begingroup\$ Revisiting this, I noticed you mentioned the BoardGenerator being unnecessary. If that was the case how would a board know when to be created or updated without having some sort of link to a boardable? \$\endgroup\$ – Carl Edwards Sep 21 '18 at 12:06
  • \$\begingroup\$ @CarlEdwards: I revisited my answer since you revisited the question. Looking at this now I think responsibilities need to reside with each "boardable". See my updated answer. \$\endgroup\$ – Greg Burghardt Sep 21 '18 at 12:58
  • \$\begingroup\$ Thanks for the quick and thorough response. I just wonder if after_initialize would be the best choice. Reading up in the rails docs it states that it's, "called whenever an Active Record object is instantiated, either by directly using new or when a record is loaded from the database.". With this in mind wouldn't you essentially be creating a board regardless of the boardable passing validations? As far as your second point, I agree that the writer functionality could/should be put into a module. Working on a concern for that. \$\endgroup\$ – Carl Edwards Sep 21 '18 at 13:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.