5
\$\begingroup\$

The function days_name receive a val (string) corresponding to weekdays in hexadecimal.

def days_name(val)
    value = val.to_i(16)
    week = Array.new

    if value.eql? 127
      week.push 'All'
    elsif value.eql? 0
      week.push 'No'
    else
      week.push 'mon' if (value & 1) > 0
      week.push 'tue' if (value & 2) > 0
      week.push 'wed' if (value & 4) > 0
      week.push 'thu' if (value & 8) > 0
      week.push 'fri' if (value & 16) > 0
      week.push 'sar' if (value & 32) > 0
      week.push 'sun' if (value & 64) > 0
    end
    week.join(' ')
end
\$\endgroup\$
0

1 Answer 1

8
\$\begingroup\$
  • days_name? Shouldn't that rather be day_names? Actually, given its function it should probably be weekdays

  • Shouldn't "sar" be "sat" for Saturday? Never seen it abbreviated "sar"

  • The opposite of "all" is "none" - not "no"

  • Day names are capitalized in English, so "Mon", "Tue", "Wed", etc.

  • You rarely need to create an array and then push items to it. It's easier to simply filter a complete array. But if you do need to create an empty array, just use [] - no need for Array.new

Sticking close to your current code, I'd do this

def weekdays(hex)
  value = hex.to_i(16)
  case value
  when 127
    'All'
  when 0
    'None'
  else
    days = %w(Mon Tue Wed Thu Fri Sat Sun)
    days.each_with_index.select { |_, i| value & 2**i > 0 }.map(&:first).join(" ")
  end
end

However, I'd probably prefer a method that simply returns an array of day names, rather than a string. Then let other code check and join the array as needed.

Something like this

def weekdays(hex)
  value = hex.to_i(16)
  days = %w(Mon Tue Wed Thu Fri Sat Sun)
  days.each_with_index.select { |_, i| value & 2**i > 0 }.map(&:first)
end

Update: As tokland points out in the comments, it'd be cleaner to use map.with_index plus compact, rather than each_with_index.select plus map. I.e.

def weekdays(hex)
  value = hex.to_i(16)
  days = %w(Mon Tue Wed Thu Fri Sat Sun)
  days.map.with_index { |day, i| day if value & 2**i > 0 }.compact
end

or, for the first code block

def weekdays(hex)
  value = hex.to_i(16)
  case value
  when 127
    'All'
  when 0
    'None'
  else
    days = %w(Mon Tue Wed Thu Fri Sat Sun)
    days.map.with_index { |day, i| day if value & 2**i > 0 }.compact.join(" ")
  end
end

As mentioned in the comments, I happened to make a gem for weekday bitmasks a while back, which you might find useful. The major difference is that the gem expects Sunday to be zero, Monday to be 1 and so forth (like Date#wday and most other date APIs).

\$\endgroup\$
3
  • 2
    \$\begingroup\$ Nitpick: I'd prefer to write the list-comprehensions like this: days.map.with_index { |day, i| day if value & 2**i > 0 }.compact. \$\endgroup\$
    – tokland
    Commented Aug 23, 2014 at 15:40
  • 1
    \$\begingroup\$ @tokland Better - certainly better. I'll update the answer an addendum \$\endgroup\$
    – Flambino
    Commented Aug 23, 2014 at 16:14
  • 2
    \$\begingroup\$ Very good answer. +1. Two ways that do not convert to an array and back: 7.times.with_object('') { |i,s| s << "#{days[i]} " if value & 2**i > 0 }.rstrip and, without an index, ''.tap { |s| days.each { |d| s << "#{d} " if value & 1 > 0; value = value >> 1 } }.rstrip \$\endgroup\$ Commented Aug 24, 2014 at 5:48

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.