shakycode

I had to chime in on this too. I agree on blacklisting the word forum. It's an easy fix and something I just learned about after implementing friendly_id.

You'll want to open config/initializers/friendly_id.rb and add your namespace to the array shown below:

  config.reserved_words = %w(new edit index session login logout users admin
    stylesheets assets javascripts images forum)

This should take care of the issue for you. But also make sure to place your route in the proper order as Chris mentioned.

Cheers!

Wow, Chris is really becoming a rockstar! Good job!

I had no idea about devise_invitable. This is awesome. In the past I removed :registerable and created a match route to 404 signup requests like so:

match 'users/sign_up' => redirect('/404.html')

Hackish but it works. But I think Chris' suggestion is better.

Lastly, when I sum the total of @clock_events like so and if I'm in the middle of a clock cycle (clock_in with not clock_out) I get the following crazy negative number (see below)

<%= TimeFormatter.format_time(@clock_events.sum(&:total_hours)) %>

So using my current methodology, how would I avoid that sum displaying skewed in the midst of a clock cycle?

Thanks for the tip. I think this is as about as clean as it's going to get now using and else clause. Note you have a small syntax error <%= else %> :)

This works well and flows better instead of using two if statements. :)

I'm probably going to have more questions about this feature moving forward. One thing I've done is creating a method to calculate total hours on the fly. I'd like your feedback on this.

class ClockEvent < ActiveRecord::Base
  attr_accessible :clock_in, :clock_out, :user_id

  belongs_to :user
  scope :incomplete, -> { where(clock_out: nil) }
  scope :complete, -> { where.not(clock_out: nil) }

  def punch_in
    self.clock_in = Time.zone.now
  end

  def punch_out
    self.clock_out = Time.zone.now
  end

  def completed?
    clock_in.present? && clock_out.present?
  end

  def total_hours
    self.clock_out.to_i - self.clock_in.to_i
  end
end

The class method, total_hours takes the difference between the datetime fields clock_in and clock_out and to integer to give a result. This works well in theory then to display the time in HH:MM I have this as an initializer:

class TimeFormatter
  def self.format_time(timeElapsed) 
    seconds = timeElapsed % 60
    minutes = (timeElapsed / 60) % 60
    hours = (timeElapsed / 3600)

    "%d:%02d:%02d" % [hours, minutes,seconds]
  end
end

Then in the view I do this to show the total hours for each clock cycle and then sum the total for the entire series of clock entries.

<% @clock_events.each do |ce| %>
    Clock In: <%= ce.clock_in.try(:strftime, "%m/%d/%y-%H:%M") %> Clock Out: <%= ce.clock_out.try(:strftime, "%m/%d/%y-%H:%M") %> Total: 
    <% if ce.completed? %>
        <%=  TimeFormatter.format_time(ce.total_hours) %>
    <% end %>
<% end %>

<%= TimeFormatter.format_time(@clock_events.sum(&:total_hours)) %>

This "works" as you can see

But for some reason calculating total_hours on the fly gives me a couple of concerns that I need to work around.

