To-do-list web app in Ruby on Rails and CSS

Question

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][email protected]_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] [email protected]
 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);} 
 });
 })
});

• tarashish: should i include the controller and views then ? Copying all the code doesn't seem a good idea.
• Vladimir Starkov: ‘( Some CSS advise too please)’ where is css?

Answer 1

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

• Saving any passwords in clear text is a Very Bad IdeaTM.
• 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.

• tarashish: Thanks..I am really new to rails. So just needed to build a basic authentication system. I will look into encryption :)
• ANeves: (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.
• tarashish: 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. :)