Idea for TimeClock Need Advice

I'm wanting to integrate Time Clock functionality into an existing Rails 3.2.21 app. I think I have an idea of how to do this, but I need some advice on the how to handle different scenarios.

So here's my thoughts on the models:

User
has_many :clock_events

ClockEvent
belongs_to :user
attr_accessible :clock_in, :clock_out, :explanation

The basic functionality in the controller and view, will let current_user clock in and clock out with an explanation in case they are early/late/didn't clock in by accident. I also need to figure out how to sum the clock_in and clock_out times/dates for each shift and then for a date range.

What I'm getting tripped up on is when a user logs in to clock in, the thought is to create a new clock_event and allow them to either clock_in or clock_out. Then perform some sort of validation that checks if they are clocking in and the last even was not a clock out, to give them an error and prevent them from skewing the clock_in/clock_out process. I'm pretty sure I can do some custom class method validations to make this work. But ultimately I'd like each time entry to have a clock_in and clock_out instead of iterating over a new instance every time they go to clock in/out. So basically if I am clocked out and go to clock in a new object will be created that allows me to clock in. I'm just trying to think of the best way to work with that current clock_event to ensure that both clock_in and clock_out are populated before allowing a new object to be created.

This is all a 30,000 FT view of the idea, but I could use some advice on how to build something simple that keeps track of clock_in/clock_out, allows me to sum/calculate the time between clock_in/clock_out, performs sum/calculation for all completed clock_event between a given date range.

It's early and I'm scatter brained, so I'm sure I'll need to flesh this out some more and start writing some sample code. But if anyone has any ideas on how to do this cleanly I'd appreciate the help. I wrote something like this 3 years ago when I was learning Rails but lost the repo so I totally forgot how I managed to do it back then. lol

I think it makes sense to have a ClockEvent that has both in and out events. It also sticks around so that you can remind people to clock out if it's past a set amount of time and they have an open ClockEvent because they forgot to clock out.

Calculations on that should be as simple as subtracting the out from the in times and you'll get your result in seconds which you can easily add up and convert to hours. It will also easily work across days and things like that.

current_clock_event Could just be the last ClockEvent for the user where(clock_out_at: nil) It would return the last incomplete one, or none at all so you could create a new ClockEvent instead.

I think you've got this pretty well thought out so far. The only thing will be handling the forgotten clock outs and how to remind users and set the actual time for it after the fact.

I agree that a ClockEvent should have a clock_in and clock_out value. And yes, you're right it's easy to calculate the time between two date objects especially with to_i then parsing the seconds into a readable time which I've already written a time parser for (HH:MM). One thing I think I should look at is using .round in the calculation to deal with the seconds as we pay off of hours/minutes worked so I'll probably want to round to shred the fragments. No one gets paid for 15 seconds :)

I think doing something like current_clock_event makes a lot of sense and by scoping it will ensure I can complete the time clock cycle of clocking in/out. Does it make more sense to architect that in the controller or the model? On the ClockEvent controller the index could be something like @clockevent = current_user.current_clock_event ||= ClockEvent.new.

The forgotten clock in/clock out is easy to do by writing a rake task looking for time ClockEvent every week and notifying HR/Payroll via a mailer of the entries so he/she can go back in through a view/controller and make simple crud updates.

My only question that I'm a bit fuzzy on is calculating the total number of hours between two given dates. Calculating the difference in hours/sum for each day is easy but I'm having a brainfog moment with calculating the sum of total number of hours between date ranges. Maybe you can tip the hat on how to do that?

I was thinking to calculate the hours I'd actually store it as a value in the current_clock_event. Through a callback or something. So like an after_save model call back which will calculate the diff in integer, convert it to a string, and post it in a field in the ClockEvent called total_hours. This can be updated when the HR person goes back in and edits/makes corrections.

In reality after reading your post and thinking about it most of the day this should be pretty simple. Just haven't worked with this sort of thing in a while so I needed to think it through and get some feedback from my favorite community <3

I think current_clock_event probably makes sense in the controller because you might need it for rendering in the views to remind the user it's not finished. Like you said, you'll need to be able to reference it from the model for proper scoping, so you may technically need it in both places if you add it to the model first and then make a controller method to make it easier to access.

It make sense to cache the value of total_hours on the model after save so that you've always got a queryable value. Would definitely recommend that.

I think having it available in both the controller and model will be necessary. I'll work on getting that together, shouldn't be an issue. I like the idea of caching the total_hours value. This is good stuff. :) I'll start stubbing all this out and will post up questions if I have any.

Cheers!

Ok, so I'm stubbing through this and trying to make sense of things. So here is the scope I'm looking to build for current_clock_event

