6
\$\begingroup\$

I'm working on a DSL for web development, similar to sinatra. The git repository is here. I've been attempting to improve this code and write it for 4 months, and as a hobby programmer, I would like to have an evaluation of general readability/maintainability, and any ways I could more logically split up my code into files. I have a test suite which I would be happy to add to the question if that is part of the norm here.

So mostly I'm looking for a code review to improve my code style, general organization (how the methods are ordered and how the files are split up), commenting practices (are my comments informative enough, are they too verbose), and general code readability. Please let me know if I should change anything to better fit the norms of this site, this is my first time here.

My goal is for the code to be readable enough that I don't have to provide a README to describe how it works, but I do have one currently if that would be helpful.

The code is for a rubygem based on rack. Here is an example usage (more examples can be found in the README):

require "atd"

request "/", "index.html" #=> for any request to / return index.html

request "/home" do
  puts "/home was requested"
  @http[:output] = "This is home!"
end

atd.rb

require_relative "atd/version"
require "rack"
require "webrick"
require_relative "atd/builtin_class_modifications"
require_relative "atd/routes"
# Extension packs
# require_relative "extensions/precompilers"

# The assistant technical director of your website. It does the dirty work so you can see the big picture.
module ATD
  # Creates a new ATD App based on the template of {ATD::App}.
  # @return [ATD::App]
  # @param [Symbol] name The name of the new app and new class generated.
  def new(name)
    app = Class.new(App)
    Object.const_set(name.to_sym, app)
    app
  end

  # So called because each instance stores a route, and will be called if that route is reached.
  # A route for the purposes of {ATD} is a parser that will be fed env in {ATD::App#call the rack app}.
  class Route
    attr_accessor :args, :method, :block, :path, :output, :app, :actions, :status_code

    # The first two arguments must me the path and the output.
    def initialize(*args, &block)
      @status_code = 200
      @method = [:get, :post, :put, :patch, :delete]
      @method = [] if args.last.is_a?(Hash) && !(args.last[:respond_to].nil? || args.last[:ignore].nil?)
      @app = :DefaultApp
      parse_args(*args, &block)
    end

    # This works differently from a standard setter because is makes sure that a {Route} can belong to only one {App}.
    def app=(app_name)
      old_app = Object.const_get(@app)
      new_app = Object.const_get(app_name.to_sym)
      old_app.routes -= self if old_app.routes.is_a?(Array) && old_app.routes.include?(self)
      new_app.routes.nil? ? new_app.routes = Array(self) : new_app.routes += Array(self)
      @app = app_name
    end

    # @!method get(path = nil,*args)
    # @param [String] path The path at which the route should receive from.
    # @return ATD::Route
    # Sets route to receive a get request to path and execute the block provided (if one is provided)

    # @!method post(path = nil,*args)
    # @param [String] path The path at which the route should receive from.
    # @return ATD::Route
    # Sets route to receive a post request to path and execute the block provided (if one is provided)

    # @!method put(path = nil,*args)
    # @param [String] path The path at which the route should receive from.
    # @return ATD::Route
    # Sets route to receive a put request to path and execute the block provided (if one is provided)

    # @!method patch(path = nil,*args)
    # @param [String] path The path at which the route should receive from.
    # @return ATD::Route
    # Sets route to receive a patch request to path and execute the block provided (if one is provided)

    # @!method delete(path = nil,*args)
    # @param [String] path The path at which the route should receive from.
    # @return ATD::Route
    # Sets route to receive a delete request to path and execute the block provided (if one is provided)

    [:get, :post, :put, :delete, :patch].each do |method|
      define_method(method) do |*args, &block|
        # This conditional allows the syntax get post put "/", "Hello" because it passes
        # the variables up through the different method calls.
        if args.first.is_a?(ATD::Route)
          @method = args.first.method
          @output = args.first.output
          @path = args.first.path
          @args = args.first.args
          @block = args.first.block
          @app = args.first.app
          @actions = args.first.actions
        end
        @method = [method] if @method.length == 5
        @method += [method]
        @method.uniq!
        parse_args(*args, &block)
      end
    end

    # Converts an instance of {ATD::Route} into it's Hash representation.
    # The format for the Hash is listed {ATD::App#initialize here}
    # @api private
    def to_h
      routes = {}
      routes[@path] = {}
      routes[@path][@method] = {}
      routes[@path][@method] = {
        status_code: @status_code,
        output: @output,
        block: @block,
        args: @args,
        route: self
      }
      routes
    end

    private

    # This should also manage @method at some point
    def parse_args(*args, &block)
      args.compact!
      args.flatten!
      args.reject! { |arg| arg.is_a?(File) || arg.is_a?(Proc) || arg ? false : arg.empty? } # File doesn't respond to empty
      @block = block
      # This requires the format ATD::Route.new(path, route, args)
      @path ||= args.shift
      @output ||= args.shift
      @args = Array(@args).concat(args) unless args.nil?
      # @output should be whatever the input is unless the input is a controller/action or the input is_file_string?
      if @output =~ /^\w*#\w*$/ # Check if @path is a controller#action combo
        controller, action = @output.split("#")
        @action = Object.const_get(controller.to_sym).method(action.to_sym)
        @output = @action.call
      end
      # TODO: Choose one! They all work... I think...
      # Method 1:
      target_location = []
      caller_locations.each do |caller_location|
        target_dir = File.dirname(caller_location.absolute_path.to_s)
        target_location.push(target_dir) unless target_dir.include?(__dir__)
      end
      # Method 2:
      target_location = caller_locations.reject do |caller_location|
        File.dirname(caller_location.absolute_path.to_s).include? __dir__
      end
      output_full_path = "#{File.dirname(target_location[0].absolute_path)}/assets/#{@output}"
      @output = File.new(output_full_path) if File.exist?(output_full_path) && !Dir.exist?(output_full_path)
      if args.is_a?(Hash) || args.last.is_a?(Hash)
        @method += Array(args.last[:respond_to]) unless args.last[:respond_to].nil?
        @method -= Array(args.last[:ignore]) unless args.last[:ignore].nil?
        @status_code = args.last[:status] unless args.last[:status].nil?
        @status_code = args.last[:status_code] unless args.last[:status_code].nil?
      end
      self
    end
  end

  # A template {App} that all Apps extend. When a new App is created with {ATD.new ATD.new} it extends this class.
  class App
    attr_accessor :http
    class << self
      attr_accessor :routes # An array of instances of {ATD::Route} that belong to this {App}.

      # Generates an instance of {ATD::Route}.
      # Passes all arguments and the block to {Route.new the constructor} and sets the app where it was called from.
      def request(*args, &block)
        route = ATD::Route.new(*args, &block)
        route.app = (self == Object || self == ATD::App ? :DefaultApp : name.to_sym)
        route
      end
      alias req request
      alias r request

      [:get, :post, :put, :patch, :delete].each do |i|
        define_method(i) do |*args, &block|
          request.send(i, *args, &block) # Makes get == r.get, post == r.post, etc.
        end
      end

      # Starts the rack server
      # @param [Class] server The server that you would like to use.
      # @param [Fixnum] port The port you would like the server to run on.
      def start(server = WEBrick, port = 3150)
        Rack::Server.start(app: new, server: server, Port: port)
      end
    end

    # Sets up the @routes instance variable from the {.routes} class instance variable.
    # Can be passed an array of instances of {ATD::Route} and they will be added to @routes.
    # The format of the new @routes instance variable is:
    #   {"/" => {
    #            get: {output: "Hello World",
    #                  block: Proc.new},
    #            post: {output: "Hello World",
    #                   block: Proc.new}
    #           },
    #   "/hello" => {
    #               get: {output: "Hello World",
    #                     block: Proc.new},
    #               post: {output: "Hello World",
    #                       block: Proc.new
    #                      }
    #               }
    #   }
    # @param [Array] routes An array of instances of {ATD::Route}.
    def initialize(routes = [])
      @routes = {}
      Array(routes + self.class.routes).each do |route|
        route = route.clone
        filename = ATD::Compilation.precompile(route, (route.args.last.is_a?(Hash) ? route.args.last[:precompile] : nil))
        route_hash = route.to_h
        current_route = route_hash[route.path][route.method]
        current_route[:filename] = filename
        block = current_route[:block]
        # An instance method must be defined from the block make it the same as the controller actions. We don't want to
        # convert the controller actions to blocks because if we did that, we would have to take them out of scope to allow
        # them to use the @http variables.
        current_route[:block] = define_singleton_method(block.object_id.to_s.tr("0-9", "a-j").to_sym, &block) unless block.nil?
        current_route[:block] = route.actions unless route.actions.nil?
        @routes = @routes.to_h.deep_merge(route_hash)
      end
    end

    # Allows instance method route creation. Just another way of creating routes.
    def request(*args, &block)
      route = ATD::Route.new(*args, &block)
      filename = ATD::Compilation.precompile(route, (route.args.last.is_a?(Hash) ? route.args.last[:precompile] : nil))
      route_hash = route.to_h
      route_hash[route.path][route.method][:filename] = filename
      @routes = @routes.to_h.deep_merge(route_hash)
      route
    end
    alias req request
    alias r request

    # Starts the rack server
    # @param [Class] server The server that you would like to use.
    # @param [Fixnum] port The port you would like the server to run on.
    def start(server = WEBrick, port = 3150)
      Rack::Server.start(app: self, server: server, Port: port)
    end

    # This is the method which responds to .call, as the Rack spec requires.
    # It will return status code 200 and whatever output corresponds the that route if it exists, and if it doesn't
    # it will return status code 404 and the message "Error 404"
    def call(env)
      @http = nil
      route = route(env)
      return error(404) if route.nil?
      route[:output] = Compilation.compile(route[:filename], route[:output]) unless !route[:args].nil? && !route[:args].empty? && route[:args][0].is_a?(Hash) && route[:args][0][:compile] == false
      return [route[:status_code].to_i, Hash(route[:headers]), Array(route[:output])] if route[:block].nil?
      http output: route[:output], request: Rack::Request.new(env), method: env["REQUEST_METHOD"], response: Rack::Response.new(env)
      return_val = method(route[:block]).call
      @http[:output] = return_val if @http[:output].nil?
      [@http[:status_code].to_i, Hash(@http[:headers]), Array(@http[:output])]
    end

    private

    def route(env)
      return nil if @routes[env["PATH_INFO"]].nil?
      # return @routes[env["PATH_INFO"]][[]] unless @routes[env["PATH_INFO"]][[]].nil?
      @routes[env["PATH_INFO"]].include_in_key?(env["REQUEST_METHOD"].downcase.to_sym)
    end

    def http(additional_params)
      @http = { status_code: 200, headers: {} }.merge(additional_params)
    end

    def error(number)
      [number, {}, ["Error #{number}"]]
    end
  end
  module_function :new
