4
\$\begingroup\$

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.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – tokland Dec 22 '14 at 12:43
  • \$\begingroup\$ the second version is very lisp-like :-) \$\endgroup\$ – Dogweather Dec 22 '14 at 23:15
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Dogweather Dec 22 '14 at 23:23
  • \$\begingroup\$ 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 :) \$\endgroup\$ – tokland Dec 22 '14 at 23:26
  • \$\begingroup\$ 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] \$\endgroup\$ – Dogweather Dec 22 '14 at 23:31
6
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Yes, that makes a lot of sense. I may do that in my next refactor phase. \$\endgroup\$ – Dogweather Dec 22 '14 at 23:19
0
\$\begingroup\$

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.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Hi, and welcome to Code Review. Your answer mostly consists of a code dump, with only a small explanation of what it does, and no details on why it is better than the question's code. Please consider expanding your answer with details on why you have chosen the strategy you did, and why that strategy makes more sense than the original. \$\endgroup\$ – rolfl Jan 21 '15 at 11:38
  • \$\begingroup\$ 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. \$\endgroup\$ – 200_success Jan 21 '15 at 19:25

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.