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/. All the different community style guides in Ruby 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 your case, you are consistent with your string literals, so that is good!
I, personally prefer to use single-quoted strings whenever there is no string interpolation. That way, it is immediately obvious that no string interpolation is taking place.
So, I, personally, would not write
"Anna"
but instead I would write
'Anna'
And so on …
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.
In future versions of Ruby, files that don't have a # frozen_string_literal: magic comment will slowly default to frozen strings, see Feature #20205 Enable frozen_string_literal by default
self is the implicit receiver of any message send that does not explicitly specify a receiver. Therefore, it is not required to specify self as the explicit receiver in most cases.
Therefore,
self.size
should just be
size
For conditionals whose body only contains a single expression, prefer the trailing modifier form.
Replace
if self[i].downcase != self[-(i + 1)].downcase
return false
end
with
return false if self[i].downcase != self[-(i + 1)].downcase
You should never use for/in in Ruby. Always prefer to use each.
In fact, for/in is just syntactic sugar for each anyway, but with weird scoping rules: for/in leaks the loop variable to the outside scope and/or clobbers an existing variable with the same name!
So,
for i in 0..(self.size / 2 - 1)
return false if self[i].downcase != self[-(i + 1)].downcase
end
should instead be
(0..(size / 2 - 1)).each do |i|
return false if self[i].downcase != self[-(i + 1)].downcase
end
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:
# frozen_string_literal: true
class String
def is_palindrome
(0..((size / 2) - 1)).each do |i|
return false if self[i].downcase != self[-(i + 1)].downcase
end
true
end
end
puts 'Anna'.is_palindrome
puts 'civic'.is_palindrome
puts 'Kayak'.is_palindrome
puts 'madam'.is_palindrome
puts 'level'.is_palindrome
puts 'Testing'.is_palindrome
puts 'LoremIpsum'.is_palindrome
puts 'demonstration'.is_palindrome
puts 'Mister'.is_palindrome
puts 'Otto'.is_palindrome
And here are the offenses that Rubocop could not automatically correct:
Inspecting 1 file
C
Offenses:
palindrome.rb:6:3: C: Style/DocumentationMethod: Missing method documentation comment.
def is_palindrome ...
^^^^^^^^^^^^^^^^^
palindrome.rb:6:7: C: Naming/PredicateName: Rename is_palindrome to palindrome?. (https://rubystyle.guide#bool-methods-qmark)
def is_palindrome
^^^^^^^^^^^^^
1 file inspected, 2 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, there are only two offenses left, both of which are trivial to fix, especially in an editor with refactoring support.
In Ruby, a question mark ? is a legal character at the end of a method identifier. Predicates, i.e., methods that answer a yes/no question, should be named with a question mark at the end.
So, by only this rule, is_palindrome should be named is_palindrome?. However, see the next rule as well.
In Ruby, predicate methods should not be prefixed with is, has, can, etc.
Together with the previous rule, this means, is_palindrome should be named palindrome?
That was easy!
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 methods. 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.
There's a linter called Standard Ruby, which is essentially a set of standard configurations for Rubocop that cannot be changed. This takes all the discussions and decisions out of the whole topic.
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:
# frozen_string_literal: true
class String
def palindrome?
(0..(size / 2 - 1)).each do |i|
return false if self[i].downcase != self[-(i + 1)].downcase
end
true
end
end
puts 'Anna'.palindrome?
puts 'civic'.palindrome?
puts 'Kayak'.palindrome?
puts 'madam'.palindrome?
puts 'level'.palindrome?
puts 'Testing'.palindrome?
puts 'LoremIpsum'.palindrome?
puts 'demonstration'.palindrome?
puts 'Mister'.palindrome?
puts 'Otto'.palindrome?
Which I would argue is already more readable than what we started off with, and is a lot more conformant with community style guidelines.
Vertical whitespace
I, personally, would separate the final return value from the rest of the method body by a blank line:
def palindrome?
(0..(size / 2 - 1)).each do |i|
return false if self[i].downcase != self[-(i + 1)].downcase
end
true
end
It is generally bad form to "monkey patch" (i.e. modify) existing modules and classes.
In this case, I don't see a need for monkey patching, and palindrome? could just be a module function instead.
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 ::Palindrome
module_function
def palindrome?(str)
(0..(str.size / 2 - 1)).each do |i|
return false if str[i].downcase != str[-(i + 1)].downcase
end
true
end
end
Now, thanks to Module#module_function, if someone really, really wants to have the method available globally, they can Module#include the Palindrome module as a mixin:
include Palindrome
palindrome?('Otto')
But, they can also just use
Palindrome.palindrome?('Otto')
IFF monkey patching is absolutely, positively, required, there are a couple of things we can do to make it nicer.
Imagine you are a maintenance programmer and you see a piece of unfamiliar code for the very first time which looks like this:
'Otto'.palindrome?
You are wondering where this palindrome? method that you've never heard of comes from, so you ask Ruby herself by using the Method#owner method:
method = 'Otto'.method(:palindrome?)
method.owner
#=> String
Hmm … that's confusing. There is no method named palindrome? mentioned in the documentation of the String class.
Use a clearly named mixin to mark the extension
So, the first thing we can do is to wrap the method into a mixin whose name makes it clear that we are monkey patching a core class and that allows a reader to make an educated guess where to find the documentation:
module ::Palindrome
module StringExtension
def palindrome?
(0..(size / 2 - 1)).each do |i|
return false if self[i].downcase != self[-(i + 1)].downcase
end
true
end
end
end
class ::String
include ::Palindrome::StringExtension
end
Now, if we try the same thing as above, we get a more useful response:
method.owner
#=> Palindrome::StringExtension
And the reader can now "guess" that the documentation for this method, as well as its source code, can probably be found in a file named palindrome/string_extension.rb within a library or Gem called palindrome, at least assuming that the author followed standard Ruby naming and directly layout guidelines.
Note: we could have also used Method#source_location method to find where the method is defined. However, this method does not always work, so our poor maintenance programmer may be forgiven for not trying it in the first place. In particular, on most Ruby implementations, Method#source_location only works for methods that are defined in Ruby.
On the most popular Ruby implementation, YARV, the core String class is written in C, which means source_location wouldn't work for methods defined in the String core class. So, a maintenance programmer might not even try to use source_location on a method they find on a string.
Wrap the extension in a Refinement
Refinements are Ruby's way of controlling the scope of monkey patching. Refinements are lexically scoped, so the monkey patches are only visible within the lexical scope (script, module definition, or eval string) where they are explicitly activated.
module ::Palindrome
module StringExtension
def palindrome?
(0..(size / 2 - 1)).each do |i|
return false if self[i].downcase != self[-(i + 1)].downcase
end
true
end
end
refine ::String do
import_methods ::Palindrome::StringExtension
end
end
This gives the user the option: they can either use the Palindrome::StringExtension mixin to monkey patch String themselves, if they really want to make this monkey patch available globally. Alternatively, they can use the Refinement.
The nice thing about Refinements is that they are only visible if you explicitly activate them:
# Here, the method does not exist:
'Anna'.palindrome?
# in `<main>': undefined method `palindrome?' for an instance of String (NoMethodError)
# I need to explicitly activate the Refinement:
using ::Palindrome
'Anna'.palindrome?
#=> true
Above, I activated the Refinement in the script scope, so it will be visible in the entire script. However, I can also activate the Refinement in class or module scope:
class ::Foo
using ::Palindrome
def self.foo = 'Anna'.palindrome?
end
class ::Bar
def self.bar = 'Anna'.palindrome?
end
::Foo.foo
#=> true
::Bar.bar
# in `bar': undefined method `palindrome?' for an instance of String (NoMethodError)
Personally, I prefer to avoid mutation, explicit iteration, and side-effects as much as possible.
I will just show two options that don't use explicit iteration. Each of these have their individual tradeoffs, and I don't claim either of those is better than yours. They're just "different", and hopefully at least thought-provoking.
A palindrome is the same when read forward and backward
This is the idea that your code is based on, but it can also be expressed more directly:
def palindrome?
word = downcase
word == word.reverse
end
This is a very clear and concise expression of the idea.
Compared to your solution, this is lacking the short-circuiting behavior. In other words, this will always examine every character of the word, even if the first and last character are already different.
I would argue that the simple solution is preferred, unless you have positive proof that it causes a performance bottleneck.
The first and last letters are the same and the rest is a palindrome
This is an alternate way of expressing what a palindrome is. This lends itself nicely to a recursive solution:
def palindrome?
word = downcase
return true if word.empty?
word.first == word.last && word[1...-1].palindrome?
end
This does have the short-circuiting behavior of your solution, but, it would blow the stack for very long words. There is a way to make it tail-recursive, but that makes the method quite a bit longer. The simple transformation would also lose the short-circuiting behavior, and re-introducing that would make it even longer.
Also, it wouldn't actually help because Ruby does not have Proper Tail Calls or Proper Tail Recursion.
I do not think this is a solution I would put out into the world. This is just to demonstrate how the problem can be looked at from a different angle.
Documentation
There are now only three things left that Rubocop complains about: the fact that Palindrome, StringExtension, and palindrome? have no documentation.
The nice thing about Ruby's convention of naming predicate methods with a question mark, is that the method is almost fully self-documenting already: we know because of the question mark that it returns a Boolean, and the name pretty much tells us what it does. So, really, the method documentation is just a formality:
# Tests whether +self+ is a palindrome.
#
# = Performance guarantee
#
# * The method performs at most +self.size / 2+ iterations.
# * The method short-circuits at the first mismatch.
#
# @return [Boolean] wether or not +self+ is a palindrome.
def palindrome?
# …
end
The documentation allows us to document our performance guarantees.
I will leave the documentation for the StringExtension mixin as well as the Palindrome Refinement up to you. It would probably be a good idea to point out that the method will not be automatically monkey patched, and that two options are available to either monkey patch it globally or activate the Refinement.
Testing
It is good that you included test cases with the code! This allows us to more confidently change the code and still being sure we didn't break anything.
It would be even better to move the test code into an actual test suite. There are many different testing frameworks out there for Ruby. I, personally, am a fan of RSpec, but for this simple case, we can use minitest, which is a Default Gem that ships as part of Ruby and thus does not need to be installed:
require 'minitest/autorun'
describe ::Palindrome do
before do
::String.include(::Palindrome::StringExtension)
end
it { _('Anna').must_be :palindrome? }
it { _('civic').must_be :palindrome? }
it { _('Kayak').must_be :palindrome? }
it { _('madam').must_be :palindrome? }
it { _('level').must_be :palindrome? }
it { _('Testing').wont_be :palindrome? }
it { _('LoremIpsum').wont_be :palindrome? }
it { _('demonstration').wont_be :palindrome? }
it { _('Mister').wont_be :palindrome? }
it { _('Otto').must_be :palindrome? }
end
Additional corner cases
I would add some more tests for corner / edge cases, the two I can think oo are:
- The empty string should be a palindrome:
it { _('').must_be :palindrome? }
- A single character string should be a palindrome:
it { _('a').must_be :palindrome? }
