patternrubyMinor
Web Development DSL
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):
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
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!"
endatd.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:
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:
I prefer this:
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:
use:
Use the Forwardable module
You did a good job using metaprogramming to define methods for you:
For simple forwarding methods like this, there's a better way. Ruby's
Fowardable
module
will do it:
Dead code
In this method:
The inner checks for
outer
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:
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:
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:
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.
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 bazI prefer this:
module Foo
# Something about Bar
class Bar
# Something about baz
def bazThis 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 requestuse:
alias_method :req :requestUse 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
endFor 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] => :requestDead 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
endThe inner checks for
route.output.is_a?(File) are redundant: Theouter
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)
endRoutingCode::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
endSemantic 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 bazmodule Foo
# Something about Bar
class Bar
# Something about baz
def bazalias req requestalias_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
endContext
StackExchange Code Review Q#149200, answer score: 4
Revisions (0)
No revisions yet.