0
\$\begingroup\$

Imagine code that reads from a file, cleans the data and finally writes the cleaned data to a new file.

Let's briefly define these methods as Ruby stubs:

def write(filename, data)
   File.open(filename, "w") do |f|
      f.write(data.to_yaml)
   end
end

def read(filename)
   YAML.load_file(filename)
end

def clean(data)
   data.map do |line|
      line.strip!
   end
end

We can also have an orchestrator method that triggers the whole logic:

def read_clean_write(filename)
    write(filename, clean(read(filename)))
end

I think that while the isolated read, write and clean methods are modular and apply the single responsibility principle, read_clean_write is quite the opposite (and looks ugly). What is the best way to do that while keeping SRP?

\$\endgroup\$
3
  • 3
    \$\begingroup\$ I'm afraid this question does not match what this site is about. Code Review is about improving existing, working code. The example code that you have posted is not reviewable in this form because it leaves us guessing at your intentions. Unlike Stack Overflow, Code Review needs to look at concrete code in a real context. Please see Why is hypothetical example code off-topic for CR? \$\endgroup\$ – 301_Moved_Permanently Sep 8 '17 at 10:46
  • \$\begingroup\$ @MathiasEttinger there you go. This is working code to be improved. I strongly believe that the reviewer shouldn't be bothered with irrelevant implementation details though as I am asking something very specific. Thanks. \$\endgroup\$ – Michael Sep 8 '17 at 10:59
  • 1
    \$\begingroup\$ Your solution is perfectly fine and preferable imo to the accepted answer. The only thing I'd change is to rename read_clean_write to cleanup! or prettify! or something similar. Orchestration methods are fine, and don't violate SRP, but naming them by their individual actions is not fine: the whole reason they exist is because you've identified a sequence of actions that you think of as a whole. Figure out what that "whole" thing is and name that. \$\endgroup\$ – Jonah Sep 12 '17 at 17:00
3
\$\begingroup\$

One way would be to wrap it in an object with each method returning self so that methods can be chained.

class DataObject
  def initialize(data)
    @data = data
  end

  def clean
     @data = @data.data.map(&::strip)
     self
  end

  def write(filename)
    File.open(filename, "w") do |f|
      f.write(@data.to_yaml)
    end
    self
  end

end

def read(filename)
  data = YAML.load_file(filename)
  DataObject.new(data)
end

Another way would be to use mixins.

module DataMethods

  def clean
    # need to use the destructive methods to modify self
    map!(&::strip)
    # or each(&:strip!)
    self
  end

  def write(filename)
    File.write(filename, to_yaml)
    self
  end

end

def read(filename)
  YAML.load_file(filename).extend(DataMethods)
end
\$\endgroup\$

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.