4
\$\begingroup\$

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 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
\$\endgroup\$
  • 2
  • \$\begingroup\$ Don't suggested edits need to be approved by the OP anyway? In that case it's completely under their control, or is it? \$\endgroup\$ – CMW Nov 13 '13 at 9:33
  • 2
    \$\begingroup\$ @CMW that isn't how it works. other people approve the suggested edits by vote. \$\endgroup\$ – Malachi Nov 13 '13 at 13:47
  • \$\begingroup\$ @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. \$\endgroup\$ – CMW Nov 13 '13 at 13:51
  • 1
    \$\begingroup\$ @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 \$\endgroup\$ – Malachi Nov 13 '13 at 14:17
5
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.