6
\$\begingroup\$

I have a simple Rails app, which is used to run some clinical surveys. Participants answer sets of questions (multiple-choice, valued 1-5), and, within each set, the answers are summed up and the resulting score placed into to certain score brackets, which are different for each set of questions, and for each participant's age group.

The brackets are based on empirical research, so there's no way to derive the relevant bracket from the score itself; the brackets are essentially an opaque lookup table.

In hard-coded Ruby, one could write it like so (not actual values:

def find_bracket(age, score)
  case age
  # minimum age is 18
  when 18...25
    case score
    # minimum score is 10
    when 10...15 then "bracket A"
    when 16...28 then "bracket B"
    when 29...37 then "bracket C"
    else "bracket D" # score of 38 or more
    end
  when 26...40
     # ... another nested case switch ...
  else # 41 or older
    # ...
  end
end

The bracket names are the same ("bracket A" to "bracket D" in this faked example) for each age range, but the score ranges they cover are different.

Since I really don't want to hardcode all that, I've implemented it as a serialized score_brackets attribute on the QuestionSet model like so:

question_set_a.score_brackets #=> [
  {
    min_age: 18,
    min_scores: [10, 16, 29, 38]
  },
  {
    min_age: 26,
    min_scores: [10, 15, 31, 42]
  },
  {
    min_age: 41,
    min_scores: [10, 17, 22, 36]
  }
]

The structure seems neat; everything can seemingly be derived from it (the bracket labels are, as mentioned, constant). There are other ways of doing it of course, but bear with me - my question concerns the code below.

Since the score_brackets attribute is YAML serialized, it can't handle Ruby's Range objects very well, but those seem ideal for this purpose. So I wrote the following, which, frankly, isn't pretty:

# in question_set.rb
def ranged_brackets
  brackets = self.score_brackets.dup
  brackets.each_cons(2) do |pair|
    pair.first[:age_range] = Range.new(pair.first[:min_age], pair.last[:min_age], true) # I could just use the `x...y` syntax, I know
  end
  if brackets.count.odd?
    brackets.last[:age_range] = Range.new(brackets.last[:min_age], 999)
  end

  brackets.map do |bracket|
    score_ranges = []
    bracket[:min_scores].each_cons(2) do |bounds|
      score_ranges << Range.new(bounds.first, bounds.last, true)
    end
    if bracket[:min_scores].count.odd?
      score_ranges << Range.new(bracket[:min_scores].last, 999)
    end
    score_ranges = Hash[BRACKET_LABELS.zip(score_ranges)]
    { age: bracket[:age_range], score: score_ranges }
  end
end

Point is, that now it's easy to pick the right age bracket and score bracket using ranged_brackets.detect ....

(Yes, I could skip all that and find the correct bracket without converting to ranges, but where's the fun in that? What I'm curious to know is whether the above can be done more elegantly, whether or not it's actually the best approach)

I know there are some nasty hard-coded 999 values in there, but those don't worry me too much (if people can grow older than 999 years old, or score more than 999 on test with a known - and much lower - maximum score, I've got other problems).

What really smells is the kinda-sorta mapping going on in the first each_cons block, and the << stuff happening in the second. Ideally, I would like to say something like "map each number in the array to a range from that number up to but excluding the next number (or infinity)" but, well... this is as close as I got.

Again, I know there are lots of other ways to do all this, but now I'm just curious if there's a cleaner way of achieving such array-to-range mapping.


Edit: If anyone's interested, this is what I ended up with thanks to Tokland's suggestions - much, much nicer!

def ranged_brackets
  brackets = self.score_brackets + [{ min_age: 999 }]
  brackets.each_cons(2).map do |a, b|
    age_range = (a[:min_age]...b[:min_age])

    scores = a[:min_scores] + [999]
    score_ranges = scores.each_cons(2).map do |from, upto|
      (from...upto)
    end

    score_ranges = BRACKET_LABELS.zip(score_ranges).reject { |k, v| k.nil? || v.nil? }
    { age: age_range, score: Hash[score_ranges] }
  end
end

The reject-check isn't really be necessary for these data, but it makes me feel a little safer.

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

You repeat this pattern a couple of times:

score_ranges = []
bracket[:min_scores].each_cons(2) do |bounds|
  score_ranges << Range.new(bounds.first, bounds.last, true)
end

Note that this can be written in functional style. Also, you can unpack the args in a block:

score_ranges = bracket[:min_scores].each_cons(2).map { |from, to| from...to }

Other than this, your code looks ok to me. Conceptually you should use a binary search - O(log n) in time - but, with those small data sizes, a O(n) detect is just fine. Anyway, some references on binary search:

\$\endgroup\$
4
  • \$\begingroup\$ Don't know why I never thought to chain each_cons to map - much nicer! The repetition is "on purpose", so to speak, in order to maintain the age → scores relationship; there are some slight differences in the blocks (not that it can't be improved with refactoring). And you're right of course: I could do a bsearch. But for this I was just curious if the above approach could be made nicer. And I was very much expecting an answer from you, tokland :) Thanks! I'll leave a question up for a bit, just to see if anyone else wants to take a swing, but it's answered as far as I'm concerned. \$\endgroup\$
    – Flambino
    May 9, 2013 at 12:53
  • \$\begingroup\$ great, glad to help. I mentioned binary-search` because conceptually it's the way to go, but of course with those sizes of arrays, a O(n) detect is more than ok. On a side node, it's a pity that codereview has such a small community, we're always the same here :-/ \$\endgroup\$
    – tokland
    May 9, 2013 at 13:42
  • \$\begingroup\$ Yeah, I'm not at all worried about performance in this case. It just seemed semantically neat to use ranges for, well, ranges - even if it isn't necessary, conceptually speaking. But it was because it felt neat that I wondered why my code wasn't equally neat. And, yeah, codereview could do with some more people. The quality's here, though, even if the quantity's lacking. \$\endgroup\$
    – Flambino
    May 9, 2013 at 20:11
  • \$\begingroup\$ FYI, I added what I came up with based on your suggestions - it's a lot nicer, I think. Should still do a binary search, but.. meh. This was a fun little exercise :) \$\endgroup\$
    – Flambino
    May 9, 2013 at 23:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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