# Weekly flex calculator

This is one of the first RSpec tests I've written, and I'm not really sure what the best practices are etc.

My question basically is: what would you do differently? What is good? What is bad?

The complete code can be found here.

Here is the class to be tested:

class WeeklyFlexCalculator

def initialize(params)
@params = params
end

def calculate
group_efforts(
(params.start_date..params.end_date).map do |date|
daily_effort(date)
end.compact
)
end

def group_efforts(result)
weekly = result.group_by { |e| get_week_key(e[:date]) }
weekly.map do |key,w|
{
year: get_year_from_week_key(key),
week: get_week_from_week_key(key),
weekTarget: get_target_sum(w),
weekEffort: get_effort_sum(w),
efforts: w
}
end.sort { |a, b| b.efforts[0].date <=> a.efforts[0].date }
end

def daily_effort(date)
target = get_target(date)
effort = get_effort(date)
return if target == 0 && effort == 0
{
date: date,
effort: effort,
target: target,
diff: effort - target
}
end

def get_target_sum(efforts)
efforts.inject(0){|sum,e| sum + e[:target]}
end

def get_effort_sum(efforts)
efforts.inject(0){|sum,e| sum + e[:effort]}
end

def get_week_key(date)
date.cwyear.to_s + "|" + date.cweek.to_s
end

def get_year_from_week_key(key)
key.split('|')[0]
end

def get_week_from_week_key(key)
key.split('|')[1]
end

def get_target(date)
day_off?(date) ? 0 : params.user.hours_per_day
end

def get_effort(date)
ts = get_timesheet(date)
ts.nil? ? 0.0 : ts.TimeInHours
end

def day_off?(date)
date.wday == 0 or date.wday == 6 or params.holidays.include? date.to_s
end

def get_timesheet(date)
params.timesheets.select {|ts| ts.Date == date.to_s}.first
end
end


And here is the test:

require './WeeklyFlexCalculator'

describe WeeklyFlexCalculator, "during Christmas week" do

subject(:calculation) { WeeklyFlexCalculator.new(params).calculate }

let(:params) do
messages = {
:start_date => Date.new(2013,12,23),
:end_date => Date.new(2013,12,29),
:holidays => ["2013-12-24", "2013-12-25", "2013-12-26"],
:timesheets => [],
:user => user
}
double(:params,messages)
end

let(:user) do
messages = {
:hours_per_day => 7.5
}
double(:user, messages)
end

let(:timesheet) do
messages = {
:Date => Date.new(2013,12,23).to_s,
:TimeInHours => 5.0
}
double(:timesheet, messages)
end

context "with no work performed" do
it { should have(1).item }

context "the week calculated" do
subject(:workweek) {calculation[0]}

its([:year]) { should eq "2013" }
its([:week]) { should eq "52" }
its([:weekTarget]) { should eq 15.0 }
its([:weekEffort]) { should eq 0.0 }

context "the work efforts" do
subject(:efforts) {workweek[:efforts]}

it { should have(2).items }

context "the first work effort" do
subject(:effort) {efforts[0]}

its([:target]) {should eq 7.5}
its([:diff]) {should eq -7.5}
its([:effort]) {should eq 0.0}
end
end
end
end

context "with work effort on normal day" do
before do
params.stub(:timesheets => [timesheet])
end

it { should have(1).item }

context "the week calculated" do
subject(:workweek) {calculation[0]}

its([:year]) { should eq "2013" }
its([:week]) { should eq "52" }
its([:weekTarget]) { should eq 15.0 }
its([:weekEffort]) { should eq 5.0 }

context "the work efforts" do
subject(:efforts) {workweek[:efforts]}

it { should have(2).items }

context "the first work effort" do
subject(:effort) {efforts[0]}

its ([:effort]) { should eq 5.0 }
its ([:diff]) { should eq -2.5 }
end
end
end
end

context "with work effort on a holiday" do
before do
timesheet.stub(:Date => Date.new(2013,12,24).to_s)
params.stub(:timesheets => [timesheet])
end

it { should have(1).item }

context "the week calculated" do
subject(:workweek) {calculation[0]}

its([:year]) { should eq "2013" }
its([:week]) { should eq "52" }
its([:weekTarget]) { should eq 15.0 }
its([:weekEffort]) { should eq 5.0 }