end

# @return [ATD::Route]
def request(*args, &block)
  ATD::App.request(args, block)
end
alias req request
alias r request

# Starts the rack server
# @param [Class] app The app you would like to start
# @param [Class] server The server that you would like to use.
# @param [Fixnum] port The port you would like the server to run on.
def start(app = DefaultApp, server = WEBrick, port = 3150)
  Rack::Server.start(app: app.new, server: server, Port: port)
end

[:get, :post, :put, :patch, :delete].each do |i|
  define_method(i) do |*args, &block|
    request.send(i, *args, &block)
  end
end

Object.const_set(:DefaultApp, Class.new(ATD::App)) # Create DefaultApp

atd/builtin_class_modifications.rb

# @!visibility private
class Hash
  # Not only merges two hashes, but also merges the hashes that may be nested in.
  #
  # For example:
  #   {a: {b: "c"}}
  # Is a nested hash
  def deep_merge(second)
    merger = proc do |_, v1, v2|
      if v1.is_a?(Hash) && v2.is_a?(Hash) then v1.merge(v2, &merger)
      elsif v1.is_a?(Array) && v2.is_a?(Array) then v1 | v2
      elsif [:undefined, nil, :nil].include?(v2) then v1
      else v2
      end
    end
    merge(second.to_h, &merger)
  end

  def include_in_key?(search)
    each do |key, val|
      return val if key.is_a?(Array) && key.include?(search)
    end
  end
