Cfrtjryk

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 %>

So '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.)

These codes are 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. (You can tell that I'm a newbie.) How would you optimize them?

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