• I don't know of a way via Ruby natively to calculate the difference of time in hours/minutes so I have to use the to_i method it convert it to an integer, then subtract the two values to get the difference and then throw it into the TimeFormatter class to make it pretty. So there's really no total field that gets saved with a callback. This leaves an edge case open that if somehow the clock_in or clock_out does not get populated (like a user hits the back button and the screen is cached they could create an incomplete punch or one that's out of whack and it may throw NIL. Which is why i use the completed? method to avoid NIL. I mean this works, but I feel it could be better.
• When a manager goes to edit times to fix mistake, what if she wants to edit the total hours and override? Would it be better to have some sort of column for total that she can edit (and if so, it's going to be an integer and she'll have no idea what she's doing) or would it be better just to allow her to edit the actual clock_in and clock_out attributes for each user to make the corrections and let the total_hours method recalculate?

Those are my concerns so far. Again this is all pretty simple stuff but I'm trying to think outside the box and avoid NIL while giving a good UX to the user and/or manager.

Thoughts?

Been playing with this a bit today (had to take a break and work on other stuff). Got pretty much everything working with a mix of my original code and your refactoring. I also figured out a way to prevent screwy punches in the view like so

<% if  !@clock_event.clock_in? %>
<%= link_to "Clock In", clock_in_employees_path(@clock_event), :method => :put %>
<% end %>
<% if @clock_event.clock_in? %>
<%= link_to "Clock Out", clock_out_employees_path(@clock_event), :method => :put %>
<% end %>

So the Clock In link will only show up if clock_in is empty and the Clock Out link will only show up if the clock_in value is present. Doing this is a bit dirty, but it prevents screwy and missing time punches.

This is all prototype and needs to be fleshed out, was wondering if you can suggest a better refactor on this view logic?

For sure! And Sandi Metz is a really good person to have looking over your shoulder. She's the queen of abstraction (among others).

I can tell you from my experience that I don't always abstract. When a class becomes > 200 LOC that's typically a sign for me to abstract. Personally, I try to put as many methods into a Model/Class that I can. In the beginning my controllers were out of control with some really complex methods. But after Chris showed me some basic refactoring and abstraction I learned to start pulling logic and complicated methods out of the controller and into classes or modules. It's made my life much easier.

There is no "hard and fast" rule on when to abstract. Really it's a gut feeling more than anything. If you think that small objects are fine in one class, then stick with it. If you feel that it's going to be hard to maintain, then abstract it. There's really not a right or wrong answer here, just general guidelines.

As you progress more you'll start to sniff the "code smells" and will figure out when it's best to separate. One thing that helps me is looking at older code I wrote and figuring out how I could abstract better. It helps when refactoring old code and makes me a better developer moving forward.

You got this! :P

Whenever you can, try to rely on abstraction. Separation of concerns and responsibilities is a good pattern to follow. Note, you can go a little crazy with this, but in general good abstraction is what you want to keep maintainable code.

I personally try to stay away from STI whenever possible. I haven't really had a use case for it besides being able to manage things easier from the db point. Like Chris said, it's whatever you feel is easiest and what you feel is maintainable in the long run.

Great post! And very true. Lived it....

No, I know that. I tried just patient_bill? and it also threw a nomethoderror. I was just trying it with the form builder as well. The object exists in memory when creating a new record or editing.

Once I nail this down, I'm going to move this into a helper I think. Just got to figure out the logic and such.

I see what you're saying about the logic and it makes sense. I tried the following:

<div id="billing_address" <% if !f.patient_bill? %>style="display: none"<% end %> >
            <%= f.label :Billing_Address, "Billing Address", class: "control-label" %>
            <%= f.text_field :billing_address, :placeholder => '555 W 8th St. Houston, TX 77448', class: "input-large" %></br>
</div>

And in doing that I get the following exception raised:
undefined methodpatient_bill?' for #ActionView::Helpers::FormBuilder:0x007fa7e4d24388`

Ok, so I've tried both. Removing the style reversed the behavior of the checkbox being checked. So without the style in the HTML it will display the billing_address field. When I check the patient_bill checkbox the billing address field disappears.

So on to step #2
Using the above code results in the div being displayed if the box is checked when editing a record, which is great. But when creating a new record checking the check box hides the div instead of displaying it.

I have some simple CoffeeScript that toggles a div based off of if a checkbox is checked in a Rails form. The CoffeeScript looks like this:

  $("#bill_patient_checkbox").change ->
    $("#billing_address").toggle()

In the form I have a div setup to default to display:none

<%= f.check_box :patient_bill, :id => "bill_patient_checkbox" %>

          <div id="billing_address" style="display:none">
            <%= f.label :Billing_Address, "Billing Address", class: "control-label" %>
            <%= f.text_field :billing_address, :placeholder => '555 W 8th St. Houston, TX 77448', class: "input-large" %></br>
          </div>

This works fine when creating a new record and clicking the checkbox displays the div with the form field and if unchecked hides it. The problem I'm having is when editing a record the checkbox will remain checked but the div is hidden. The only way to change it is to uncheck the checkbox, the div appears, then you can edit the information, and re-check the box.

So what I'm trying to figure out is how to expand my CoffeeScript to say if the checkbox is checked always display the div in addition the toggle functionality.

Please let me know if you have any questions or need more example code.

I think I have this figured out. Definitely not awake enough to be writing code.

I had this in my form before the BillFacility collection_select

<%= f.label :Bill_Facility, "Bill Facility", class: "control-label" %><%= f.check_box :bill_facility, :id => 'bill_facility_checkbox' %>

So I was calling a field named bill_facility that was in my database, but since bill_facility is an associated model, it gave me the mismatch error. So I simple altered the migration to change the check_box to bill_fac and it worked.

I have a Rails 3.2.21 app where I'm adding a simple form/collection_select field in my form. When selecting an object from the dropdown (or leaving it blank) I get the following error:

ActiveRecord::AssociationTypeMismatch at /calls/5
BillFacility(#70179530126460) expected, got String(#70179505820040)

Here's what my models look like:

call.rb

    belongs_to :bill_facility
    attr_accessible :bill_facility_id

bill_facility.rb

    has_many :calls

Here's what my form looks like:

_form.html.erb

    <%= f.collection_select(:bill_facility_id, BillFacility.order("facility_name ASC"), :id, :facility_name, {:include_blank => true}, {:class => 'select'}) %>

Here's the migrations I did to add the BillFacility model and add the bill_facility_id to the Call model:

class CreateBillFacilities < ActiveRecord::Migration
  def change
    create_table :bill_facilities do |t|
      t.string :facility_name
      t.string :facility_address

      t.timestamps
    end
  end
end

class AddBillFaciltyIdToCalls < ActiveRecord::Migration
  def change
    add_column :calls, :bill_facility_id, :integer
  end
end

If I manually assign the call an id in bill_facility_id I get an unknown to_i method error. If I manually make it nil then select a BillFacilty from the drop down (or leave it blank) I get the mismatch error:

`ActiveRecord::AssociationTypeMismatch at /calls/5
BillFacility(#70179530126460) expected, got String(#70179505820040)`

I'm sure this is something simple that I'm missing. Anyone have any ideas on what I'm doing wrong here? It's a bit early so my mind is kind of fuzzy so I should probably wait until I'm fully awake but figured I'd ask for some help.

If you have any questions, please let me know.

That's the same issue I have with TDD. I've tried it several times and I feel "constrained" when passing then refactoring. It feels so much more natural to refactor without TDD. So I have a different way of approaching it. I write code, refactor, write a test, pass it, then look for areas of improvement. It's kind of backwards from the typical red-green-refactor but it works ok for me so far.

Thanks for the links, I'm going to watch these today. Basecamp does it right so I'm one to listen and learn from their examples.

I see where you're coming from. I'm going to try NOT and touch master as this code will be segregated for the most part minus the association between User and ClockEvent. One thing I'm trying to do as I develop is look 10 steps ahead to see how code/features/etc scale and if their maintainable. I used to have a problem with writing code that was not very maintainable and was "shot from the hip" often times. Now I try to look a year ahead of time and make sure it scales and will be easy to maintain. Bottom line, I'm loving refactoring. There's so many good patterns to follow and anti-patterns to avoid.

I think a feature branch will do the trick. I'm just going to try my best to stay out of master so it doesn't get too far ahead of the branch otherwise it's going to be "merge fun". I may prototype in a separate app just for fun but it definitely makes more sense to do a feature branch. (thumbs up)