```class TimeEvent
belongs_to :user
scope :current_clock_event, last.where(clock_out: nil)
end

Doing this I'm trying to fetch the last record for the user where `clock_out` is nil.  The problem is AR won't let me pull the last record and chain a where clause to the `.last` method.  

What would be the best way to fetch/scope the record looking for the last record where `clock_out: nil`?

Also in the controller, I'm trying to figure out the cleanest way to do the following.
``` class ClockEvents
def index
  @clock_event = current_user.current_clock_event #grab scope from model or create new ClockEvent?  Need guidance on this.
end
end

So basically what I want to do in the controller is to fetch the current_clock_event if it exists (such as last record for the user with a clock_out of nil) and if the ClockEvent is complete instantiate a new event to be passed into the view/form. What would be the best way to do this in the controller to ensure that I'm either completing the last ClockEvent or if completed to a ClockEven.new?

I really shouldn't have nuked my repo that had this code 2 years ago. I had this all figured out a while back but nuked it since I didn't think I'd ever need it again.

Thanks in advance and let me know if you need more detail. I'm open to whatever design patterns you have in mind, just looking to make it clean and work out of the gate. I'll get to model validations and call backs in a bit.

Update, too early to be doing this but my scope was botched, this should fetch the last record with clock_out: nil

scope :current_clock_event, where(clock_out: nil).last

Still need to figure out a conditional of current_clock_event or setting up a ClockEvent.new depending on if the current_clock_event exists or not.

What about something like this?

class ClockEvent

  belongs_to :user

  # scope the last_clock_event for a user
  scope :last_clock_event, -> { where("clock_out NULL").last }

#assure the clockevent is completed
  def completed?
    clock_in.present? && clock_out.present?
  end

end

class User

  has_many :clock_events

#Method to check if last_clock_event is completed and if so instantiate a new event or if not load the last_clock_event
  def current_clock_event
    ce = clock_events.last_clock_event
    ce.completed? ? ClockEvent.new : ce
  end

end

class ClockEventsController < ApplicationController

#Instantiate either last_clock_event or ClockEvent.new to be passed into the form
  def index
    @clock_event = current_user.current_clock_event
    render :index
  end

end

Note, this is a work in progress and I'm looking for peer review.

Looks pretty good, but here's what I'd refactor:

class ClockEvent
  belongs_to :user

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

class User
  has_many :clock_events

  def clock_event
    @clock_event ||= clock_events.incomplete.last || clock_events.new
  end
end

Some changes:

1. The scopes on the clock event make them useful for many other use cases, not just this one. Also I don't think you can use .last in the scope like you had, you'd want a has_one instead if you really wanted that or to use the last call attached to the scope like in my example's clock_event

2. User now has clock_event which simply returns their current event. First it looks up any incomplete ones and grabs the latest one and if that returns nil it will create a new associated ClockEvent in memory. I cache that to a variable since you'll probably reference it a few times in your views and this will be important so you either don't query the record many times or create a bunch of new ones in memory each time you access the method.

3. The completed? method isn't required for this anymore either because we're using the nil return value in the statement to trigger the || clock_events.new which is nice. Less code ftw, but you'll probably want completed? for rendering things in the view or logic elsewhere still.\

Hope that gives you some ideas! Looks great so far too.

Hey Chris,

I really appreciate your review and critiques. The refactor makes a lot of sense and is way cleaner and I like the idea of caching the the variable as that will definitely take some hits from the db and get rid of them. I'm still stubbing this all out, so I'll take your suggestions and refactor the models and the controller and post up either a gist or code snippets today to see what you think. I'm hoping to have the basic functionality working in the next day or so. This was more of a weekend project so I didn't really have time to start building, just writing stubs and seeing how it might work.

Would you suggest I work off another branch and test all this or simply write a new tiny app to test the functionality then port it over into my main legacy app? I'm doing my best not to work off of master in case something needs to be pushed quickly.

If you're building this into an existing app, a branch makes sense. If you need to quickly prototype it without trying to fit it into the legacy code, it might make sense to try out a new app, but you'll still have to integrate it with the legacy code at some point. I'd probably go with the feature branch.

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)

Yeah refactorings are always really painful like that because you can't merge in the main application ever when it updates around this same code. This is where testing really shows its value, but versioning doesn't have a good way to handle this yet.

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.

You gotta start somewhere. It's like art, just start painting. You can't plan a masterpiece, you've got to feel it out as you go (which is my main issue with TDD).

Last night I rewatched these two episodes and they gave me a lot of ideas on maintainable code:

https://www.youtube.com/watch?v=yhseQP52yIY
https://www.youtube.com/watch?v=mWo3oEwFFzM

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.

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?

I don't think this needs refactoring. It's straightforward and unless it gets more confusing, you won't get any benefit out of changing it. The only suggestion I'd make is to use an else, because you don't need two separate if statements.

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

You could move this logic into a helper, but that's up to you. That will only just obscure what's going on a bit and not give you any more clarity when reading this again 3 months from now.

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?

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?