# Writing successive function calls

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
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(
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(/  +/, ' ')
.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.

• I think you should re-evaluate why you don't like the first snippet. Using local variables so intermediate values have meaningful names is the base for writing clean code. The second version is just awful in comparison. Dec 22 '14 at 12:43
• the second version is very lisp-like :-) Dec 22 '14 at 23:15
• Also, @tokland, it felt like these intermediate variables don't provide any more meaning. They are just redundant noise since the functions are already well named. They're only necessary because of how function invocation works. Dec 22 '14 at 23:23
• you'd definitely like a language based on Haskell I wrote, there it would look relevant_violations.to_cleaned_up_comments.to_text.save_to_file :) Dec 22 '14 at 23:26
• Yep - exactly. I feel like that syntax is called for here, but in Ruby it'd require programming overhead. I had the idea, though, of a Workflow utilty which would look like this: Worflow [:relevant_violations, :to_cleaned_up_comments, :to_text, :save_to_file] Dec 22 '14 at 23:31

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
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

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

def generate
save_to_file formatted_violations
end

private

def formatted_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.

• Yes, that makes a lot of sense. I may do that in my next refactor phase. Dec 22 '14 at 23:19

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_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


• May I suggest changing value_through_method_chain to take *methods instead of methods? Then you could write value_through_method_chain relevant_violations, :convert_to_cleaned_up_comments, :convert_to_text, :save_to_file with less punctuation. Jan 21 '15 at 19:25