iTranslated by AI
Code Reviewing My Own Rails App Based on 'Learning Ruby on Rails Through Code Reviews'
Introduction
Hello! This is Ookuma!
In this article, I have summarized my thoughts on code reviews after reading "Learning Ruby on Rails through Code Reviews" and applying those practices to my own web application.
I hope this proves helpful to you!
Target Audience
Note
What is "Learning Ruby on Rails through Code Reviews"?
"Learning Ruby on Rails through Code Reviews" is a technical book sold at Techbook Fest. It was written by Ichiro Nakatani, Yoshito Tanaka, and Masato Soura from SonicGarden, Inc., and proposes better coding practices for web application development using Ruby on Rails!
It explains code reviews in a very easy-to-understand way, and it was very accessible even for a beginner like me!
The Web App for Code Review
I changed careers and became a web engineer in November 2023 with no prior experience!
I created a web application with Ruby on Rails for my job hunt, so I will be code reviewing that now!
Code Review
Separate models for general users and administrators
class User < ApplicationRecord
authenticates_with_sorcery!
has_many :favorite_zoos, dependent: :destroy
has_many :zoos, through: :favorite_zoos
has_many :posts, dependent: :destroy
has_many :likes, dependent: :destroy
validates :name, presence: true
validates :email,presence: true, uniqueness: true
validates :password, length: { minimum: 3 }, if: -> { new_record? || changes[:crypted_password] }
validates :password, confirmation: true, if: -> { new_record? || changes[:crypted_password] }
validates :password_confirmation, presence: true, if: -> { new_record? || changes[:crypted_password] }
validates :reset_password_token, uniqueness: true, allow_nil: true
enum role: { general: 0, admin: 1}
mount_uploader :avatar, AvatarUploader
end
class Admin::BaseController < ApplicationController
before_action :require_login
before_action :check_admin
layout 'admin/layouts/application'
private
def not_authenticated
redirect_to admin_login_path, alert: t('message.require_login')
end
def check_admin
return if current_user.admin?
redirect_to root_path, alert: t('message.no_authority')
end
end
These are the user model file and the controller file containing the logic to determine if a user is an administrator.
I was using an enum with a role column in the user model to distinguish between general users and administrators!
However, according to the technical book, it is recommended to separate models for two reasons:
-
Prone to security issues due to errors in judgment logic
When using the same model, you need judgment logic to treat specific users as administrators. If there is an error in this logic, a general user might be incorrectly identified as an administrator, leading to security accidents. Therefore, when managing permissions with admin flags like enum, you must be very careful when writing the judgment logic. -
Requirements for general users and administrators often differ
General users and administrators often require different information. As requirements diverge, you end up with many columns and code snippets that are unused by one side or the other, making it pointless to manage them in the same model.
For these reasons, separating the models for general users and administrators is safer and keeps the code to a minimum, which is why it is recommended.
Always apply NOT NULL constraints in the database for columns that must have values
class CreateFavoriteZoos < ActiveRecord::Migration[7.0]
def change
create_table :favorite_zoos do |t|
t.belongs_to :user, index: true
t.belongs_to :zoo, index: true
t.integer :rank
t.timestamps
end
end
end
class FavoriteZoo < ApplicationRecord
belongs_to :user
belongs_to :zoo
validates :rank, presence: true, numericality: { only_integer: true, greater_than_or_equal_to: 1, less_than_or_equal_to: 3 }
validate :validate_rank_uniqueness
validate :validate_zoo_uniqueness
private
def validate_rank_uniqueness
return unless rank.present? && user_id.present?
existing_records = FavoriteZoo.where(user_id: user_id, rank: rank)
errors.add(:rank, "is already registered") if existing_records.exists?
end
def validate_zoo_uniqueness
return unless zoo_id.present? && user_id.present?
existing_records = FavoriteZoo.where(user_id: user_id, zoo_id: zoo_id)
errors.add(:zoo_id, "is already registered") if existing_records.exists?
end
end
These are the migration file and model file for the intermediate table where users register their top 3 favorite zoos!
The rank column should always contain a value between 1 and 3. While there is a presence: true declaration in the model file, there is no null: false declaration in the migration file, meaning the database level allows NULL values.
Rails validations are application-level constraints, but NOT NULL constraints are lower-layer database-level constraints and are therefore more reliable. Since validations can sometimes be skipped, you should also apply restrictions at the database level for columns that must always have a value!
Use describe/context/it effectively in tests
require 'rails_helper'
RSpec.describe User, type: :model do
it "can be registered if name, email address, and password are provided" do
expect(FactoryBot.build(:user)).to be_valid
end
it "cannot be registered without a name" do
expect(FactoryBot.build(:user, name: "")).to be_invalid
end
it "cannot be registered without an email address" do
expect(FactoryBot.build(:user, email: "")).to be_invalid
end
it "cannot be registered if the email address is duplicated" do
user = FactoryBot.create(:user)
expect(FactoryBot.build(:user, email: user.email)).to be_invalid
end
it "cannot be registered without a password" do
expect(FactoryBot.build(:user, password: "")).to be_invalid
end
it "cannot be registered without password confirmation" do
expect(FactoryBot.build(:user, password_confirmation: "")).to be_invalid
end
it "cannot be registered if password and confirmation do not match" do
expect(FactoryBot.build(:user, password_confirmation: "password_confirmation")).to be_invalid
end
end
This is the actual model spec for users that I wrote. Looking back at this test code after reading the technical book, I realize it's quite poor. Haha.
The book explained how to use describe/context/it as follows:
-
describe
- Describes the test target.
- Often used for class names, method names, or feature names.
-
context
- Describes the situation or condition.
- Often used for expressions like "when ...".
-
it
- Describes the expected behavior/result.
- Often used for expressions ending in "should ..." or "can ...".
With this in mind, I partially rewrote my model spec!
require 'rails_helper'
RSpec.describe User, type: :model do
context "when name, email, and password are present" do
it "can register a user" do
expect(FactoryBot.build(:user)).to be_valid
end
end
context "when name is missing"
it "cannot register a user" do
expect(FactoryBot.build(:user, name: "")).to be_invalid
end
end
end
This makes the test conditions and expected behaviors much clearer! I intend to write my tests this way from now on!
Conclusion
After reading "Learning Ruby on Rails through Code Reviews," I pointed out three areas in my own web application! There was so much more to learn from this book beyond what I applied to my own project! I will keep these points in mind when writing code in the future.
Thank you for reading until the end!
Discussion