1
\$\begingroup\$

I'm making an app using Rails. I have 4 Boolean columns in a table like below, and I want to show a message if that column is true.

class CreatePosts < ActiveRecord::Migration[5.2]
  def change
    create_table :posts do |t|
      t.integer :user_id, null: false, default: 0
      t.string :title, null: false, default: ''
      t.text :description, null: true
      t.boolean :tag_1, null: false, default: false
      t.boolean :tag_2, null: false, default: false
      t.boolean :tag_3, null: false, default: false
      t.boolean :tag_4, null: false, default: false

      t.timestamps
    end
  end

posts_controller.rb

def index
  @posts = Post.all
end

index.html.erb

<% @posts.each do |post| %>
  <ul>
    <% if post.tag_1 %>
      <li><%= 'tag_name_A' %></li>
    <% end %>
    <% if post.tag_2 %>
      <li><%= 'tag_name_B' %></li>
    <% end %>
    <% if post.tag_3 %>
      <li><%= 'tag_name_C' %></li>
    <% end %>
    <% if post.tag_4 %>
      <li><%= 'tag_name_D' %></li>
    <% end %>
    <% if !post.tag_1 && !post.tag_2 && !post.tag_3 && !post.tag_4 %>
      <li>none</li>
    <% end %>
  </ul>
<% end %>

tag_name_A will be shown if tag_1 is the only true column, and there'll be 'tag_name_B' & 'tag_name_D' shown if tag_2 and tag_4 are both true. (I hope I'm making myself clear.)

This code is working fine and I'm already getting what I want, but I just don't like how they are written. It looks messy and obviously not smart. How would you optimize it?

\$\endgroup\$
  • \$\begingroup\$ Is using 4 boolean columns a requirement? That's not really the right way to build a tagging system. Typically you you use something like a tags table and a junction table joining posts to tags, which lets you have an arbitrary number of tags and makes the data a lot easier to work with. \$\endgroup\$ – meagar Dec 3 '18 at 12:35
1
\$\begingroup\$

There's a lot of great work here, I have a few suggestions, I'll start with a few recommended changes, and finish with some suggestions for improvements.

Let's firstly take a look at your migration.

Do not set a default on a foreign key column. Your user_id defaults to 0, which could be an actual user record, so if a record was ever (somehow) created or modified to without a user_id, it would suddenly belong to that user.

You can take this opportunity to add an index on the user_id for the posts table, it will make certain requests significantly faster, such as looking up all of the posts belonging to one user.

Moving on to your view.

You can add a method to your Post model to indicate if the post has no tags. Rather than using !post.tag_1 && !post.tag_2 && !post.tag_3 && !post.tag_4 in the view, we can make a method to handle that logic for us.

Imagine something like this in your Post model:

def no_tags?
  !tag_1 && !tag_2 && !tag_3 && !tag_4
end

This would mean you could change your view from:

<% if !post.tag_1 && !post.tag_2 && !post.tag_3 && !post.tag_4 %>
  <li>none</li>
<% end %>

to:

<% if post.no_tags? %>
  <li>none</li>
<% end %>

which I think reads a lot more easily. This kind of method, where you're not modifying data but simply reading it, is sometimes refactored (tidied up) using design patterns like decorators or presenters. It's common to have methods that are really only used for the presentation of data, rather than manipulation, and those design patterns can help keep them away from the business logic of the model.

Those are the first minor changes I'd make to this code, without significantly changing the architecture. If you wanted to take it further, however, I would suggest the following.

It's clear from what you've said in your post that you can see there's something not quite right. Those conditionals in the view are probably the obvious 'smell'.

Think through a moment, and consider what you would do if you needed to add another tag.

You'd first need to add a column in the posts table, to store the boolean value. You'd need to modify our new no_tags? method to include tag_5, and you'd need to change the view to add another conditional, checking to see if tag_5 was present.

I think you'll be much better off if you define a many-to-many relationship between the Post model and a new Tag model. Each tag model can have a name, and your post model will has_and_belongs_to_many tags, allowing you to change your view code to something like:

<% post.tags.each do |tag| %>
  <li><%= tag.name %></li>
<% end %>

I recommend you read the rails guide on relationships, specifically: https://guides.rubyonrails.org/association_basics.html#has-and-belongs-to-many-association-reference

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for your kind reply. It's really helpful. The only thing that I'm still unclear is to not set a default value on a foreign key column. Should I allow NULL and not set a default, or should I keep it NOT NULL without setting a default? (It feels weird setting it NOT NULL without giving a default value. Is that a right thing to do?) \$\endgroup\$ – ta539tg70 Dec 4 '18 at 0:33
  • \$\begingroup\$ @ta539tg70 there's a lot of opportunity here for personal preference.. I would at least allow null (remove the NOT NULL), beyond that 'best practice' would be to use the rails method to define it as a proper foreign key and include a foreign key constraint (robots.thoughtbot.com/referential-integrity-with-foreign-keys old article, still relevant) using something like t.references :user, index: true, foreign_key: true \$\endgroup\$ – Dan Dec 4 '18 at 23:21
  • \$\begingroup\$ Ah I see. Thank you again. I've learned a lot from you! \$\endgroup\$ – ta539tg70 Dec 5 '18 at 6:11

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.