Consistency
Sometimes you use parentheses around the argument list for a message send, sometimes you don't. Sometimes you use mutating methods (e.g. Array#map! instead of Enumerable#map) and sometimes you use purely functional methods (e.g. Array#reverse instead of Array#reverse!). Sometimes you introduce temporary local variables, sometimes you chain message sends.
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. One popular such style guide is the one found at https://rubystyle.guide/. They all agree on the basics (e.g. indentation is 2 spaces), but they might disagree on more specific points (e.g. 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).
If you don't use string interpolation, it is helpful if you use single quotes for your strings. That way, it is immediately obvious that no string interpolation is taking place.
require "./exercises/print_last_lines.rb"
should instead be
require './exercises/print_last_lines.rb'
And
lines.join ", "
should instead be
lines.join ', '
Note that it is perfectly fine to use double quoted strings if you otherwise needed to use escapes, e.g. if you had some something like
puts "michael's Ruby exercise"
that reads much better than
puts 'Michael\'s Ruby exercise'
So in this case, double quotes would be preferred.
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.
It is generally preferred to use parentheses around the argument list for message sends with a non-empty argument list. For example,
lines.join ', '
should instead be
lines.join(', ')
There are exceptions to this rule. Message sends that act more like "commands" (for lack of a better word), in particular message sends that invoke methods in Kernel which do not interact with self and are invoked purely for their side-effects (for example Kernel#require or Kernel#puts), are usually written without parentheses around the argument list.
One-line blocks
A block which contains only a single expression is more readable when written on a single line.
lines.map! do |line|
line.chomp
end
can become
lines.map! do |line| line.chomp end
However, most style guides say you should use curly braces { / } instead of do / end for single-line blocks, so it should look like this:
lines.map! {|line| line.chomp }
In Ruby, you can use the unary prefix ampersand & block unrolling operator. This operator is only valid inside an argument list, and what it does, is "unroll" a Proc object into a block. This allows you to pass a Proc to a method which expects a block. If the object isn't Proc, Ruby will send it the message to_proc in order to tell it to convert itself into a Proc.
This allows you to do some neat things to make your code more expressive.
One of the most-used of these neat things is using Symbol#to_proc to convert a Symbol into a Proc, and then unroll that Proc into a block using the unary prefix ampersand & block unrolling operator.
Symbol#to_proc looks roughly like this:
class Symbol
def to_proc = -> *args { args.shift.__send__(self, *args) }
end
[The real implementation is a bit more complicated but that's the gist.]
So, basically, what it does, is it creates a Proc which sends the name of the Symbol as a message to the first argument of the block, passing the rest of the block arguments as arguments to the message send.
So, if I do this:
reverser = :reverse.to_proc
that is roughly equivalent to
reverser = -> *args { args.shift.__send__(:reverse, *args) }
which is even more roughly equivalent to
reverser = -> a { a.__send__(:reverse) }
which is very roughly equivalent to
reverser = -> a { a.reverse }
Which means I can now do this:
reverser.('Hello')
#=> 'olleH'
Which allows to do useful things like this:
%w[Hello World].map(&reverser)
#=> %w[olleH dlroW]
Which can also be written like this:
%w[Hello World].map(&:reverse)
#=> %w[olleH dlroW]
And now you are hopefully starting to see how this can be more readable than the equivalent:
%w[Hello World].map {|word| word.reverse }
#=> %w[olleH dlroW]
In your case, you can replace
lines.map! {|line| line.chomp }
with
lines.map!(&:chomp)
In your argument to the Kernel#require method, you include the .rb file extension. That is not necessary. Kernel#require will search for files with the extension .rb automatically, as well as a number of other file extensions based on the Ruby implementation and the Operating System.
Let's assume you are using YARV on Linux and want to rewrite print_last_lines in C for better performance. Now, your file will no longer be named print_last_lines.rb but print_last_lines.so. If you use
require 'print_last_lines.rb'
that will not work, as that file no longer exists. However, if you use
require 'print_last_lines'
then Ruby will automatically find the print_last_lines.so native extension without you having to change anything.
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 most of the ones it detected.
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".
Here's what the result of the auto-fix looks like:
print_last_lines.rb
# frozen_string_literal: true
def print_last_lines(filename, n)
lines = File.readlines(filename).last(n) # Get last n-lines
lines = lines.reverse # Reverse the order
lines.map!(&:chomp)
lines.join(', ') # Concatenate the lines together
end
main.rb
# frozen_string_literal: true
require './exercises/print_last_lines'
puts print_last_lines('/Library/WebServer/Documents/ruby-playground/input.txt', 3)
# Returns: '9. Line of Text, 8. Line of Text, 7. Line of Text'
And here are the offenses that Rubocop could not automatically correct:
Inspecting 2 files
W.
Offenses:
exercises/print_last_lines.rb:3:1: C: Style/TopLevelMethodDefinition: Do not define methods at the top-level. (https://rubystyle.guide#top-level-methods)
def print_last_lines(filename, n) ...
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
exercises/print_last_lines.rb:3:32: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def print_last_lines(filename, n)
^
exercises/print_last_lines.rb:4:11: W: Lint/ConstantResolution: Fully qualify this constant to avoid possibly ambiguous resolution.
lines = File.readlines(filename).last(n) # Get last n-lines
^^^^
exercises/print_last_lines.rb:4:44: C: Style/InlineComment: Avoid trailing inline comments.
lines = File.readlines(filename).last(n) # Get last n-lines
^^^^^^^^^^^^^^^^^^
exercises/print_last_lines.rb:5:25: C: Style/InlineComment: Avoid trailing inline comments.
lines = lines.reverse # Reverse the order
^^^^^^^^^^^^^^^^^^^
exercises/print_last_lines.rb:7:32: C: Style/InlineComment: Avoid trailing inline comments.
lines.join(', ') # Concatenate the lines together
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2 files inspected, 6 offenses detected
It is a good idea to set up your tools such that the linter is automatically run when you paste code, edit code, save code, commit code, or build your project, and that passing the linter is a criterium for your CI pipeline.
In my editor, I actually have multiple linters and static analyzers integrated so that they automatically always analyze my code, and also as much as possible automatically fix it while I am typing. This can sometimes be annoying (e.g. I get over 20 notices for your original code, lots of which are duplicates because several different tools report the same problem), but it is in general tremendously helpful. It can be overwhelming when you open a large piece of code for the first time and you get dozens or hundreds of notices, but if you start a new project, then you can write your code in a way that you never get a notice, and your code will usually be better for it.
However, even by simply hitting "Save", my editor applies a series of automatic fixes which brings the number of notices down substantially. Running Rubocop as described above, further reduces this, and as mentioned, lots of these are duplicates because I have multiple different linters and analyzers configured.
So, let's look at what Rubocop complains about.
exercises/print_last_lines.rb:3:1: C: Style/TopLevelMethodDefinition: Do not define methods at the top-level. (https://rubystyle.guide#top-level-methods)
def print_last_lines(filename, n) ...
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
What Rubocop is telling us here is that you shouldn't define methods at the top-level (which actually makes them private instance methods of Object). In general, you should structure your code as multiple specialized objects sending messages to each other.
However, the style guide says:
It is fine to use top-level method definitions in scripts.
Which is another important lesson: if you disagree with something that is flagged by the linter (in this case Rubocop), then turn it off.
Alternatively, we could follow the advice in Private Global Methods:
Private Global Methods
If you really need "global" methods, add them to Kernel and make them private.
Like this:
module Kernel
private
def print_last_lines(filename, n)
# …
end
end
Method parameter must be at least 3 characters long
exercises/print_last_lines.rb:3:32: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def print_last_lines(filename, n)
^
I would agree that the naming here is not particularly clear. As Phil Karlton famously said:
There are only two hard things in Computer Science: cache invalidation and naming things.
I would argue that n is not a particularly good name. I would prefer something like number_of_lines.
Fully qualify this constant to avoid possibly ambiguous resolution
exercises/print_last_lines.rb:4:11: W: Lint/ConstantResolution: Fully qualify this constant to avoid possibly ambiguous resolution.
lines = File.readlines(filename).last(n) # Get last n-lines
^^^^
What Rubocop is telling us here is that we should use the full path to the File constant in order to avoid a different File constant somewhere accidentally breaking our program. In other words, use ::File instead of File.
Avoid trailing inline comments
exercises/print_last_lines.rb:4:60: C: Style/InlineComment: Avoid trailing inline comments.
lines = ::File.readlines(filename).last(number_of_lines) # Get last n-lines
^^^^^^^^^^^^^^^^^^
exercises/print_last_lines.rb:5:25: C: Style/InlineComment: Avoid trailing inline comments.
lines = lines.reverse # Reverse the order
^^^^^^^^^^^^^^^^^^^
exercises/print_last_lines.rb:7:32: C: Style/InlineComment: Avoid trailing inline comments.
lines.join(', ') # Concatenate the lines together
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
What Rubocop wants us to do here is to put the comment above the code that is explained by the comment, like this:
def print_last_lines(filename, number_of_lines)
# Get last n-lines
lines = ::File.readlines(filename).last(number_of_lines)
# Reverse the order
lines = lines.reverse
lines.map!(&:chomp)
# Concatenate the lines together
lines.join(', ')
end
The hard stuff
Note that all we did so far was either done automatically for us by the editor or Rubocop's auto-correct feature, or we were blindly following instructions such as renaming variables. We did not yet have to think at all.
This is one of the great things about using automatic code formatters, linters, and static analyzers: you don't have to think about all the simple stuff. Where do I put my parentheses? How do I indent my code? What are the naming conventions? All of that is taken care of by the tools, I don't have to worry about it and can instead focus on the important stuff: what should the code actually do?
Another advantage of following community style guides and using community tools is that, if my code looks like everybody else's code, and everybody else's code looks like my code, it is much easier to get someone else to help me with my code.
So, just by asking Rubocop to automatically fix our code for us, and by stupidly and without thinking following what Rubocop told us to do, we ended up with the following code:
print_last_lines.rb
# frozen_string_literal: true
module Kernel
private
def print_last_lines(filename, number_of_lines)
# Get last n-lines
lines = ::File.readlines(filename).last(number_of_lines)
# Reverse the order
lines = lines.reverse
lines.map!(&:chomp)
# Concatenate the lines together
lines.join(', ')
end
end
main.rb
# frozen_string_literal: true
require './exercises/print_last_lines'
puts print_last_lines('/Library/WebServer/Documents/ruby-playground/input.txt', 3)
# Returns: '9. Line of Text, 8. Line of Text, 7. Line of Text'
Which I would argue is already more readable than what we started off with, and is a lot more conformant with community style guidelines.
However, we can do even better. And we still don't have to think! There are still some things in the Ruby Style Guide that aren't automatically flagged by Rubocop but that we can fix just by reading the Style Guide and applying its advice.
This tells us that you should replace
require './exercises/print_last_lines'
with
require_relative 'exercises/print_last_lines'
Another interesting thing is that there is usually a reason for why community style guidelines are the way they are. In this particular case, we have actually fixed a bug that I bet you didn't know was there!
The . in
require './exercises/print_last_lines'
means "the current working directory of the process". However, you have no idea what the current working directory of the process is! So, you have no idea whether the file you are requireing is actually where you think it is.
For example, if your file hierarchy looks like this:
totally_unrelated_directory/projects/
And I do
cd /projects
ruby main.rb
Then it will work, because . is /projects and thus ./exercises/print_last_lines.rb is /projects/exercises/print_last_lines.rb, which is correct.
But if I do
cd /totally_unrelated_directory
ruby /projects/main.rb
Then it will break (more precisely, it will raise a LoadError), because . is /totally_unrelated_directory and thus ./exercises/print_last_lines.rb is /totally_unrelated_directory/exercises/print_last_lines.rb, which does not exist.
Instead of loading a script relative to the current working directory (which is what you are doing), what I am guessing you actually meant to do was load a script relative to the currently executing script, and that is exactly what Kernel#require_relative is for.
Note that you have fixed a bug that you didn't even know was there, and you still didn't even have to think. You were just following the Ruby Style Guide.
It is generally bad form to "monkey patch" (i.e. modify) existing modules and classes. Now, above, we added print_last_lines to Kernel based on this advice from the Style Guide:
Private Global Methods
If you really need "global" methods, add them to Kernel and make them private.
So, if we followed the Style Guide in adding print_last_lines to Kernel, then why is the Style Guide also telling us to not modify Kernel? Well, notice the qualifier here [bold italic emphasis mine]:
If you really need "global" methods, add them to Kernel and make them private.
I would argue that we don't "really need" print_last_lines to be global. It could just as well be a module method of some module.
In fact, it is generally good form to have a module that is named the same as your project to enclose all your code in, just to keep it separate from everybody else's code. Something like this:
module ::PrintLastLines
module_function
def print_last_lines(filename, number_of_lines)
# Get last n-lines
lines = ::File.readlines(filename).last(number_of_lines)
# Reverse the order
lines = lines.reverse
lines.map!(&:chomp)
# Concatenate the lines together
lines.join(', ')
end
end
Now, thanks to Module#module_function, if someone really, really wants to have the method available globally, they can Module#include the PrintLastLines module as a mixin:
include PrintLastLines
print_last_lines('/Library/WebServer/Documents/ruby-playground/input.txt', 3)
But, they can also just use
PrintLastLines.print_last_lines('/Library/WebServer/Documents/ruby-playground/input.txt', 3)
instead.
Naming, part 1
We already mentioned naming above. But I believe, there is still more that can be done.
The name print_last_lines is confusing, as the method doesn't actually print anything. So, since it doesn't print, it should not include print in its name.
Conversely, it reverses the output, which is not apparent from the name. In particular, the method performs a function somewhat similar to the POSIX tail utility with the -n parameter and a negative number, but that one returns the output forwards, not reversed. So, the fact that the return value is reversed is somewhat non-intuitive and should therefore be apparent from the name.
I would argue something like last_lines_in_reverse_order is a more intention-revealing name.
Although, I still don't like that name. I would expect a method with the name last_lines_in_reverse_order to return either an Array or an Enumerator, not concatenate the lines together into a String. But I don't understand enough about your use case and your business domain to find a better name.
Personally, I prefer to avoid mutation and side-effects as much as possible. For example, here, you are using mutation:
lines.map!(&:chomp)
And it is not clear why you are using mutation here, when in the very line before that, you aren't:
lines = lines.reverse
In my opinion, you should at least be consistent. Either use
lines.reverse!
lines.map!(&:chomp)
or use
lines = lines.reverse
lines = lines.map(&:chomp)
As mentioned above, I prefer the latter.
Naming, part 2
lines is also not a particularly good name. Sure, it is technically correct, but it doesn't tell the reader anything about why that particular code is written that way, and what the intention of that code is.
Part of the problem is that the variable gets re-assigned multiple times to different things. It is very hard to find a good name for a single variable that gets assigned to multiple things, don't you think?
So, I would propose to not reuse that variable.
This ties partially into the previous section as well: re-assigning a variable is still mutation – it is mutating that variable's binding. Best to avoid that. I prefer the way functional programming views variables: they are immutable names for something rather a label for a portion of memory that can arbitrarily change. Therefore, I prefer to assign variables only once.
Something like this:
# Get last n-lines
last_lines = ::File.readlines(filename).last(number_of_lines)
# Reverse the order
reversed_lines = last_lines.reverse
chomped_lines = reversed_lines.map(&:chomp)
# Concatenate the lines together
chomped_lines.join(', ')
The Ruby Style Guide starts its section on comments off with this gem:
No comments
Write self-documenting code and ignore the rest of this section. Seriously!
And I whole-heartedly agree!
If your code is so complicated that you think it needs a comment to explain it … make your code simpler instead so it doesn't need a comment!
Specifically, comments should never explain how the code does something. Because the code already explains how the code does something. That is, in fact, what code is: a specification for how to do something.
Comments should only ever explain why code does something in a certain, non-obvious, way. Again, ideally, code should be obvious, and I shouldn't need to explain why the code is the way it is – it should be obvious from the code. However, there are certain cases where the obvious way to do something is the wrong way to do it. And in that case, for example, it would make sense to write a comment explaining what the obvious way is, why it doesn't work, and why this particular, non-obvious way, is the correct way.
The most useless kind of comment is a comment that just repeats what the code already says. And unfortunately, there are a couple of those in your code.
For example, here:
# Reverse the order
reversed_lines = last_lines.reverse
How many people, do you think, are unable to understand that reverse reverses? And if someone really doesn't understand that reverse reverses something, do you think repeating that in a comment is really going to help them understand?
Same here:
# Concatenate the lines together
chomped_lines.join(', ')
This comment is only useful for people who know the word "concatenate" but don't know the word "join". How many people do you think those are?
Also, as the Ruby Style Guide says, you need to make sure to keep the comments up-to-date. I don't know if you noticed, but we actually violated this here:
# Get last n-lines
last_lines = ::File.readlines(filename).last(number_of_lines)
We renamed n to number_of_lines, so now the comment talking about n doesn't make sense anymore. That is one of the problems with comments: they don't exist as part of the code. I used the automated Rename Refactoring tool of my Ruby editor to change that name, so that I could be sure it would be changed everywhere where it is used (i.e. both in the method definition and in the method body). But of course the tool couldn't understand that the "n" in the comment also refers to this n, and thus didn't rename it.
And lastly, there is this comment:
# Returns: '9. Line of Text, 8. Line of Text, 7. Line of Text'
Which is also wrong: the script doesn't return this, it prints it, which is a big difference.
In my opinion, none of the comments in your code contribute anything to the understanding, some of them are even misleading, and they should just be removed.
Make the main file executable
In order to make the main file executable, we need to tell the OS how to interpret it. In Windows, this would be done by using the assoc and ftype commands like this:
assoc .rb=RubyScript
ftype RubyScript=ruby.exe %1 %*
Or, more likely, the installer for the Ruby implementation you are using should have done this for you (as well as added ruby.exe to the %PATH).
In a Unixoid environment, this is done using the shebang line in the first line of the file as well as by making sure the executable bit is set for the script.
If we combine all of this, our two files should now look like this:
last_lines_in_reverse_order.rb
# frozen_string_literal: true
module ::LastLinesInReverseOrder
module_function
def last_lines_in_reverse_order(filename, number_of_lines)
last_lines = ::File.readlines(filename).last(number_of_lines)
reversed_lines = last_lines.reverse
chomped_lines = reversed_lines.map(&:chomp)
chomped_lines.join(', ')
end
end
main.rb
#!/usr/bin/env ruby
# frozen_string_literal: true
require_relative 'exercises/last_lines_in_reverse_order'
puts ::LastLinesInReverseOrder.last_lines_in_reverse_order('/Library/WebServer/Documents/ruby-playground/input.txt', 3)
Remove intermediate variables
Personally, I would remove the intermediate variables altogether, and just chain the method calls together.
def last_lines_in_reverse_order(filename, number_of_lines)
::File.readlines(filename).last(number_of_lines).reverse.map(&:chomp).join(', ')
end
It might be more readable to break this across multiple lines. There are two popular styles for how to do this across multiple lines, just pick one, and stick with it. I just use whatever the default is in Rubocop – remember what I wrote above about not needing to think! (Actually, it turns out that Rubocop just formats it back to one line, so just choose one and stick with it.)
Something like this:
def last_lines_in_reverse_order(filename, number_of_lines)
::File.
readlines(filename).
last(number_of_lines).
reverse.
map(&:chomp).
join(', ')
end
Ruby 3 has introduced a simplified method definition syntax for methods whose body consists of a single expression. E.g. instead of
def add(a, b)
a + b
end
You can now write
def add(a, b) = a + b
Personally, I love this style, but the Ruby community seems to be a bit apprehensive. Maybe this is because I am used to functional programming where every function is only a single expression, and specifically, because I am used to Scala where this is exactly how methods are defined.
So, I, personally, would have no problem writing this like this:
def last_lines_in_reverse_order(filename, number_of_lines) =
::File.
readlines(filename).
last(number_of_lines).
reverse.
map(&:chomp).
join(', ')
But the community disagrees.
Documentation
There are now only two things left that Rubocop complains about: the fact that LastLinesInReverseOrder has no documentation and the fact that last_lines_in_reverse_order has no documentation.
I will leave that up to you, but indeed, they both should have documentation.
Large files
This was already mentioned in other answers, but it bears repeating: since you are reading the entire file into memory and constructing an array of all lines, you will run out of memory for very large files, and will hurt performance long before that.
However, you don't need to keep the entire file in memory. All you need to keep in memory is the last number_of_lines lines. You will still have to read the entire file, there is no way around that, since you need to read all the way to the end to know which ones are the last number_of_lines lines. But you can throw away everything before that.
Unfortunately, I haven't been able to come up with an elegant way to do this. The best I found was something like:
def last_lines_in_reverse_order(filename, number_of_lines) =
::File.open(filename) do |file|
lines_sliding_window = file.each_line.lazy.each_cons(number_of_lines)
last_lines = []
loop { last_lines = lines_sliding_window.next }
last_lines
end
.reverse
.map(&:chomp)
.join(', ')
