1
\$\begingroup\$

I am pretty new to Rails and in fact this is the first thing I have made. This is a todo list type app. My JS and Rails is a big mess. I have just kinda hacked it up to work. Please suggest better ways to do things. Styling suggestions are welcome too.

class UsersController < ApplicationController
    def index
        @users = User.all
    end

    def create
        @newuser = User.new params[:user]
        if @newuser.save
          flash[:notice] = "Successfully created your account. You can now log in"
          redirect_to root_path
        else
            flash[:error]=@newuser.errors.full_messages;
          render :action => 'new'
        end

    end

    def show
        @user = User.find(params[:id])
        if session[:user_id] != @user.id
            flash[:notice] = "not authorised!!"
            redirect_to User.find(session[:user_id])
        end

        @tasks = @user.tasks
    end

    def add_task
        #render text: params.inspect
        Task.create(job:params[:task][:job],user_id:params[:id])
        flash[:notice]="Task has been added Successfully"
        redirect_to user_path(params[:id])
    end

    def authenticate
        #render text: params.inspect
        @user = User.where(:username => params[:username],
            password:params[:password]).first
        if @user.nil?
            flash[:notice]="Invalid Username and/or Password"
            redirect_to root_path
        else
            session[:user_id] =@user.id
            flash[:notice]="Welcome #{@user.username}!!"
            redirect_to @user
        end
    end

    def delete_task
        #render text: params.inspect
        Task.destroy(params[:task_id])
        flash[:notice]="Task was marked as done and hence deleted!!"
        redirect_to User.find(session[:user_id])

    end

    def home
        if session[:user_id]
            redirect_to User.find(session[:user_id])
        else
            flash[:notice]="Please sign in!"
            redirect_to root_path
        end
    end

    def signout
        session[:user_id]=nil
        redirect_to root_path
    end

    def updatetask
        @task = Task.find(params[:task_id])

        if session[:user_id] != @task.user_id
            flash[:notice] = "not authorised!!"
            redirect_to User.find(session[:user_id])
        else

            @task.planned = params[:planned]
            @task.started = params[:started]
            #@task.color = params[:color]
            @task.finished = params[:finished]
            @task.save
            redirect_to root_path
        end
    end

end

show.html.erb

<% if flash[:notice] %>
    <div id="notice"><%=flash[:notice]%></div>
<% end %>
<% if @tasks.any? %> 
<table border="1">
    <tr>
        <th class="task-column">Task</th>
        <th>Planned</th>
        <th>Started</th>
        <th>Finished</th>
        <th>Delete Task</th>
    </tr>


    <% @tasks.each do |t| %>
    <tr>
        <td class="task-column"><%= t.job %></td> 
        <td><%= check_box_tag "planned[#{t.id}]","#{t.id}", t.planned, :class => 'planned'%></td>
        <td><%= check_box_tag "started[#{t.id}]","#{t.id}",t.started, :class => 'started'%></td>
        <td><%= check_box_tag "finished[#{t.id}]","#{t.id}", t.finished, :class => 'finished' %></td>
        <td><%=link_to "Delete", delete_task_path(t)%></td>

    </tr>

    <%end%>
</table>

<div id="done-container"><div id="done"></div></div>
<%end%>
<% if @tasks.empty? %>
    <p>Please Add Some Tasks.</p>
<% end %>

jQuery