context "the work efforts" do
subject(:efforts) {workweek[:efforts]}

it { should have(3).items }

context "the first work effort" do
subject(:effort) {efforts[0]}

its ([:effort]) { should eq 0.0 }
its ([:diff]) { should eq -7.5 }
end

context "the second work effort" do
subject(:effort) {efforts[1]}

its ([:effort]) {should eq 5.0}
its ([:diff]) { should eq 5.0}
end
end
end
end
end

• – svick Nov 12 '13 at 22:17
• Don't suggested edits need to be approved by the OP anyway? In that case it's completely under their control, or is it? – CMW Nov 13 '13 at 9:33
• @CMW that isn't how it works. other people approve the suggested edits by vote. – Malachi Nov 13 '13 at 13:47
• @Malachi, svick My apologies then, I didn't have that case yet of the community approving any edits on my questions. Only did so myself. – CMW Nov 13 '13 at 13:51
• @CMW : the edits are always sent to the review queue but that is bypassed if you accept the edit, so it is whatever happens first, vote or OP approval – Malachi Nov 13 '13 at 14:17

# Spec

## The Good

There's not much here that needs changing.

• You only check one condition per it or specify
• Good use of let, subject, before, etc. to keep the test DRY and simple.
• Appropriate use of #stub for methods which are referentially transparent (when you say #stub, you are telling the reader of the test, "We don't care how many times this gets called, because it has no side-effects."

## Can be improved

There are some minor inconsistencies in indentation (see #daily_effort).

Some instances of context might be better as describe instead. Use context to indicate a precondition; describe to indicate a class, method, or behavior being tested.

describe SomeClass

context 'when things are setup a certain way' do

describe 'the aspect of its behavior being checked' do
its(:metasyntacticvariable) {should eq :foo}
its(:meaning) {should eq 42}
end

describe 'a different aspect of its behavior being checked' do
its(:shoe_size) {should eq 7}
its(:hair_color) {should eq 'brown'}
end

end

context 'when things are setup a different way' do
# etc
end

end


Here are a few of the context calls I would consider changing to describe:

context "the work efforts"
context "the first work effort"
context "the second work effort"


There's a little bit of missing test coverage:

• In #get_timesheet, the .first can be changed to .last and the test still passes.
• In #group_efforts, the entire .sort {...} can be removed without the test noticing.

# The code under test

## The good

There's a little more here that could be better, but overall the class has a lot going for it:

• Small, concrete methods
• Immutable
• Good names

## Could be improved

In #group_efforts, the sort can probably be changed from:

sort { |a, b| b.efforts[0].date <=> a.efforts[0].date


to:

sort_by { |e| e.efforts.first.date }


I say probably because the sort has no test coverage.

There's much that could be improved if attribute access were via accessor methods rather than by [], but that goes outside the scope of this class and imposes changes on its collaborators. For example, if an effort's target could be accessed by effort.target instead of effort[:target], then this:

efforts.inject(0){|sum,e| sum + e[:target]}


could be changed to:

efforts.map(&:target).inject(0, :+)


The pipe separator "|" appears repeatedly. It deserves a constant.

These methods:

  def get_year_from_week_key(key)
key.split('|')[0]
end

def get_week_from_week_key(key)
key.split('|')[1]
end


are a little damp. Consider:

  def get_year_from_week_key(key)
split_key(key)[0]
end

def get_week_from_week_key(key)
split_key(key)[1]
end

def split_key(key)
key.split('|')
end


In #day_off?"

date.wday == 0 or date.wday == 6 or params.holidays.include? date.to_s


Replace the or with ||:

date.wday == 0 || date.wday == 6 || params.holidays.include?(date.to_s)                              |


or (and and) have odd (and often surprising) precedence rules. They are not interchangeable with && and ||, a common source of bugs, and are seldom used in practice.

• What about using rspec 2.0 expect vs should? github.com/rspec/rspec-expectations/blob/master/Should.md – Fred the Magic Wonder Dog Jan 7 '14 at 23:11
• @Fred, good point. Note that it's not the method should that's deprecated, it's the use of the globally defined should. The use of should in the form 'its(:foo) {should eq 1}` is not deprecated. – Wayne Conrad Jan 7 '14 at 23:33