Keep ABC size down for conditional string concatenation in Ruby

Question

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?

• 200_success: 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.
• 200_success: Take care not to say "refactoring", which has a specific meaning, when you just mean "rewriting".
• willlma: @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.

Answer 1

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

Answer 2

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

• pacmaninbw: 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.