Writing successive function calls

Question

I have a Ruby function which generates and saves a report. I'm looking for a Clean Code way of writing this:

def generate
  violations = relevant_violations
  comments = convert_to_cleaned_up_comments(violations)
  text = convert_to_text(comments)
  save_to_file(text)  
end

But I hate single-use low-semantic-value variables like these. So here's my first re-write:

def generate
  save_to_file(
    convert_to_text(
      convert_to_cleaned_up_comments(
        relevant_violations)))
end

And even though the flow is now in the reverse order, I find this quicker to grasp. There's less junk getting in the way.

But maybe a map/reduce or block style of programming would provide the same clarity but show data flow in the direction written, top to bottom.

EDIT: I re-wrote it so that the code can be read mostly top-to-bottom, not bottom-to-top:

corpus.rake — the reporting script

def generate_and_save
  save_to_file Violation
    .select(&:within_past_30_days?)
    .map(&:to_corpus_friendly_comment)
    .join "\n"
end

def save_to_file(text)
  File.open(LOCAL_PATH, 'w') { |f| f.write text }
end

violation.rb — the Violation class

  def to_corpus_friendly_comment
    facts
      .downcase
      .gsub(/\b(\d+f?|\w|pic|inspection)\b/, ' ')
      .gsub(/\W/, ' ')
      .gsub(/(foods?|please|days|use)/, '')
      .gsub(/  +/, ' ')
      .strip
  end

  def within_past_30_days?
    date > 30.days.ago
  end

...enabled by object-oriented refactoring; moving the functions into the Violations object enabled method chaining. One part I don't like is that .join "\n" is a lower level of abstraction than the other lines in generate_and_save.

Answer 1

I honestly think your first version is the best one. It's readable, and makes more sense at a glance than the refactorings. That said, I would consider a couple of things.

def generate
  violations = relevant_violations
  comments = convert_to_cleaned_up_comments(violations)
  text = convert_to_text(comments)
  save_to_file(text)  
end

The comments and text temp variables represent a transitional state. I would ask myself if I will use them in any other form. If not, and if their entire reason for existence is to transition to text, I would do away with them and have one entry point for clean, or savable text.

I would consider if generating the report is the Violation class responsibility. The report is a presentational concern, and I feel it should have its own class.

class ViolationsReport
  attr_reader :violations, :file

  def initialize(violations, file)
    @violations = violations
    @file = file
  end

  def generate
    save_to_file formatted_violations
  end

private

  def formatted_violations
    convert_to_comments violations
  end

  def save_to_file(text)
    file.open(LOCAL_PATH, 'w') { |f| f.write text }
  end

  # more methods here to do the job
end

The idea is to isolate presentation logic that is only needed for the report, in the report class itself. You then have cleanly defined responsibilities for each class.

Answer 2

If the goal is to make the following code possible (in the context of your question):

def generate
  value_through_method_chain(
    relevant_violations,
    :convert_to_cleaned_up_comments,
    :convert_to_text,
    :save_to_file
  )
end

then this method can be defined:

def value_through_method_chain(initial_value, *methods)
  methods.reduce(initial_value) do |return_value, method|
    return_value = send(method, return_value)
  end
end

Edited in response to comments.