2
\$\begingroup\$

This method takes a comment object and displays it correctly depending on what columns are available. EVENT_TEXT and REASON_TEXT are hashes that map properties to more human readable strings, like so

EVENT_TEXT = { edit_item: 'edited an item' }
REASON_TEXT = { pickup_date: 'a pickup date change' }

The method that calls this one prepends this text with a username, so the text output expected is something along the lines of edited an item due to a pickup date change: the supplier is not ready yet.

  def comment_text(comment)
    text = ''
    if comment.event.present?
      text += EVENT_TEXT[comment.event.to_sym]
      if comment.reason.present?
        text += ' due to ' + REASON_TEXT[comment.reason.to_sym]
      end
    end
    text + ': ' + comment.text if comment.text.present?
  end

Which has a an Assignement Branch Condition size of 17, higher than Rubocop's default of 15. Is there a better way to concatenate strings conditionally in Ruby? I really don't feel like this method is doing too much.

Should I just increase Rubocop's threshhold? But even if I did, how would I handle a scenario like this with 10 conditions?

\$\endgroup\$
  • \$\begingroup\$ Please provide some more context by showing the code for the Comment class. It seems that in some circumstances, your comment_text() function might return an empty string, or just "due to…" or just ": some text" — is that really so? Note that the second implementation does not have the same behaviour as the first. \$\endgroup\$ – 200_success Apr 8 at 19:59
  • \$\begingroup\$ Take care not to say "refactoring", which has a specific meaning, when you just mean "rewriting". \$\endgroup\$ – 200_success Apr 8 at 19:59
  • \$\begingroup\$ @200_success I've made some updates to the question. You made me realize that I expect a reason whenever there's an event. event, reason, and text are all strings. There is nothing in the comment class. Do you still need more context? It could theoretically return an empty string, but given that different fields are required in different circumstances, it never does in practice. \$\endgroup\$ – willlma Apr 8 at 20:41
0
\$\begingroup\$

Perhaps just extracting the hash lookups would help:

def comment_event_string(comment)
  EVENT_TEXT[comment.event.to_sym]
end

def comment_reason_string(comment)
  " due to #{REASON_TEXT[comment.reason.to_sym]}"
end

def comment_text_string(comment)
  ": #{comment.text}"
end

def comment_text(comment)
  text = ''
  if comment.event.present?
    text += comment_event_string(comment)
    text += comment_reason_string(comment) if comment.reason.present?
  end
  text + comment_text_string(comment) if comment.text.present?
end

Passing comment around seems to be to be something of a code smell though, and I wonder if this code would be better placed inside the Comment class.

private def event_string
  EVENT_TEXT[event.to_sym]
end

private def reason_string
  " due to #{REASON_TEXTreason.to_sym]}"
end

private def text_string
  ": #{text}"
end

def nice_text(comment)
  nice_text = ''
  if event.present?
    nice_text += event_string
    nice_text += reason_string if reason.present?
  end
  nice_text + text_string if text.present?
end
\$\endgroup\$
0
\$\begingroup\$

I think extracting methods would help to make the method more readable and easier to understand. This also reduces the ABC size.

# frozen_string_literal: true

require "ostruct"
require "active_support"
require "active_support/core_ext"
require "minitest/autorun"

EVENT_TEXT = { edit_item: 'edited an item' }.freeze
REASON_TEXT = { pickup_date: 'a pickup date change' }.freeze

def comment_text(comment)
  text = append_event('', comment)
  text = append_reason(text, comment)
  append_comment_text(text, comment)
end

def append_event(text, comment)
  return text if comment.event.blank?

  text + EVENT_TEXT[comment.event.to_sym]
end

def append_reason(text, comment)
  return text if comment.reason.blank?

  text + ' due to ' + REASON_TEXT[comment.reason.to_sym]
end

def append_comment_text(text, comment)
  text + ': ' + comment.text if comment.text.present?
end

alias context describe

describe "comment_text" do
  context "when there is no text" do
    it "returns nil" do
      assert_nil comment_text(OpenStruct.new)
    end
  end

  context "when there is text but no event" do
    it "returns text" do
      assert_equal ": foo", comment_text(OpenStruct.new(text: "foo"))
    end
  end

  context "when the event is unknown" do
    it "blows up" do
      assert_raises do
        comment_text(OpenStruct.new(event: "foo"))
      end
    end
  end

  context "when there is no text and event is known" do
    it "returns nil" do
      assert_nil comment_text(OpenStruct.new(event: "edit_item"))
    end
  end

  context "when there is no text and event and reason is known" do
    it "returns nil" do
      assert_nil comment_text(OpenStruct.new(event: "edit_item", reason: "pickup_date"))
    end
  end

  context "when there is no text and event is known and reason is unknown" do
    it "blows up" do
      assert_raises do
        comment_text(OpenStruct.new(event: "edit_item", reason: "foo"))
      end
    end
  end

  context "when there is text and event is valid" do
    it "displays text" do
      assert_equal "edited an item: foo", comment_text(OpenStruct.new(event: "edit_item", text: "foo"))
    end
  end

  context "when there is text and event and reason are valid" do
    it "displays text with reason" do
      assert_equal "edited an item due to a pickup date change: foo", comment_text(OpenStruct.new(event: "edit_item", reason: "pickup_date", text: "foo"))
    end
  end
end
\$\endgroup\$
  • \$\begingroup\$ Why would extracting methods help? Please add more explanation. This site is primarily about making observations about user code which you do, but you don't provide enough context. Good answers may contain no code at all or a single example. \$\endgroup\$ – pacmaninbw May 10 at 15:11

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.