$(document).ready(function() {
    $("input:checkbox:checked.started").parent().css("background-color","yellow").siblings().css("background-color","yellow");
    $("input:checkbox:checked.finished").parent().css("background-color","green").siblings().css("background-color","green");

    function doneDisplay() { 
            var done_div =$("#done"); 
            var done = Math.floor(($(":checked").length)/($(":checkbox").length)*100);
            done_div.html( "you are " + (done||"0") +"% done!!").css("width",done+"%");
            //$(done_div.children()[0]).css("font-color","blue");
            if (done <= 33){
                done_div.css("background-color","red");
            }
            if (done > 33){
                done_div.css("background-color","yellow");
            }
            if (done > 66){
                done_div.css("background-color","green");
            }
            if (done == 0){
                done_div.css("width","100%").css("background-color","inherit");
            }


        }

    doneDisplay();

    $(":checkbox").change(function(){
        var $this = $(this);
        var color,finished,started,planned;
        var task_id = $this.attr("value");
        if (($this.attr("class") =="finished") && ($this.attr("checked")=="checked")){
            $this.parent().css("background-color","green").siblings().css("background-color","green")
            .children().attr("checked",true);
            //color ="green";
            finished = started = planned =true;
        }
        if ($this.attr("class") =="planned" && !$this.attr("checked")){
            $this.parent().css("background-color","red").siblings().css("background-color","red")
            .children().attr("checked",false);
            //color = "red";
            finished = started = planned =false;
        }
        if (($this.attr("class") =="started") && ($this.attr("checked")=="checked")){
            $this.parent().css("background-color","yellow").siblings().css("background-color","yellow")
            .children(".planned").attr("checked",true);
            //color = "yellow";
            started = planned =true;
            finished = false;

        }
        if ($this.attr("class") =="started" && !$this.attr("checked")){
            $this.parent().css("background-color","red").siblings().css("background-color","red")
            .children(".finished").attr("checked",false);
            //color = "red";
            planned =true;
            finished =started= false;
        }
        if ($this.attr("class") =="finished" && !$this.attr("checked")){
            $this.parent().css("background-color","yellow").siblings().css("background-color","yellow");
            //color = "yellow";
            started = planned =true;
            finished = false;
        }
        if (($this.attr("class") =="planned") && ($this.attr("checked")=="checked")){
            $this.parent().css("background-color","red").siblings().css("background-color","red")
            //color ="green";
            finished = started =false;
             planned =true;
        }






        $.ajax({
            type: "POST",
            url : "/updatetask/"+task_id+"/"+planned+"/"+started+"/"+finished,
            success: doneDisplay,
            //error: function(xhr){  alert("The error code is: "+xhr.statusText);}        
        });



  })
});
\$\endgroup\$
2
  • \$\begingroup\$ should i include the controller and views then ? Copying all the code doesn't seem a good idea. \$\endgroup\$ – tarashish Jun 27 '12 at 12:31
  • 2
    \$\begingroup\$ ‘( Some CSS advise too please)’ where is css? \$\endgroup\$ – Vladimir Starkov Jul 21 '12 at 18:11
2
\$\begingroup\$

A few quick suggestions, without really looking thoroughly into the code:

  • Saving any passwords in clear text is a Very Bad Idea™.
  • Why are you limiting the max-length of the password?
  • Do not set styles in JS, set classes instead and then style them with CSS.
  • Use $this.hasClass("finished") instead of $this.attr("class") =="finished".
  • You repeatedly access $this.attr("checked"), so it might be a good idea to store it in a var at the beginning of the change handler.
\$\endgroup\$
3
  • \$\begingroup\$ Thanks..I am really new to rails. So just needed to build a basic authentication system. I will look into encryption :) \$\endgroup\$ – tarashish Jun 27 '12 at 13:40
  • \$\begingroup\$ (I would wait a few days for other good input, before marking anything as answer.) Maybe it would be overwhelming to learn too much at once. But when you feel motivated, there are good resources to learn about it. I find railscasts to be a good source. \$\endgroup\$ – ANeves thinks SE is evil Jun 27 '12 at 13:48
  • \$\begingroup\$ ok then i take your suggestion and unmarking it . Btw I actually made this authentication thing by mixing up stuff i learned from Rails for zombies and one of the railscast.I am currently trying to make the layout better, once I am done I will clean up the js and make a better authentication system. :) \$\endgroup\$ – tarashish Jun 27 '12 at 13:58

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.