end

# This method only exists for the test suite, specifically {ATDTest#test_route_creation}.
# @!visibility private
class Object
  # Checks if two objects are instances of the same class and that they have the same instance variables
  def same_properties_as?(other_class)
    other_class.class == self.class && class_instance_variables == other_class.class_instance_variables
  end

  # Returns the instance variables of a class
  def class_instance_variables
    instance_variables.map { |var| [var, instance_variable_get(var)] }.to_h
  end
end

atd/routes.rb

module ATD
  # This module holds everything related to the compilation of routes.
  module Compilation
    # A module designed to hold all the precompilation methods
    module Precompiler
      extend self

      # Lists all filestypes that have defined precompiler methods
      def filetypes
        instance_methods(true) - [:filetypes]
      end
    end

    # A module designed to hold all the compilation methods
    module Compiler
      extend self

      # Lists all file extentions which have defined compiler methods
      def filetypes
        instance_methods(true) - [:filetypes]
      end
    end

    # This method is responsible for live compilation. It takes an ATD::Route as input, and returns either
    # the filename if Route.output is a file or the Route.output string if Route.output is a string.
    # It will also take the file and call the corresponding compilation method on it.
    def self.compile(name, contents)
      return contents if name.nil?
      contents = File.read(contents) if contents.is_a? File
      parse(Compiler, name, contents)
    end

    # This method is responsible for precompilation. It takes an ATD::Route as input, and returns either
    # the filename if Route.output is a file or the Route.output string if Route.output is a string.
    # It will also take the file and call the corresponding precompilation method on it.
    # route.output is either a full, expanded file path, a file, or a string
    def self.precompile(route, *opts)
      return nil if route.output.nil?
      if route.output.is_a?(File)
        name = route.output.is_a?(File) ? File.basename(route.output) : route.output.dup
        file = route.output.is_a?(File) ? route.output.dup : File.new(route.output)
        route.output = parse(Precompiler, name, File.read(file)) if opts[0].nil? || opts[0]
        return name
      end
      route.output
    end

    class << self
      private

      def parse(type, name, contents)
        name = name.split(".")
        extensions = name - [name.first]
        extensions.each do |extension|
          if type.filetypes.include? extension.to_sym
            contents = type.send(extension, contents)
            extensions -= [extension]
          end
        end
        contents
      end
    end
  end
