ruby refactoring class method-CodePudding

I am new to ruby. I am trying to create a report_checker function that checks how often the word "green, red, amber" appears and returns it in the format: "Green: 2/nAmber: 1/nRed:1".

If the word is not one of the free mentioned, it is replaced with the word 'unaccounted' but the number of times it appears is still counted.

My code is returning repeats e.g if I give it the input report_checker("Green, Amber, Green"). It returns "Green: 2/nAmber: 1/nGreen: 2" as opposed to "Green: 2/nAmber: 1".

Also, it doesn't count the number of times an unaccounted word appears. Any guidance on where I am going wrong?

def report_checker(string)
  array = []
  grading = ["Green", "Amber", "Red"]

    input = string.tr(',', ' ').split(" ")
  
    input.each do |x|
       if grading.include?(x)
        array.push( "#{x}: #{input.count(x)}")
       else
         x = "Unaccounted"
        array.push( "#{x}: #{input.count(x)}")
       end
    end   
    
    array.join("/n")
end

 report_checker("Green, Amber, Green")

I tried pushing the words into separate words and returning the expected word with its count

CodePudding user response：

There's a lot of things you can do here to steer this into more idiomatic Ruby:

# Use a constant, as this never changes, and a Set, since you only care
# about inclusion, not order. Calling #include? on a Set is always
# quick, while on a longer array it can be very slow.
GRADING = Set.new(%w[ Green Amber Red ])

def report_checker(string)
  # Do this as a series of transformations:
  # 1. More lenient splitting on either comma or space, with optional leading
  #    and trailing spaces.
  # 2. Conversion of invalid inputs into 'Unaccounted'
  # 3. Grouping together of identical inputs via the #itself method
  # 4. Combining these remapped strings into a single string
  string.split(/\s*[,|\s]\s*/).map do |input|
    if (GRADING.include?(input))
      input
    else
      'Unaccounted'
    end
  end.group_by(&:itself).map do |input, samples|
    "#{input}: #{samples.length}"
  end.join("\n")
end

report_checker("Green, Amber, Green, Orange")

One thing you'll come to learn about Ruby is that simple mappings like this translate into very simple Ruby code. This might look a bit daunting now if you're not used to it, but keep in mind each component of that transformation isn't that complex, and further, that you can run up to that point to see what's going on, or even use .tap { |v| p v }. in the middle to expand on what's flowing through there.

Taking this further into the Ruby realm, you'd probably want to use symbols, as in :green and :amber, as these are very tidy as things like Hash keys: { green: 0, amber: 2 } etc.

While this is done as a single method, it might make sense to split this into two concerns: One focused on computing the report itself, as in to a form like { green: 2, amber: 1, unaccounted: 1 } and a second that can convert reports of that form into the desired output string.

CodePudding user response：

There are lots and lots of ways to accomplish your end goal in Ruby. I won't go over those, but I will take a moment to point out a few key issues with your code in order to show you where the most notable probelms are and to show you how to fix it with as few changes as I can personally think of:

Issue #1:

if grading.include?(x)
array.push( "#{x}: #{input.count(x)}")

This results in a new array element being added each and every time grading includes x. This explains why you are getting repeated array elements ("Green: 2/nAmber: 1/nGreen: 2"). My suggested fix for this issue is to use the uniq method in the last line of your method defintion. This will remove any duplicated array elements.

Issue #2

else
x = "Unaccounted"
array.push( "#{x}: #{input.count(x)}")

The reason you're not seeing any quantity for your "Unaccounted" elements is that you're adding the word(string) "Unaccounted" to your array, but you've also re-defined x. The problem here is that input does not actually include any instances of "Unaccounted", so your count is always going to be 0. My suggested fix for this is to simply find the length difference between input and grading which will tell you exactly how many "Unaccounted" elements there actually are.

Issue #3 ??

I'm assuming you meant to include a newline and not a forward slash (/) followed by a literal "n" (n). My suggested fix for this of course is to use a proper newline (\n). If my assumption is incorrect, just ignore that part.

After all changes, your minimally modified code would look like this:

def report_checker(string)

array = []
  grading = ["Green", "Amber", "Red"]

    input = string.tr(',', ' ').split(" ")
  
    input.each do |x|
       if grading.include?(x)
        array.push( "#{x}: #{input.count(x)}")
       else
        array.push( "Unaccounted: #{(input-grading).length}")
       end
    end   
    
    array.uniq.join("\n")
end

report_checker("Green, Amber, Green, Yellow, Blue, Blue")
#=> 
Green: 2
Amber: 1
Unaccounted: 3

Again, I'm not suggesting that this is the most effective or efficient approach. I'm just giving you some minor corrections to work with so you can take baby steps if so desired.

CodePudding user response：

Try with blow code

add your display logic outside of method

def report_checker(string, grading = %w[ Green Amber Red ])
  data = string.split(/\s*[,|\s]\s*/)
  unaccounted = data - grading
  (data - unaccounted).tally.merge('Unaccounted' => unaccounted.count)  
end

result = report_checker("Green, Amber, Green, Orange, Yellow")
result.each { |k,v| puts "#{k} : #{v}"}

Output

Green : 2
Amber : 1
Unaccounted : 2