One functional difference between the other solutions and the below solution
Nicola proposes is whether an empty allStates array would be considered pass
or failure. Nicola's code assumes an empty array is failure. I guess the OP
may want to decide how he wants the empty array case handled.
I think that assumption is making things unnecessary complicated
because it changes the semantics of the false return.
Ruby good practice tends to encourage a holistic view of arrays, generally
avoiding #each iteration, so I'd encourage a beginner, on refactoring, to
look at whether an #each is needed and whether state variables like the
"hasGoodStates" Is needed.
Agree.
And if I had to use "hasGoodStates" variable and if it only contains a
boolean value, then I would replace "if hasGoodStates == true" with"if
hasGoodStates"
Absolutely!
Finally, instead of returning the strings "good" and "bad" which are a pain
to test for, I'd expect a method that returns a boolean (and to reflect
this, I would end the method name with a question mark)
So I'd be more likely to suggest the following
def no_unexpected?(all_states, good_states)
return false if all_states.empty? # needed if we assume empty test array
is bad
That test complicates things unnecessarily. Also, it changes the
consistency of the return value.
all_states.any? { |element| !good_states.include? element }
end
There is too much negation going on. This is confusing - at least for me.
I would just use simple logic - the result can easily be negated if needed:
irb(main):001:0> array_status = ['hired','hired','provisionally
hire','hired','hired', 'provisionally hire', 'provisionally
hire','hired']
=> ["hired", "hired", "provisionally hire", "hired", "hired",
"provisionally hire", "provisionally hire", "hired"]
irb(main):002:0> filter = ['hired', 'provisionally hire']
=> ["hired", "provisionally hire"]
irb(main):003:0> array_status.all? {|x| filter.include? x}
=> true
As a method
def all_valid?(enum, filter)
enum.all? {|x| filter.include? x}
end
But I doubt this little piece of code is worthwhile to be defined as a method.
Although I'm still partial to the subtraction method 
def no_unexpected?(all_states, good_states)
return false if all_states.empty? # needed if we assume empty test array
is bad
(all_states - good_states).empty?
end
Disadvantage of the subtraction method is that all elements must be
read. This can make a difference for large inputs. The approach with
#any? or #all? allows to exit as soon as one outlier is found.
And you can tighten up with a ternary and (I think) without sacrificing too
much readability.
def no_unexpected?(all_states, good_states)
all_states.empty? ? false : (all_states - good_states).empty?
end
Again, the ternary operator makes this unnecessary hard to read. You can just do
def no_unexpected?(all_states, good_states)
!all_states.empty? && (all_states - good_states).empty?
end
But see remark above. And btw. did you notice that your method is way
more generic than the current use case? That is why I picked more
generalized parameter names.
Since you are a beginner I would follow a more "didactic" approach;)
As other have said you can write that code in one line ... well, this is the
opposite,
verbose and "as readable as possible".
I disagree that this is "as readable as possible". The longer a method
and the more code it contains the more work the brain has to do. This
is especially true the more variables are in use because variables
store state. And in this case it is totally superfluous.
# INPUT:
# allStates -> array
# goodStates -> array
# WHAT:
# returns "good" if all elemenst of "allStates" are contained into
# "goodStates" array. Otherwise returns "bad"
# EXAMPLES:
# checkStatus(["a","a","b"], ["a","b"])
# => "good"
# checkStatus(["a","a","b","c"], ["a","b"])
# => "bad"
def checkStatus(allStates, goodStates)
hasGoodStates = false
allStates.each do |x|
if goodStates.include? x then
hasGoodStates = true
else
return "bad"
end
end
if hasGoodStates == true then
Don't EVER compare boolean values with a boolean constant! This is
error prone! Do not let yourself get into that habit, even in
languages which have exactly two boolean values. Ruby is not one of
them. It is especially bad showing this as an example to a "beginner".
in case someone wonders, here's why:
irb(main):004:0> [true, false, nil, 123, "hello"].map {|b| if b ==
true; "TRUE"; else "false"; end}
=> ["TRUE", "false", "false", "false", "false"]
irb(main):005:0> [true, false, nil, 123, "hello"].map {|b| if b;
"TRUE"; else "false"; end}
=> ["TRUE", "false", "false", "TRUE", "TRUE"]
Kind regards
robert
···
On Thu, Feb 22, 2018 at 11:17 AM, Steve Turczyn <steve@turczyn.co.uk> wrote:
On Thu, Feb 22, 2018, at 9:34 AM, Nicola Mingotti wrote:
--
[guy, jim, charlie].each {|him| remember.him do |as, often| as.you_can
- without end}
http://blog.rubybestpractices.com/