1
\$\begingroup\$

I would definitely love some feedback on this message board app I made. I been doing front end development for the most part and I just want to keep my backend chops up. Let me know anywhere that can be improved.

Here's the github and the heroku

PostController

class PostsController < ApplicationController
  before_action :authenticate_user!
  before_action :set_post, only: [:show, :edit, :update, :destroy]

  # GET /posts
  # GET /posts.json
  def index
    @posts = Post.page(params[:page]).per(5)
  end

  # GET /posts/1
  # GET /posts/1.json
  def show
  end

  # GET /posts/new
  def new
    @post = Post.new
  end

  # GET /posts/1/edit
  def edit
  end

  # POST /posts
  # POST /posts.json
  def create
    @post = Post.new(post_params)
    @post.author_id = current_user.id

    respond_to do |format|
      if @post.save
        format.html { redirect_to @post, notice: 'Post was successfully created.' }
        format.json { render action: 'show', status: :created, location: @post }
      else
        format.html { render action: 'new' }
        format.json { render json: @post.errors, status: :unprocessable_entity }
      end
    end
  end

  # PATCH/PUT /posts/1
  # PATCH/PUT /posts/1.json
  def update
    respond_to do |format|
      if @post.update(post_params)
        format.html { redirect_to @post, notice: 'Post was successfully updated.' }
        format.json { head :no_content }
      else
        format.html { render action: 'edit' }
        format.json { render json: @post.errors, status: :unprocessable_entity }
      end
    end
  end

  # DELETE /posts/1
  # DELETE /posts/1.json
  def destroy
    @post.destroy
    respond_to do |format|
      format.html { redirect_to posts_url }
      format.json { head :no_content }
    end
  end

  private
    # Use callbacks to share common setup or constraints between actions.
    def set_post
      @post = Post.find(params[:id])
    end

    # Never trust parameters from the scary internet, only allow the white list through.
    def post_params
      params.require(:post).permit(:author_id, :body, :title)
    end
end

Post model

class Post < ActiveRecord::Base
  validates :author_id, presence: true

  belongs_to :author,
             class_name: "User",
             foreign_key: :author_id,
             primary_key: :id

  has_many :comments, class_name: "Comment"
  has_many :commenters, through: :comments, source: :user

  default_scope { order("created_at DESC") }
end

CommentsController

class CommentsController < ApplicationController
  before_action :authenticate_user!
  before_filter :load_post
  before_action :set_comment, only: [:show, :edit, :update, :destroy]

  # GET /comments
  # GET /comments.json
  def index
    @comments = @post.comments.all
  end

  # GET /comments/1
  # GET /comments/1.json
  def show
    @comment = @post.comments.find(params[:id])
  end

  # GET /comments/new
  def new
    @comment = @post.comments.new
  end

  # GET /comments/1/edit
  def edit
    @comment = @post.comments.find(params[:id])
  end

  # POST /comments
  # POST /comments.json
  def create
    @comment = @post.comments.new(comment_params)
    @comment.author_id = current_user.id
    @comment.post_id = @post.id

    respond_to do |format|
      if @comment.save
        format.html { redirect_to @post, notice: 'Comment was successfully created.' }
        format.json { render action: 'show', status: :created, location: @post }
      else
        format.html { render action: 'new' }
        format.json { render json: @comment.errors, status: :unprocessable_entity }
      end
    end
  end

  # PATCH/PUT /comments/1
  # PATCH/PUT /comments/1.json
  def update
    @comment = @post.comments.find(params[:id])
    respond_to do |format|
      if @comment.update(comment_params)
        format.html { redirect_to @post, notice: 'Comment was successfully updated.' }
        format.json { head :no_content }
      else
        format.html { render action: 'edit' }
        format.json { render json: @comment.errors, status: :unprocessable_entity }
      end
    end
  end

  # DELETE /comments/1
  # DELETE /comments/1.json
  def destroy
    @comment = @post.comments.find(params[:id])
    @comment.destroy
    respond_to do |format|
      format.html { redirect_to @post }
      format.json { head :no_content }
    end
  end

  private
    # Use callbacks to share common setup or constraints between actions.
    def load_post
      @post = Post.find(params[:post_id])
    end
    def set_comment
      @comment = Comment.find(params[:id])
    end

    # Never trust parameters from the scary internet, only allow the white list through.
    def comment_params
      params.require(:comment).permit(:author_id, :post_id, :body, :title)
    end
end

Comment model

class Comment < ActiveRecord::Base
  validates :post, :body, presence: true

  belongs_to :post,
             class_name: "Post",
             foreign_key: :post_id,
             primary_key: :id

  belongs_to :author,
             class_name: "User",
             foreign_key: :author_id,
             primary_key: :id

  default_scope { order("created_at DESC") }
end

User model

class User < ActiveRecord::Base
  # Include default devise modules. Others available are:
  # :confirmable, :lockable, :timeoutable and :omniauthable
  devise :database_authenticatable, :registerable,
         :recoverable, :rememberable, :trackable, :validatable

  has_many :posts, class_name: "Post", foreign_key: :author_id, primary_key: :id
  has_many :comments
  has_many :commented_posts, through: :comments, source: :post
end
\$\endgroup\$
0
\$\begingroup\$

I hope it's the kind of feedback you are looking for, don't take them too personally (there are no big things to say about your code).

for the code itself

  • not sure I would have defaulted the scope with "order by" (just a named scope, it's harder to change its mind once in the default_scope)
  • don't commit your secret in git (see brakeman tool warnings)
  • don't think you need the class_name and primary_key options in the belongs_to :post
  • I would use latest stable rails version (5.0.x)
  • I don't know if a post is supposed to editable by its author ? Then I would use current_user.posts.find(params[:id]) instead of Post.find(params[:id]) (see the pundit gem for more advanced authorization scheme)
  • I would use the shorter hash syntax in erb and be consistent : eg : , method: :delete, :class => 'navbar-link' vs method: :delete, class: 'navbar-link'
  • I would not commit DS_store files
  • I would add Foreign key constraint in the db (see foreigner or rails 5)
  • I'm not a big fan of before action and so on in controllers
  • I would add paging to all your index methods (see comments controller)

  • When you write code like this @comment = @post.comments.new(comment_params) @comment.author_id = current_user.id @comment.post_id = @post.id I would minimize the @comment access from the controller by merging the author_id in comment_params in one line and call build (not new) on the comments association. for the post_id you are probably missing the "inverse_of:" options

for your toolbox, setup

  • rubocop (pick some .rubocop.yml from a project you like the code style)
  • brakeman
  • bundler audit
  • simplecov
  • continuous inspection like codeclimate, pullreview, or pronto
  • continuous integration tool like travis-ci, codeship,...

purist would tell you to

  • use haml or slim instead of erb
  • use decent_exposure instead of instance variables to expose model between controllers and views
  • don't clutter your views with bootstrap classes
  • organize your code with service object or use case classes
  • use presenters instead of rails helpers
  • use reform to decouple persistent models and form objects

but make your mind on the kind of code you like and pick what you want

\$\endgroup\$
  • \$\begingroup\$ This is incredible feedback! Thanks @mestachs for all the detail and thought! I am definitely going to go and add these changes. Much appreciated. \$\endgroup\$ – catch22 Apr 10 '17 at 20:05

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.