end

class Object
  include ATD::Compilation::Compiler
end
\$\endgroup\$
4
\$\begingroup\$

Things you've done well

There's a lot to like here. This is good code, easy to read and understand. I especially like:

  • Comments
  • Standard formatting (two-space indent, etc.)
  • Short methods

A little more vertical white space

Comment blocks that preceed a method, module, or class definition should be preceded by a blank line. So instead of this:

module Foo
  # Something about Bar
  class Bar
    # Something about baz
    def baz

I prefer this:

module Foo

  # Something about Bar
  class Bar

# Something about baz
def baz

This helps to set methods, classes and definitions apart visually.

Prefer lines shorter than 80 characters.

Long lines cause the code to be hard to read when someone is using an editor with a narrower window than you used, or when the code is printed, or when it is displayed on a stackexchange site (witness the horizontal scroll bars above). For that reason, prefer lines shorter than 80 characters.

Note: This is an opinion not universally held by Ruby programmers.

use alias method instead of alias

alias_method is generally preferred over alias. So, instead of:

alias req request

use:

alias_method :req :request

Use the Forwardable module

You did a good job using metaprogramming to define methods for you:

 [:get, :post, :put, :patch, :delete].each do |i|
   define_method(i) do |*args, &block|
     request.send(i, *args, &block) # Makes get == r.get, post == r.post, etc.
   end
 end

For simple forwarding methods like this, there's a better way. Ruby's Fowardable module

will do it:

require 'forwardable'
...
  extend Forwardable
  ...
  delegate %i[get put patch delete] => :request

Dead code

In this method:

# This method is responsible for precompilation. It takes an
# ATD::Route as input, and returns either the filename if
# Route.output is a file or the Route.output string if
# Route.output is a string.  It will also take the file and call
# the corresponding precompilation method on it.  route.output is
# either a full, expanded file path, a file, or a string
def self.precompile(route, *opts)
  return nil if route.output.nil?
  if route.output.is_a?(File)
    name = route.output.is_a?(File) ? ...
    file = route.output.is_a?(File) ? ...
  end
  route.output
end

The inner checks for route.output.is_a?(File) are redundant: The outer if has already determined that.

Type checks are not always the best way to do things

Note that checking an object's type is a code smell in Ruby. It's sometimes necessary, but often it is not. It can be preferable to ask the object if it responds to the behavior you want to use:

`some_object.respond_to?(:some_method)`

It can be even better to turn primitive objects into your own polymorphic classes that all behave the same way, so that no type or behavior check is needed at all. When doing this, a type or behavior check is often still needed, but only in the factory method used to create the first-class object.

For more on this, see Confident Ruby by Advi Grimm.

For an example of how this might work in practice, let's look at a redacted version of the #initialize method from the usps_intelligent_barcode gem:

# Create a new barcode
#
# @param routing_code [String] Nominally a String, but can be
#   anything that {RoutingCode.coerce} will accept.
def initialize(routing_code)
  @routing_code = RoutingCode.coerce(routing_code)
end

RoutingCode::coerce does whatever it can to convert an object to a RoutingCode instance. This is where type checks are done in this code, but it's the only place. The rest of the code gets to work with a RoutingCode, without worrying how it came to be:

# Turn the argument into a RoutingCode if possible.  Accepts:
# * {RoutingCode}
# * nil (no routing code)
# * String of length:
#   * 0 - no routing code
#   * 5 - zip
#   * 9 - zip + plus4
#   * 11 - zip + plus4 + delivery point
# * Array of [zip, plus4, delivery point]
# @return [RoutingCode]

def self.coerce(o)
  case o
  when nil
    coerce('')
  when RoutingCode
    o
  when Array
    RoutingCode.new(*o)
  when String
    RoutingCode.new(*string_to_array(o))
  else
    raise ArgumentError, 'Cannot coerce to RoutingCode'
  end
end

Semantic Versioning

Have you thought of using semantic versioning?. It provides a way for your version numbers to clearly communicate when your change to the library might break client code, making upgrades easier for users of your library.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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