4
\$\begingroup\$

I am not a Ruby developer but cold-reading existing Jekyll plugins I implemented one to bundle files in a zip archive and open sourced it on GitHub.

What do I need to fix to make it look more Ruby-native and/or Jekyll-native?

# Copyright 2021 by Philipp Hasper
# MIT License
# https://github.com/PhilLab/jekyll-zip-bundler

require "jekyll"
require 'zip'
#~ gem 'rubyzip', '~>2.3.0'

module Jekyll
  # Valid syntax:
  # {% zip archiveToCreate.zip file1.txt file2.txt %}
  # {% zip archiveToCreate.zip file1.txt file2.txt %}
  # {% zip archiveToCreate.zip file1.txt folder/file2.txt 'file with spaces.txt' %}
  # {% zip {{ variableName }} file1.txt 'folder/file with spaces.txt' {{ otherVariableName }} %}
  # {% zip {{ variableName }} {{ VariableContainingAList }} %}
  class ZipBundlerTag < Liquid::Tag

    VARIABLE_SYNTAX = %r![^{]*(\{\{\s*[\w\-\.]+\s*(\|.*)?\}\}[^\s{}]*)!mx

    def initialize(tagName, markup, tokens)
      super
      # Split by spaces but only if the text following contains an even number of '
      # Based on https://stackoverflow.com/a/11566264
      # Extended to also not split between the curly brackets of Liquid
      @files = markup.strip.split(%r!\s(?=(?:[^'}]|'[^']*'|{{[^}]*}})*$)!)
    end

    def render(context)
      files = []
      # Resolve the given parameters to a file list
      @files.each do |file|
        matched = file.strip.match(VARIABLE_SYNTAX)
        if matched
          # This is a variable. Look it up.
          resolved = context[file]
          if resolved.respond_to?(:each)
            # This is a collection. Flatten it before appending
            resolved.each do |file|
              files.push(file)
            end
          else
            files.push(resolved)
          end
        elsif file.strip.length > 0
          files.push(file.strip)
        end
      end

      # First file is the target zip archive path
      if files.length < 2
        abort "zip tag must be called with at least two files"
      end
      # Generate the file in the cache folder
      cacheFolder = ".jekyll-cache/zip_bundler/"
      zipfile_path = cacheFolder + files[0]
      FileUtils.makedirs(File.dirname(zipfile_path))

      files_to_zip = files[1..-1]

      # Create the archive. Delete file, if it already exists
      File.delete(zipfile_path) if File.exists?(zipfile_path)
      Zip::File.open(zipfile_path, Zip::File::CREATE) do |zipfile|
        files_to_zip.each do |file|
          # Two arguments:
          # - The name of the file as it will appear in the archive
          # - The original file, including the path to find it
          zipfile.add(File.basename(file), file)
        end
      end
      puts "Created archive #{zipfile_path}"

      # Add the archive to the site's static files
      site = context.registers[:site]
      site.static_files << Jekyll::StaticFile.new(site, site.source + "/" + cacheFolder, File.dirname(files[0]), File.basename(zipfile_path))
      # No rendered output
      ""
    end
  end
end

Liquid::Template.register_tag("zip", Jekyll::ZipBundlerTag)
\$\endgroup\$
1

2 Answers 2

3
\$\begingroup\$

Consistency

Sometimes you use snake_case, sometimes you use camelCase for methods and local variables. Sometime you use single-quoted strings and sometimes you use double-quoted strings.

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).

Single-quoted strings

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 "jekyll"

should instead be

require 'jekyll'

Note that on the very next line, you use

require 'zip'

That is inconsistent and it trips up the reader, because they are left wondering what the difference is between zip and jekyll. Why did you choose to enclose one in single quotes and one in double quotes? What does that mean? What are you trying to tell us?

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 "Philipp's Jekyll plugin"

that reads much better than

puts 'Philipp\'s Jekyll plugin'

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.

Freeze objects assigned to constants

As mentioned above, you should prefer as much as possible to use purely functional code, immutable data, and immutable bindings.

Here, you assign a mutable value to a constant:

VARIABLE_SYNTAX = %r![^{]*(\{\{\s*[\w\-\.]+\s*(\|.*)?\}\}[^\s{}]*)!mx

This could be somewhat confusing because while the object is assigned to a constant, the object itself can still be changed. It is better to freeze objects assigned to constants:

VARIABLE_SYNTAX = %r![^{]*(\{\{\s*[\w\-\.]+\s*(\|.*)?\}\}[^\s{}]*)!mx.freeze

Prefer / for Regexp literals

It is generally preferred to use / to delimit Regexp literals unless you need to use / within your Regexp literal:

VARIABLE_SYNTAX = /[^{]*(\{\{\s*[\w\-\.]+\s*(\|.*)?\}\}[^\s{}]*)/mx.freeze

Superfluous escape in the Regexp literal

There is no need to escape the . character in a character class: it has no special meaning inside a character class.

VARIABLE_SYNTAX = /[^{]*(\{\{\s*[\w\-.]+\s*(\|.*)?\}\}[^\s{}]*)/mx.freeze

Also, you don't need to escape - in a character class if it is the first or last character in the class:

VARIABLE_SYNTAX = /[^{]*(\{\{\s*[\w.-]+\s*(\|.*)?\}\}[^\s{}]*)/mx.freeze

Superfluous option in the Regexp literal

You are using the x eXtended mode option, which allows you to write the Regexp in multiple lines and add comments to it … but you are not actually doing any of that. So, it is just extra fluff that can be removed

VARIABLE_SYNTAX = /[^{]*(\{\{\s*[\w.-]+\s*(\|.*)?\}\}[^\s{}]*)/m.freeze

Prefer predicate methods over relational operators

You should prefer "speaking" predicate methods such as Numeric#negative? over relational operators such as < 0.

So, this:

elsif file.strip.length > 0

should be

elsif file.strip.length.positive?

Prefer predicate methods over explicit length checks

As mentioned above, you should prefer predicates over relational operators. But in this specific case, we can even do one better, we can use String#empty?:

elsif file.strip.length.positive?

can just be

elsif !file.strip.empty?

Space after # in comments

There should be one space after the # in a comment:

#~ gem 'rubyzip', '~>2.3.0'

should be

# ~ gem 'rubyzip', '~>2.3.0'

Actually, it looks like you just commented out a line of code. You can just delete it. If you want it back, that's what Version Control Systems are for.

No empty line after opening keyword

There should be no empty line after the opening module, class, def, do, while, until, if, unless, case, etc.

Which means there should be no empty line here:

class ZipBundlerTag < Liquid::Tag

  VARIABLE_SYNTAX = /[^{]*(\{\{\s*[\w.-]+\s*(\|.*)?\}\}[^\s{}]*)/m.freeze

it should just be

class ZipBundlerTag < Liquid::Tag
  VARIABLE_SYNTAX = /[^{]*(\{\{\s*[\w.-]+\s*(\|.*)?\}\}[^\s{}]*)/m.freeze

Modifier if

If the body of a conditional expression is only a single expression, you can use the modifier form of the conditional.

For example, you can use that here:

if files.length < 2
  abort 'zip tag must be called with at least two files'
end

could be

abort 'zip tag must be called with at least two files' if files.length < 2

Use of deprecated / removed methods

File::exists? is deprecated, and has been for many years. Its documentation literally reads, in its entirety:

Deprecated method. Don't use.

In fact, in current versions of Ruby, the method actually does not exist anymore. This means that in current versions of Ruby, your code will fail with a NoMethodError exception!

You should use File::exist? instead.

Prefer string interpolation over concatenation

site.source + '/' + cacheFolder

should be

"#{site.source}/{cacheFolder}"

Although in this particular case, since you are actually constructing a path, you could also use one of the methods dedicated to that purpose, like File::join.

You already use similar methods for finding the name of the directory and the base name.

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 almost 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".

In particular, running Rubocop on your code, it detects 29 offenses, of which it can automatically correct 19.

Here's what the result of the auto-fix looks like:

# frozen_string_literal: true

# Copyright 2021 by Philipp Hasper
# MIT License
# https://github.com/PhilLab/jekyll-zip-bundler

require 'jekyll'
require 'zip'
# ~ gem 'rubyzip', '~>2.3.0'

module Jekyll
  # Valid syntax:
  # {% zip archiveToCreate.zip file1.txt file2.txt %}
  # {% zip archiveToCreate.zip file1.txt file2.txt %}
  # {% zip archiveToCreate.zip file1.txt folder/file2.txt 'file with spaces.txt' %}
  # {% zip {{ variableName }} file1.txt 'folder/file with spaces.txt' {{ otherVariableName }} %}
  # {% zip {{ variableName }} {{ VariableContainingAList }} %}
  class ZipBundlerTag < Liquid::Tag
    VARIABLE_SYNTAX = /[^{]*(\{\{\s*[\w\-.]+\s*(\|.*)?\}\}[^\s{}]*)/mx.freeze

    def initialize(tagName, markup, tokens)
      super
      # Split by spaces but only if the text following contains an even number of '
      # Based on https://stackoverflow.com/a/11566264
      # Extended to also not split between the curly brackets of Liquid
      @files = markup.strip.split(/\s(?=(?:[^'}]|'[^']*'|{{[^}]*}})*$)/)
    end

    def render(context)
      files = []
      # Resolve the given parameters to a file list
      @files.each do |file|
        matched = file.strip.match(VARIABLE_SYNTAX)
        if matched
          # This is a variable. Look it up.
          resolved = context[file]
          if resolved.respond_to?(:each)
            # This is a collection. Flatten it before appending
            resolved.each do |file|
              files.push(file)
            end
          else
            files.push(resolved)
          end
        elsif file.strip.length.positive?
          files.push(file.strip)
        end
      end

      # First file is the target zip archive path
      abort 'zip tag must be called with at least two files' if files.length < 2
      # Generate the file in the cache folder
      cacheFolder = '.jekyll-cache/zip_bundler/'
      zipfile_path = cacheFolder + files[0]
      FileUtils.makedirs(File.dirname(zipfile_path))

      files_to_zip = files[1..-1]

      # Create the archive. Delete file, if it already exists
      File.delete(zipfile_path) if File.exist?(zipfile_path)
      Zip::File.open(zipfile_path, Zip::File::CREATE) do |zipfile|
        files_to_zip.each do |file|
          # Two arguments:
          # - The name of the file as it will appear in the archive
          # - The original file, including the path to find it
          zipfile.add(File.basename(file), file)
        end
      end
      puts "Created archive #{zipfile_path}"

      # Add the archive to the site's static files
      site = context.registers[:site]
      site.static_files << Jekyll::StaticFile.new(site, "#{site.source}/#{cacheFolder}", File.dirname(files[0]),
                                                  File.basename(zipfile_path))
      # No rendered output
      ''
    end
  end
end

Liquid::Template.register_tag('zip', Jekyll::ZipBundlerTag)

And here are the offenses that Rubocop could not automatically correct:

Inspecting 1 file
W

Offenses:

zip_bundler.rb:21:20: C: Naming/MethodParameterName: Only use lowercase characters for method parameter.
    def initialize(tagName, markup, tokens)
                   ^^^^^^^
zip_bundler.rb:21:20: C: Naming/VariableName: Use snake_case for variable names.
    def initialize(tagName, markup, tokens)
                   ^^^^^^^
zip_bundler.rb:29:5: C: Metrics/AbcSize: Assignment Branch Condition size for render is too high. [<11, 36, 10> 38.95/17]
    def render(context) ...
    ^^^^^^^^^^^^^^^^^^^
zip_bundler.rb:29:5: C: Metrics/CyclomaticComplexity: Cyclomatic complexity for render is too high. [9/7]
    def render(context) ...
    ^^^^^^^^^^^^^^^^^^^
zip_bundler.rb:29:5: C: Metrics/MethodLength: Method has too many lines. [32/10]
    def render(context) ...
    ^^^^^^^^^^^^^^^^^^^
zip_bundler.rb:29:5: C: Metrics/PerceivedComplexity: Perceived complexity for render is too high. [11/8]
    def render(context) ...
    ^^^^^^^^^^^^^^^^^^^
zip_bundler.rb:39:31: W: Lint/ShadowingOuterLocalVariable: Shadowing outer local variable - file.
            resolved.each do |file|
                              ^^^^
zip_bundler.rb:53:7: C: Naming/VariableName: Use snake_case for variable names.
      cacheFolder = '.jekyll-cache/zip_bundler/'
      ^^^^^^^^^^^
zip_bundler.rb:54:22: C: Naming/VariableName: Use snake_case for variable names.
      zipfile_path = cacheFolder + files[0]
                     ^^^^^^^^^^^
zip_bundler.rb:73:75: C: Naming/VariableName: Use snake_case for variable names.
      site.static_files << Jekyll::StaticFile.new(site, "#{site.source}/#{cacheFolder}", File.dirname(files[0]),
                                                                          ^^^^^^^^^^^

1 file inspected, 10 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 76 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 to 48. Running Rubocop as described above, further reduces this to 38, and as mentioned, lots of these are duplicates because I have multiple different linters and analyzers configured. I would say about 16 are unique.

Use snake_case for local variables

Methods, local variables, instance variables, class variables, global variables, and parameters should use snake_case naming convention. You are jumping back and fort between camelCase and snake_case, for example here:

cacheFolder = '.jekyll-cache/zip_bundler/'
zipfile_path = cacheFolder + files[0]

This should be

cache_folder = '.jekyll-cache/zip_bundler/'
zipfile_path = cache_folder + files[0]

With a good editor or IDE, you should be able to fix this fairly easily with the Rename Variable Refactoring.

This brings the number of Rubocop offenses down to 5 and the number of notices in my editor down to 26.

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.

However, if we look at the remaining notices we get from Rubocop and the other linters and analyzers, it is clear that we will have to apply some brains to get rid of them:

List of notices for the zip_bundler.rb file

Duplicate code

Some of these are still fairly easy to get rid of, in particular the ones about duplicate code.

For example, files[0] is really the target. You even have a comment that says so:

# First file is the target zip archive path

So, let's just make that clear:

target = files.first

And now we can replace all occurrences of files[0] with target, which not only gets rid of the notices in the editor, but also makes the code more intention-revealing.

The same applies to the file.strip. We could simply assign something like stripped_file = file.strip and replace all the mentions of file.strip with stripped_file. But in my opinion, it would be even better to fix the problem at the source, and make sure that the @files variable only contains pre-stripped files.

So, we change the initialize method to

@files = markup.strip.split(/\s(?=(?:[^'}]|'[^']*'|{{[^}]*}})*$)/).map(&:strip)

Note: it might also be possible to tweak the Regexp so that there is nothing to strip in the first place.

The same applies to the file.empty? check. We can either remove empty strings right here:

@files = markup.strip.split(/\s(?=(?:[^'}]|'[^']*'|{{[^}]*}})*$)/).map(&:strip).reject(&:empty?)

or maybe tweak the Regexp in order to not produce empty strings in the first place.

Better design

Unfortunately, getting rid of most of the other notices and offenses requires a level of domain knowledge of Liquid that I do not have.

For example, I don't know what kinds of collections can appear at this point:

if resolved.respond_to?(:each)
  # This is a collection. Flatten it before appending
  resolved.each do |file|
    files.push(file)
  end

If these collections respond to to_ary, then we can replace the whole loop with a map + flatten, something like this:

target, files = @files.map do |file|
  next file unless file.match(VARIABLE_SYNTAX)

  # This is a variable. Look it up.
  context[file]
end.flatten

Splitting code

You have already put in some comments explaining the individual steps. As a very general rule, whenever you write a comment, you should take that as an opportunity to take a step back and think about why you are writing that comment in the first place.

In particular,

  • if you are writing the comment because the code is too complex, then make the code simpler and the comment is no longer needed
  • if you are writing the comment to explain what something is, then give it a better name (or give it a name in the first place if it doesn't have one) and the comment is no longer needed
  • if you are writing the comment to point out logical breaks in the code, then split the code at that point and the comment is no longer needed
  • if you are writing the comment to explain what the code does, then really the code should explain what the code does instead, and the comment should not be needed

The main reason to have a comment is to explain why a piece of code does something in a certain way that may be non-obvious.

So, looking at the comments that delineate the separate steps, maybe we can split the code at those points and actually make each step a separate method.

It would look somewhat like this:

def render(context)
  # First file is the target zip archive path
  target, files = resolve_parameters(context)

  abort 'zip tag must be called with at least two files' if files.empty?

  zipfile_path = CACHE_FOLDER + target

  klass = self.class
  klass.create_directory(zipfile_path)
  klass.create_archive(files, zipfile_path)
  klass.register_files(context, target, zipfile_path)

  # No rendered output
  ''
end

private

def resolve_parameters(context)
  target, files = @files.map do |file|
    next file unless file.match(VARIABLE_SYNTAX)

    # This is a variable. Look it up.
    context[file]
  end

  [target, files]
end

private_class_method def self.create_directory(zipfile_path)
  FileUtils.makedirs(File.dirname(zipfile_path))
end

private_class_method def self.create_archive(files, zipfile_path)
  File.delete(files, zipfile_path) if File.exist?(zipfile_path)
  Zip::File.open(zipfile_path, Zip::File::CREATE) do |zipfile|
    files.each do |file|
      # Two arguments:
      # - The name of the file as it will appear in the archive
      # - The original file, including the path to find it
      zipfile.add(File.basename(file), file)
    end
  end
  puts "Created archive #{zipfile_path}"
end

private_class_method def self.register_files(context, target, zipfile_path)
  # Add the archive to the site's static files
  site = context.registers[:site]
  site.static_files << Jekyll::StaticFile.new(site, File.join(site.source, CACHE_FOLDER), File.dirname(target),
                                              File.basename(zipfile_path))
end

As mentioned above, I don't have enough domain knowledge in Jekyll or Liquid to make any suggestions about a fundamentally better design. Maybe there are some helper methods that could be used. For example, I cannot imagine that you have to parse variables yourself. (But I was wrong, the documentation actually explicitly says that you have to do that.)

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for this very extensive answer! I have applied most of the suggestions and added a CI to check for them (github.com/PhilLab/jekyll-zip-bundler/pull/1/files). I did not fully do the code splitting part because my personal tolerance towards method length is a little higher. But I definitely will keep these stricter constraints in mind in the future. \$\endgroup\$
    – PhilLab
    Commented Nov 1, 2021 at 13:58
3
\$\begingroup\$

Jörg already provided some good suggestions. The only thing I would add is that your render function contains all of your business logic and is quite difficult to understand. It could benefit from splitting it up a bit into helper methods. However, the render function accepts the context parameter which makes this a bit difficult. Therefore I would suggest to extract a (nested) class which gets the context and files as attributes.

Something like this

class ZipBundlerTag < Liquid::Tag
  def initialize(tagName, markup, tokens)
    super
    # Split by spaces but only if the text following contains an even number of '
    # Based on https://stackoverflow.com/a/11566264
    # Extended to also not split between the curly brackets of Liquid
    @files = markup.strip.split(%r!\s(?=(?:[^'}]|'[^']*'|{{[^}]*}})*$)!)
  end

  def render(context)
    ZipBundlerTagRender.new(context, @files).create
  end
end

and then you can do something like this

class ZipBundlerTagRender
  CACHE_FOLDER = ".jekyll-cache/zip_bundler/".freeze
  VARIABLE_SYNTAX = %r![^{]*(\{\{\s*[\w\-\.]+\s*(\|.*)?\}\}[^\s{}]*)!mx

  def initialize(context, params)
    @context = context
    @params = params
    @site = context.registers[:site]
  end

  def create
    FileUtils.makedirs(File.dirname(zipfile_path))
    File.delete(zipfile_path) if File.exists?(zipfile_path)
    create_archive!
    site.static_files << Jekyll::StaticFile.new(site, site.source + "/" + CACHE_FOLDER, File.dirname(files[0]), File.basename(zipfile_path))
  end

  private

  attr_reader :context, :params, :site

  def create_archive
    Zip::File.open(zipfile_path, Zip::File::CREATE) do |zipfile|
      files_to_zip.each do |file|
        zipfile.add(File.basename(file), file)
      end
    end
  end

  def files_to_zip
    files[1..-1]
  end

  def zipfile_path
    CACHE_FOLDER + files[0]
  end

  def file_names
    @file_names ||= fetch_file_names
  end

  def fetch_file_names
    params.map do |file|
      if file.strip.match(VARIABLE_SYNTAX)
        files.push(context[file])
      elsif file.strip.length > 0
        files.push(file.strip)
      end
    end.flatten
  end
end
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for this answer which really highlights how to modularize the code better. I have improved the code at github.com/PhilLab/jekyll-zip-bundler but I did not introduce a second class and not all of the separate helper methods. Given the simplicity of the plugin's task, it personally felt better being able to just read from top to bottom without frequently jumping between methods. However, if I will need to extend (or unit test) the plugin, I might introduce the code splits you proposed. This was definitely helpful! \$\endgroup\$
    – PhilLab
    Commented Nov 1, 2021 at 14:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.