Ruby on Rails design patterns and good practices

Question

I have developed a code for an admin page with crud in ruby on rails. But I have a doubt. Can this code be improved in any way? In therms of design patterns, good practices and etc.

Also, I did separate the crud actions from the AdminController Class. Is this considered a good practice at all? Or is there any way I can improve it even further? Thank you.

# app/controllers/game_controller.rb
class GameController < ApplicationController
  def create
    Game.create(game_params)
  end

  def update
    game = Game.find(params[:id])
    if (game.update_attributes(game_params))
      return true
    end
    return false
  end

  def read
    game = Game.find(params[:id])
  end

  def delete
    if (Game.find(params[:id]).destroy)
      return true
    else
      return false
    end
  end

  private

    def game_params
      params.require(:game).permit(:description,:name,:category,:status,:boxshot)
    end
end


# app/controllers/admin_controller.rb
class AdminController < ApplicationController
  before_action :require_logged_in_user
  before_action :define_action

  def index
    @games = Game.all
    @admin = get_admin_details()
  end

  def read
    render :json =>  @action.read
  end

  def update
    if ([email protected])
      render :text => "false"       
    end
    render :text => "true"
  end

  def delete
    if([email protected])
      render :text => "false"
    end
    render :text => "true"
  end

  def logout
    reset_session
    redirect_to(:controller => 'login', :action => 'view')
  end

  private 

    def define_action
      @action = GameController.new()
      @action.params = params
      I18n.locale = cookies['language']
    end

    def get_admin_details
      admin = User.find(require_logged_in_user())
    end

    def require_logged_in_user
      if(session[:user_id])
        return session[:user_id]
      else
        redirect_to(:controller => 'login', :action => 'view')
      end
    end  
end

After a quick first glance, the very first thing that jumps right out is that indentation and whitespace are a total mess and violate just about every Ruby style guide out there. — Jörg W Mittag
– Jörg W Mittag, Commented Apr 14, 2017 at 21:29
Yeah, i actually messed up when pasting it here. I'll edit it asap. — Lucas Meine
– Lucas Meine, Commented Apr 14, 2017 at 21:30
There are several things that can be improved/fixed. I recommend going first through this guide (github.com/bbatsov/rails-style-guide), update your code, and then come back for more feedback. — Gerry
– Gerry, Commented Apr 14, 2017 at 21:33

Kieran E · Accepted Answer · 2017-04-14 21:34:47Z

3

To be honest, there's quite a bit that can be improved. Here's my take on GameController

class GameController < ApplicationController
  before_action :find_game, only: [:update, :show, :delete]

  def new
    @game = Game.new
  end

  def create
    @game = Game.new(game_params)

    if @game.save
      redirect_to game_path(@game)
    else
      render :new
    end
  end

  def edit
  end

  def update    
    if @game.update(game_params)
      redirect_to game_path(@game)
    else
      render :edit
    end
  end

  def show
  end

  def delete
    if @game.destroy
      redirect_to games_path
    else
      render :show
    end
  end

  private

  def find_game
    @game = Game.find(params[:id])
  end

  def game_params
    params.require(:game).permit(:description, :name, :category, :status, :boxshot)
  end
end

I would take some time to learn the basics of Rails a little more thoroughly. Check out Railscasts. It's older, but still good info. Any premium episodes can be found on Youtube.

You can also look at style guides an use the advice they provide.

Whatever you do, the most important thing is to stay consistent. Maintain the same indent amount, the same class structure, the same assignment and conditional structure, etc.

To answer your second question, you want a new controller for each new CRUD action. Don't try and shoehorn in a ton of functionality into one controller. Keep it simple, lines are cheap.

answered Apr 14, 2017 at 21:34

Kieran E

3,6963 gold badges22 silver badges43 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Jörg W Mittag Over a year ago

The Rails mantra corresponding to the last paragraph is "Fat Model, Skinny Controller", although the current trend seems to move toward "Skinny Controller, Simple Model, many Services".

Collectives™ on Stack Overflow

Ruby on Rails design patterns and good practices

1 Answer 1

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related