Consistency
Sometimes you are using whitespace around operators, sometimes you don't. Sometimes you are using whitespace between the message name and the argument list of a message send, sometimes you don't.
You should choose one style and stick with it. If you are editing some existing code, you should adapt your style to be the same as the existing code. If you are part of a team, you should adapt your style to match the rest of the team.
Most communities have developed standardized community style guides. In Ruby, there are multiple such style guides. They all agree on the basics (e.g. indentation is 2 spaces), but they might disagree on more specific points (single quotes or double quotes).
In general, if you use two different ways to write the exact same thing, the reader will think that you want to convey a message with that. So, you should only use two different ways of writing the same thing IFF you actually want to convey some extra information.
For example, some people always use parentheses for defining and calling purely functional side-effect free methods, and never use parentheses for defining and calling impure methods. That is a good reason to use two different styles (parentheses and no parentheses) for doing the same thing (defining methods).
Whitespace around operators
There should be 1 space either side of an operator.
Whitespace in hash literals
In a hash literal, there should be one space after the opening curly brace and one space before the closing curly brace.
New-style hash literals
Well, they are called "new-style" or "Ruby 1.9" hash literals, but really, they were introduced in 2007, and we have Ruby 3.0 soon, so we should really simply be calling them "hash literals" at this point. They are neither new anymore nor is Ruby 1.9 still relevant.
Ruby 1.9 introduced a shortcut syntax for hash literals where keys are symbols, inspired by ECMAScript.
So, this:
{:arg => ARGV[i], :val => ARGV[i+1]}
should be this:
{ arg: ARGV[i], val: ARGV[i+1] }
Frozen string literals
Immutable data structures and purely functional code are always preferred, unless mutability and side-effects are required for clarity or performance. In Ruby, strings are always mutable, but there is a magic comment you can add to your files (also available as a command-line option for the Ruby engine), which will automatically make all literal strings immutable:
# frozen_string_literal: true
It is generally preferred to add this comment to all your files.
No space before parameter list
In a method definition, there should be no whitespace between the method name and the parameter list, e.g. this:
def self.get_params (ary)
should be
def self.get_params(ary)
Ruby allows you to specify a parameter list either in parentheses or with whitespace. If you combine the two, it could get confusing, because Ruby also allows array destructuring in parameter lists:
def foo(((first_of_first, second_of_first), second))
second_of_first
end
foo([[23, 42], 'Hello'])
#=> 42
Array destructuring is performed by adding an additional set of parentheses. Now, if you have a method with two parameters, you can write it like this:
def foo(bar, baz)
end
or like this:
def foo bar, baz
end
If you write it like this:
def foo (bar, baz)
end
A reader might think that this is a single parameter that uses array destructuring. In fact, Ruby even prints a warning that tells you exactly that:
warning: parentheses after method name is interpreted as an argument list, not a decomposed argument
No space before argument list
In a message send, there should be no whitespace between the message and the argument list, e.g. this:
args.push ({:arg => ARGV[i], :val => ARGV[i+1]})
should be
args.push({:arg => ARGV[i], :val => ARGV[i+1]})
This will bite you as soon as you have more than one argument. Ruby allows you to write the argument list to a message send without parentheses like this:
foo.bar baz, quux
instead of
foo.bar(baz, quux)
The problem arises because the whitespace triggers the first mode, so
foo.bar (baz, quux)
is not interpreted as two arguments baz and quux but as passing arguments with whitespace instead of parentheses and thus as a single argument (baz, quux). Except (baz, quux) is not legal Ruby code, and thus you get an error.
Warnings
The Ruby developers work hard on providing helpful warning messages. You should read them and investigate them. They almost always point to problems or at least potential problems in your code.
For example, running your code generates the following warning message:
warning: parentheses after method name is interpreted as an argument list, not a decomposed argument
Unused parameter
Your get_params method is defined with one parameter ary, but you are not using this parameter anywhere. It is completely useless.
If you have a parameter or a local variable that is intentionally unused, the Ruby community standard is to name it with a leading underscore, e.g. _ary or even just _.
However, in this case, it serves absolutely no purpose and should simply be removed altogether.
Empty literals
Instead of using the new method to create instances of core classes, you should prefer to use literals. E.g. use '' or "" instead of String.new, [] instead of Array.new, and so forth.
This is especially true for empty objects.
args = Array.new
should be
args = []
Prefer each over for / in
Nobody in Ruby uses for / in. In fact, we hardly use any loops at all. I would go so far as to say, when you are using a loop, you are doing it wrong.
The idiomatic way would be to use Range#each instead. Not only is each more idiomatic, but for / in has this weird scoping rule where the loop variable escapes into the surrounding scope, which is almost always not what you want.
So, this:
for i in 0..ARGV.size-1
if ARGV[i].start_with?('-') and i < ARGV.size + 1
args.push ({:arg => ARGV[i], :val => ARGV[i+1]})
end
end
should become this:
(0..ARGV.size-1).each do |i|
if ARGV[i].start_with?('-') and i < ARGV.size + 1
args.push ({:arg => ARGV[i], :val => ARGV[i+1]})
end
end
Actually, there is a lot more that can be improved about that, but we will stick with the purely mechanical transformations for now, and keep our brain switched off.
Modifier if
If you have a conditional expression with only a single expression in the body and no else, it is generally preferred to use the modifier form.
if ARGV[i].start_with?('-') and i < ARGV.size + 1
args.push ({:arg => ARGV[i], :val => ARGV[i+1]})
end
should be
args.push ({:arg => ARGV[i], :val => ARGV[i+1]}) if ARGV[i].start_with?('-') and i < ARGV.size + 1
Prefer symbolic operators ||, &&, ! over keywords and, or, not
In a boolean expression, you should prefer the symbolic operators over the keywords. The keywords have doubly-unexpected precedence: not only is the precedence of the keywords and and or much lower than the precedence of && and ||, but also and and or both have the same precedence while && has higher precedence than ||. This can be very confusing.
Idiomatic Ruby community standard is to always use the symbolic operators, except for when you actually use boolean logic for control flow, as in
do_something and exit
So,
ARGV[i].start_with?('-') and i < ARGV.size + 1
should be
ARGV[i].start_with?('-') && i < ARGV.size + 1
Redundant return
The return value of a method is the value of the last expression that was evaluated inside the method. Unless you want an early return, it is generally not necessary to use return at all.
Linting
You should run some sort of linter or static analyzer on your code. Rubocop is a popular one, but there are others.
Rubocop was able to detect all of the style violations I pointed out above (plus some more), and also was able to autocorrect all of the ones I listed.
Let me repeat that: I have just spent two pages pointing out how to correct tons of stuff that you can actually correct within milliseconds at the push of a button. I have set up my editor such that it automatically runs Rubocop with auto-fix as soon as I hit "save".
In particular, running Rubocop on your code, it detects 19 offenses, of which it can automatically correct 17. This leaves you with 2 offenses, both of which are very simple.
Here's what the result of the auto-fix looks like:
#!/usr/bin/env ruby
# frozen_string_literal: true
class Script_Helper
def self.get_params(_ary)
args = []
unless ARGV.empty?
(0..ARGV.size - 1).each do |i|
args.push({ arg: ARGV[i], val: ARGV[i + 1] }) if ARGV[i].start_with?('-') && (i < ARGV.size + 1)
end
end
args
end
end
And here are the offenses that Rubocop could not automatically correct:
Offenses:
script_helper.rb:4:1: C: Style/Documentation: Missing top-level class documentation comment.
class Script_Helper
^^^^^
script_helper.rb:4:7: C: Naming/ClassAndModuleCamelCase: Use CamelCase for classes and modules.
class Script_Helper
^^^^^^^^^^^^^
1 file inspected, 2 offenses detected
Let's look at the simple one first.
Naming
Class names should be PascalCase, constants should be SCREAMING_SNAKE_CASE, everything else should be snake_case. Files should be named after the primary module or class they define, and they should be named in snake_case as well.
So, the name of your class should be ScriptHelper, not Script_Helper.
Unused parameter, take 2
As mentioned above, the parameter should be removed. Rubocop only automatically renamed it, because removing it could break code that calls this method with an argument.
But since it isn't used, it should simply be removed and all callers fixed to not pass an argument.
Vertical whitespace
Your method could use some vertical whitespace to give the code more room to breathe. I would suggest at least separating the initialization at the beginning of the method, and the return at the end:
def self.get_params(_ary)
args = []
unless ARGV.empty?
(0..ARGV.size - 1).each do |i|
args.push({ arg: ARGV[i], val: ARGV[i + 1] }) if ARGV[i].start_with?('-') && (i < ARGV.size + 1)
end
end
args
end
"Reader" methods
I re-run Rubocop every time I change something in my code. In fact, I have it set up to run automatically when I save my code, when I commit my code, etc.
Since we now have removed the parameter from the method, it now tells us something new: "Reader" methods or "getter methods" that don't take any arguments and simply return a value should not be named with a get_ prefix. This is unlike Java, for example.
So, the method should simply be named params.
Redundant check
If ARGV is empty, its size will be 0, which means that the range 0..ARGV.size - 1 will be the range 0..-1, which is empty. Iterating over an empty range doesn't do anything, so we can just delete the check for ARGV.empty?
Redundant check 2
You are iterating from 0 to size-1, therefore i will never be greater than size. The check i < ARGV.size + 1 will always be true, and is thus redundant and can be deleted.
Iterators, take 2
We already converted the for loop into an each iterator. However, you are iterating over the indices of ARGV and then pull out the elements using the indices. This is a very FORTRAN way of writing that code. It is much easier to iterate over the elements directly.
Even better, since you always need the to look at the current and the next element, we can iterate over a sliding window of 2 consecutive elements. That's exactly what Enumerable#each_cons does:
ARGV.each_cons(2) do |param, arg|
args.push({ arg: param, val: arg }) if param.start_with?('-')
end
As above, if ARGV is empty, each_cons will simply not do anything, so we don't need to check.
The "shovel" operator <<
And while we are looking at this code: Array#push is not idiomatic. More precisely, it is only idiomatic if you are using the array as a stack, then you would use Array#push and Array#pop, since those are the standard names for the stack operations.
The idiomatic way of appending something to something else is the shovel operator, in this case Array#<<, so that should be
args << { arg: param, val: arg }
Iterators, take 3
We are still doing a lot of stuff manually. Initialize the empty array, loop over ARGV, append to the array, etc.
Whenever you have the pattern of "Initialize a result, loop over a collection appending to the result, return result", that is a fold. There are two implementations of fold in the Ruby core library, inject and each_with_object. inject is the more functional one, each_with_object is the more imperative one. So, for now, we will use each_with_object here, since the code is still pretty imperative, and that makes the relationship between the old and new code more clear:
args = []
ARGV.each_cons(2) do |param, arg|
args << { arg: param, val: arg } if param.start_with?('-')
end
args
becomes
ARGV.each_cons(2).each_with_object([]) do |(param, arg), args|
args << { arg: param, val: arg } if param.start_with?('-')
end
Now, if we want to make it purely functional, it would look like this:
ARGV.each_cons(2).inject([]) do |args, (param, arg)|
if param.start_with?('-') then args + [{ arg: param, val: arg }] else args end
end
Iterators, take 4
Instead of filtering inside the iteration, we could instead chain iterators and filter first. This works for both our variations:
ARGV
.each_cons(2)
.select { |param, _| param.start_with?('-') }
.each_with_object([]) { |(param, arg), args| args << { arg: param, val: arg } }
ARGV
.each_cons(2)
.select { |param, _| param.start_with?('-') }
.inject([]) { |args, (param, arg)| args + [{ arg: param, val: arg }] }
Excursion: Generality of fold (inject / each_with_object)
When I wrote above that you can rewrite this iteration with inject or each_with_object, that was actually a tautological statement. I didn't even have to read the code to make this statement.
It turns out that fold is "general". Every iteration over a collection can be expressed using fold. This means, if we were to delete every method from Enumerable, except inject, then we could re-implement the entire Enumerable module again, using nothing but inject. As long as we have inject, we can do anything.
Iterators, take 5
So, what we did until now was replace the loop with a low-level iterator, replace the low-level iterator with a higher-level iterator, and decompose the iteration into two chained higher-level iterators.
However, we are still not done. What we are now doing is we take each element from our filtered sliding window collection, modifying it, and putting it into a new collection. So, really, what we are doing is transforming each element, or "mapping" each element to a new element.
This is called map and is also available in Ruby as Enumerable#map.
So, finally, our code looks like this:
ARGV
.each_cons(2)
.select { |param, _| param.start_with?('-') }
.map { |param, arg| { arg: param, val: arg } }
end
What we did here, was to replace the general high-level iterator fold (which can do anything) with a more restricted, more specialized high-level iterator map. By using a more specialized iterator, we are able to better convey our semantics to the reader. Instead of thinking "Okay, so here we have an accumulator, and an element, and we do something with the element, and then append it to the accumulator … ah, I see, we are transforming each element", the reader just sees map and instantly knows that map transforms the elements.
Classes and modules
In your code, you are using a class. However, your class cannot sensibly be instantiated. Therefore, it shouldn't be a class.
It makes more sense for it to be a module instead. Here's how I would write your entire file:
#!/usr/bin/env ruby
# frozen_string_literal: true
module ScriptHelper
module_function def params =
ARGV
.each_cons(2)
.select { |param, _| param.start_with?('-') }
.map { |param, arg| { arg: param, val: arg } }
end
Linter, revisited
Rubocop actually complains about my inline use of Module#module_function. It wants me to write this instead:
#!/usr/bin/env ruby
# frozen_string_literal: true
module ScriptHelper
def params =
ARGV
.each_cons(2)
.select { |param, _| param.start_with?('-') }
.map { |param, arg| { arg: param, val: arg } }
module_function :params
end
I disagree. You should not be afraid to disable or re-configure rules in your linter to fit your style.
However, note that programming is a team sport. If you are modifying code, adopt the existing style. If you are part of a team, adopt the team's style. If you write open source code, adopt the project's style. If you start your own project, adopt the community's style (do not create your own style for your project, until your project is big and successful enough to have its own independent community).
Speaking of community …
Of course, the really correct solution to your problem is to use one of the many, already existing, well-maintained, well-tested, well-documented, well-known command line option parser libraries. Ruby ships with optparse in the standard library, but there are many, many others with some amazing features:
Don't re-invent the wheel.
It is true that NASA has (kind-of) re-invented the wheel for the Perseverance rover, but most of us are not NASA. We don't face the unique challenge of sending a rover to Mars that needs to autonomously drive over rough, unknown terrain, and needs to be as light while at the same time as robust as possible. We just want to get from A to B on a paved road. There are plenty of wheels available off-the-shelf that do that.
Always remember: we are not NASA. It is highly unlikely that a problem we are encountering has never before been seen in the history of humankind. Parsing command line arguments is not a highly-specialized, complex, unique problem, it is a problem that has been solved with standardized libraries over 30 years ago.
