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.
Hi Josh!
I have to say, I was really impressed about the time you took to get all those comments together.
It really helps me, it really helps the community reading this comments.
In most sense, I completely agree with almost all your comments
it writes to the wrong filename. That's quite not 100% accurate, as the original excersice (without the bonus) tells: "Then put them into a random order and save the output into a text file." .. as you can see, no file specified.
Agree
Agree! (and consider it a mental note tattooed to my forehead!)
Agree as well, I guess I had the intention to move those two methods down the private section, but never did. Clumsy Victor!
attr_reader vs attr_accessor.. I have always liked the idea to have inmutable objects rather than allowing a change outside of the Object itself. It doesn't feel right. So, unless there's an explicit need to change the object and do recalculation, I'd stick with attr_reader
do_generage "There are only two hard things in Computer Science: cache invalidation and naming things". -- Phil Karlton
Agree
Agree
Agree
...
Overall, I really liked your approach.
One thing I could eventually do better, is follow Ruby style guide github.com/bbatsov/ruby-style-guide like single line method definitions, or somethimes using parenthesis and some other times not.
On any case, Thank you Josh! Thank you for your time and for your refactor.
it writes to the wrong filename. That's quite not 100% accurate, as the original excersice (without the bonus) tells: "Then put them into a random order and save the output into a text file." .. as you can see, no file specified.
Ahh, yes, this is fair. The filename was part of the "bonus challenge", which hopefully no one actually did :P
attr_reader vs attr_accessor.. I have always liked the idea to have inmutable objects rather than allowing a change outside of the Object itself. It doesn't feel right. So, unless there's an explicit need to change the object and do recalculation, I'd stick with attr_reader
Yeah, I'm wary of mutability, too (hence moving the arg from process to initialize). The purpose for the accessor here is so that when we initialize it, we can do self.var = ... rather than @var = ..., which I prefer because if the names become misaligned, then it will error where we try to set it, rather than where we do something after accessing it (or worse, if it doesn't error at that point, b/c nil is a valid value, and instead it blows up elsewhere in the program, or we just get really unexpected results). If we were calling the setter more than once, I'd advocate using a local var instead of an instance var. I guess I'm thinking of it as a "one-time-use" setter.
You know, the more I write Ruby, the more I contemplate making my own attr_* methods :P In this case, the way I use it, a better definition would be something like this.
One thing I could eventually do better, is follow Ruby style guide github.com/bbatsov/ruby-style-guide like single line method definitions, or somethimes using parenthesis and some other times not.
I had no issue with any of your syntactic choices (other than the -1 having mismatched whitespace). Looking at mine, there were a few weird ones, I think that's b/c I was inlining things so that my code samples in the comments would be inline, because the markdown processor doesn't seem to nest code blocks inside of lists. But, I initially wrote all my methods as multiline, and it was really just an error from trying to juggle the formatting requirements of the different places the data existed (my editor, the gist, within the comments).
For further actions, you may consider blocking this person and/or reporting abuse
We're a place where coders share, stay up-to-date and grow their careers.
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.
#process. So now we would call it like this:ExcerciseOrquestrator.new('leaked_pins.txt', 4).process. A natural evolution from that point isExcerciseOrquestrator.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.#call, rather than#processExcerciseOrquestrator, I'd move the reader into the private section, and onNumberGenerator, I'd move#digits,#max_number, and#first_numberinto 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).attr_reader :output_file_nameinto the private section, you can turn it into anattr_accessorPresumably 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 theattr_accessor, you can change the@output_file_name = ...toself.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 helpfulNoMethodError.file.write("#{row}\n")can becomefile.puts row, and sinceputswill take an array of things to puts, you can remove the iterationopen_file { |file| file.puts output }#write_to_filetakes an argument namedoutput, which I would expect to be a string, given the name and the responsibilities of this class. A better name might bepins,numbers,rows,elements, orlines, which makes it clearer that it's a collection. In your example, you call each element arow, so renamingoutputtorowsis a pretty reasonable choice. Generally, I'd start withpinsorpin_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).doondo_generatedo_generatealways returns a list, so no need for(arr || []), and thus no need for the lvar, nowNumberGenerator#processcan be implemented asdo_generate.shuffle#do_generate, usemaprather thaneach_with_object, eg(first_number..max_number).map { |digit| digit.to_s.rjust(digits, '0') }#max_numberto#last_number, just to to align it with#first_number#do_generate, the element is nameddigit, but that implies it's 0-9, I'd rename this tonornumber(nis totally fine here, eveniwould 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 overfirst_number..max_number, so clearly it's a number)NumberGenerator, the worddigitscould imply a collection of digits to generate from, so I'd rename it tonum_digitsto make it clear that we're expecting an int.#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 blockdef open_file(&block)and then pass that, rather than our own block literal:File.open(output_file_name, 'w', &block)#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 # => 11vs[10, 20, 30].first +1 # => [10]Hi Josh!
I have to say, I was really impressed about the time you took to get all those comments together.
It really helps me, it really helps the community reading this comments.
In most sense, I completely agree with almost all your comments
it writes to the wrong filename.That's quite not 100% accurate, as the original excersice (without the bonus) tells: "Then put them into a random order and save the output into a text file." .. as you can see, no file specified.Agree
Agree! (and consider it a mental note tattooed to my forehead!)
Agree as well, I guess I had the intention to move those two methods down the private section, but never did. Clumsy Victor!
attr_readervsattr_accessor.. I have always liked the idea to have inmutable objects rather than allowing a change outside of the Object itself. It doesn't feel right. So, unless there's an explicit need to change the object and do recalculation, I'd stick withattr_readerdo_generage"There are only two hard things in Computer Science: cache invalidation and naming things". -- Phil KarltonAgree
Agree
Agree
...
Overall, I really liked your approach.
One thing I could eventually do better, is follow Ruby style guide github.com/bbatsov/ruby-style-guide like single line method definitions, or somethimes using parenthesis and some other times not.
On any case, Thank you Josh! Thank you for your time and for your refactor.
Ahh, yes, this is fair. The filename was part of the "bonus challenge", which hopefully no one actually did :P
Yeah, I'm wary of mutability, too (hence moving the arg from
processtoinitialize). The purpose for theaccessorhere is so that when we initialize it, we can doself.var = ...rather than@var = ..., which I prefer because if the names become misaligned, then it will error where we try to set it, rather than where we do something after accessing it (or worse, if it doesn't error at that point, b/cnilis a valid value, and instead it blows up elsewhere in the program, or we just get really unexpected results). If we were calling the setter more than once, I'd advocate using a local var instead of an instance var. I guess I'm thinking of it as a "one-time-use" setter.You know, the more I write Ruby, the more I contemplate making my own
attr_*methods :P In this case, the way I use it, a better definition would be something like this.I had no issue with any of your syntactic choices (other than the
-1having mismatched whitespace). Looking at mine, there were a few weird ones, I think that's b/c I was inlining things so that my code samples in the comments would be inline, because the markdown processor doesn't seem to nest code blocks inside of lists. But, I initially wrote all my methods as multiline, and it was really just an error from trying to juggle the formatting requirements of the different places the data existed (my editor, the gist, within the comments).