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

  attr_reader :params

  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 WeeklyFlexCalcul

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.

StackExchange Code Review Q#35223, answer score: 5