HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

Web Development DSL

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
dsldevelopmentweb

Problem

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 t

Solution

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.

Code Snippets

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

  # Something about Bar
  class Bar

# Something about baz
def baz
alias req request
alias_method :req :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

Context

StackExchange Code Review Q#149200, answer score: 4

Revisions (0)

No revisions yet.