Hello Team,

In last week, I got the opportunity to review the latest code commits from one of our application. It was a very good experience for me to learn many new things from it.

For the following code snippets, I have also added few suggestion from my part, they may be relevant or not, as am not aware of what exact functionality is performed by those snippets.
As per my thinking, the developer who add the lines of code knows more about it and also knows how he can improve it more better than any other code-reviewer.

So please feel free to correct me if am wrong anywhere. Also would like to know the suggestions from you all.

Best things:

  • Each commit with test cases.
  • Refactored Code.
  • Proper Comments for each method.
  • Specs (use of webmock and stub) 
def move_activity_state new_task, date
  activity_states.where("occurrence_date = (?)", date).first.update_attributes(
    task_id: new_task.id,
    occurrence_time: new_task.occurrence_time)
end

This can be improved as:

def move_activity_state new_task, date
  get_record.update_attributes(
    task_id: new_task.id,
    occurrence_time: new_task.occurrence_time)
end

def get_record # add some proper name
  activity_states.where("occurrence_date = (?)", date).first
end
task.activity_states.incomplete.update_all occurrence_time: task.occurrence_time

This can be improved as:

get_incomplete_tasks.update_all occurrence_time: task.occurrence_time
def get_incomplete_tasks
  task.activity_states.incomplete
end
if activity_state.checklists_store.present?
  activity_state.completed = activity_state.checklist_complete? ? Time.now : nil
end

This can be improved as:

activity_state.completed = activity_checklist_completed? ? Time.now : nil
def activity_checklist_completed? # add some proper name
  activity_state.checklists_store.present? && activity_state.checklist_complete?
end

Improper Indentation

attr_accessible  :max_users, :overage_limit, :override,
                      :renew_subscription, :subdomain
attr_accessible  :max_users, :overage_limit, :override,
  :renew_subscription, :subdomain

Old ruby hash syntax

simple_form_for :import_contacts, :url => {:action => :list_twitter_contacts} do |f|
%a{ href: weburl =~ /^http:\/\// ? weburl : "http://#{weburl}", target: "_blank", class: "link"}

can be move to helper:

%a{ href: url_with_protocol(weburl), target: "_blank", class: "link"}
def url_with_protocol(weburl)
  weburl =~ /^http:\/\// ? weburl : "http://#{weburl}"
end

Little complex method, can be refactor more..

def get_website
  return unless web_url.present?
  web_url.length > STRING_LENGTH ? nil : (web_url.to_absolute_url rescue nil)
end
def get_website
  valid_web_url = web_url.blank? || web_url.length > STRING_LENGTH
  valid_web_url  ? nil : (web_url.to_absolute_url rescue nil)
end
def add_ex_user_contact_to_list
  if Contact.find_by_is_special_contact(true).nil?
    contact = Contact.new(first_name: 'ex_user', last_name: 'Contact',
                    company: 'Github', twitter_handle: "@ex_user",
                    is_special_contact: true )
    contact.save(validate: false)
  else
    contact = Contact.find_by_is_special_contact(true)
  end

  self.add_asset(contact)
end

This can be improved as:

def add_ex_user_contact_to_list
  contact = Contact.find_by_is_special_contact(true)
  if contact.nil?
    contact = build_special_ex_contact
    contact.save(validate: false)
  end
  self.add_asset(contact)
end
def build_special_ex_contact
  Contact.new(
    first_name: 'ex_user',
    last_name: 'Contact',
    company: 'Github',
    twitter_handle: "@ex_user",
    is_special_contact: true )
end
  else
    data = {flag: flag, user_contacts: model, subs_contacts: subs, max_contacts: max - model}
    data # <--not needed 
  end

(New thing for me): http://api.rubyonrails.org/classes/Object.html#method-i-presence(presence)

def start_date
  stringify_date(:start_date).presence || ''
end