Discussion on: Write a simple but impactful script

Hi, Victor! 😊

I'm super ambivalent about this, lol! On one hand, I think the right solution for the problem is the procedural one-liner File.write "leaked_pins.txt", (0..9999).map { |n| "%04d\n" % n }.shuffle.join, but on the other hand, I think it's incredibly important to be able to do what you're doing here.

So, since you took the time to write it, I'll take the time to review and refactor it. Hopefully I can be helpful 😊 Also, not everyone is going to agree with me, so I'll try to offer my reasoning for each opinion. My refactored code is here.

• It writes to the wrong filename. I'm only pointing that out b/c if we're making a solution like this, then we're imagining ourselves in a professional-like environment, where the original question is a stand-in for a much more sophisticated real-world problem. In that kind of environment, it's important to be attentive to requirements. Note that, at work, if someone gave me the exact problem that the OP gave, I would write the one-liner. The reason to go with a solution like yours isn't whether you're at work or not, it's that real-world requirements and use-cases tend to be more sophisticated, so as those come in, my one-liner will need to evolve in the direction of what you've written.
• Life will be better when your objects behave like functions (because you won't have to worry about the state of the object going wonky across invocations). A function takes arguments and returns a result, so in this case, I would move all arguments to the constructor (handles the input part), and then the calculation and output to #process. So now we would call it like this: ExcerciseOrquestrator.new('leaked_pins.txt', 4).process. A natural evolution from that point is ExcerciseOrquestrator.process('leaked_pins.txt', 4), which allows you to abstract the implementation to the point that callers don't even need to know whether an object is created to process the task or not.
• In Ruby, the "do that thing you do" method is #call, rather than #process
• It's best to avoid exposing anything you don't need to expose, so on ExcerciseOrquestrator, I'd move the reader into the private section, and on NumberGenerator, I'd move #digits, #max_number, and #first_number into the private section. Treat them as implementation details until there is a need to expose them (and even then, question the need before you oblige it).
• Once you move attr_reader :output_file_name into the private section, you can turn it into an attr_accessor Presumably the reason you did reader over accessor is because you don't want anyone to be able to call the setter from the outside, which is a good intuition, making it private accomplishes the same thing. Once you have the attr_accessor, you can change the @output_file_name = ... to self.output_file_name = ..., the advantage of this is that it will explode helpfully if they ever get out of alignment. Eg if you're refactoring you might change the name, if you forget to change it where you set the value, then it will just set the wrong ivar, but if you use the setter, you'll get a helpful NoMethodError.
• file.write("#{row}\n") can become file.puts row, and since puts will take an array of things to puts, you can remove the iteration open_file { |file| file.puts output }
• #write_to_file takes an argument named output, which I would expect to be a string, given the name and the responsibilities of this class. A better name might be pins, numbers, rows, elements, or lines, which makes it clearer that it's a collection. In your example, you call each element a row, so renaming output to rows is a pretty reasonable choice. Generally, I'd start with pins or pin_numbers, because it's the most concrete, so it's the easiest to understand, and then I'd move to something more abstract, as the class becomes more abstract (ie as the numbers aren't always pins, or as the things to write aren't always numbers).
• I don't see the point of do on do_generate
• do_generate always returns a list, so no need for (arr || []), and thus no need for the lvar, now NumberGenerator#process can be implemented as do_generate.shuffle
• In #do_generate, use map rather than each_with_object, eg (first_number..max_number).map { |digit| digit.to_s.rjust(digits, '0') }
• I might rename #max_number to #last_number, just to to align it with #first_number
• In #do_generate, the element is named digit, but that implies it's 0-9, I'd rename this to n or number (n is totally fine here, even i would be okay, partly because it's incredibly common to use those names for this kind of thing, so people understand it, and partly because we're iterating over first_number..max_number, so clearly it's a number)
• In NumberGenerator, the word digits could imply a collection of digits to generate from, so I'd rename it to num_digits to make it clear that we're expecting an int.
• In #open_file, it passes a block which then yields to the method's block. So, it's just sort of passing the argument through. We can instead explicitly receive the block def open_file(&block) and then pass that, rather than our own block literal: File.open(output_file_name, 'w', &block)
• In #max_number, be careful to put matching whitespace on both sides of operators. Here, it's not an issue, but in other situations it can be read as "negative one", which can change how it parses. Example: [10, 20, 30].first + 1 # => 11 vs [10, 20, 30].first +1 # => [10]
• Randomness is a pretty big dependency, so consider injecting it.
• If the OP was more real-world, we could rename the class to be something meaningful, and then I would probably use keyword arguments instead of ordinal ones.

I'm going to need to see the full test suite for this too 😉