Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor to use memoizable #24

Merged
merged 7 commits into from Dec 14, 2013
Merged

Refactor to use memoizable #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
81ed2fd
Refactor memoization to use memoizable
dkubb Nov 21, 2013
f21ffd9
Remove completed task from TODO
dkubb Nov 21, 2013
212c653
Remove return value from Adamantium.included
dkubb Nov 22, 2013
101e358
Change Adamantium.included to call super
dkubb Nov 22, 2013
6744cab
Change case statement to be a Hash lookup
dkubb Dec 1, 2013
7e2edfc
Move method builder logic to memoizable
dkubb Dec 1, 2013
878b3e8
Change flay threshold
dkubb Dec 1, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 1 addition & 4 deletions Gemfile
View file
Open in desktop
Expand Up @@ -4,10 +4,7 @@ source 'https://rubygems.org'

gemspec

platforms :rbx do
gem 'rubysl-prettyprint', '~> 2.0.2'
gem 'rubysl-singleton', '~> 2.0.0'
end
gem 'memoizable', git: 'https://github.com/dkubb/memoizable.git'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? If so, should we push another gem release? It appears that latest memoizable gem (v0.2.0) has diverged from master by a few commits.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's necessary because of a few changes I made in memoizable; nothing that breaks the interface though.

I'll probably release memoizable 0.3.0 once I test all this properly with some other dependent gems.


group :development, :test do
gem 'devtools', git: 'https://github.com/rom-rb/devtools.git'
Expand Down
1 change: 0 additions & 1 deletion TODO
View file
Open in desktop
@@ -1,2 +1 @@
* Update #hash to be memoized automatically
* Refactor memoization to use memoizable
4 changes: 2 additions & 2 deletions adamantium.gemspec
View file
Open in desktop
Expand Up @@ -17,8 +17,8 @@ Gem::Specification.new do |gem|
gem.test_files = `git ls-files -- spec/{unit,integration}`.split("\n")
gem.extra_rdoc_files = %w[LICENSE README.md CONTRIBUTING.md TODO]

gem.add_runtime_dependency('ice_nine', '~> 0.10.0')
gem.add_runtime_dependency('thread_safe', '~> 0.1.3')
gem.add_runtime_dependency('ice_nine', '~> 0.10.0')
gem.add_runtime_dependency('memoizable', '~> 0.2.0')

gem.add_development_dependency('bundler', '~> 1.3', '>= 1.3.5')
end
4 changes: 2 additions & 2 deletions config/flay.yml
View file
Open in desktop
@@ -1,3 +1,3 @@
---
threshold: 7
total_score: 53
threshold: 4
total_score: 24
2 changes: 1 addition & 1 deletion config/flog.yml
View file
Open in desktop
@@ -1,2 +1,2 @@
---
threshold: 17.5
threshold: 15.0
104 changes: 3 additions & 101 deletions lib/adamantium.rb
View file
Open in desktop
@@ -1,14 +1,11 @@
# encoding: utf-8

require 'ice_nine'
require 'thread_safe'
require 'memoizable'

# Allows objects to be made immutable
module Adamantium

# Storage for memoized methods
Memory = Class.new(ThreadSafe::Hash)

# Defaults to less strict defaults
module Flat

Expand Down Expand Up @@ -48,62 +45,15 @@ def self.included(descendant)
#
# @api private
def self.included(descendant)
super
descendant.class_eval do
include Memoizable
extend ModuleMethods
extend ClassMethods if kind_of?(Class)
end
self
end
private_class_method :included

# Freeze the object
#
# @example
# object.freeze # object is now frozen
#
# @return [Object]
#
# @api public
def freeze
memory # initialize memory
super
end

# Get the memoized value for a method
#
# @example
# hash = object.memoized(:hash)
#
# @param [Symbol] name
# the method name
#
# @return [Object]
#
# @api public
def memoized(name)
memory[name]
end

# Sets a memoized value for a method
#
# @example
# object.memoize(:hash, 12345)
#
# @param [Symbol] name
# the method name
# @param [Object] value
# the value to memoize
#
# @return [self]
#
# @api public
def memoize(name, value)
unless memory.key?(name)
store_memory(name, freeze_object(value))
end
self
end

# A noop #dup for immutable objects
#
# @example
Expand All @@ -116,54 +66,6 @@ def dup
self
end

private

# The memoized method results
#
# @return [Hash]
#
# @api private
def memory
@__memory ||= Memory.new
end

# Freeze object
#
# @param [Object] object
# an object to be frozen
#
# @return [Object]
#
# @api private
def freeze_object(object)
freezer.call(object)
end

# Return class level freezer
#
# @return [#call]
#
# @api private
def freezer
self.class.freezer
end

# Store the value in memory
#
# @param [Symbol] name
# the method name
# @param [Object] value
# the value to memoize
#
# @return [self]
#
# @return [value]
#
# @api private
def store_memory(name, value)
memory[name] = value
end

end # Adamantium

require 'adamantium/module_methods'
Expand Down
12 changes: 7 additions & 5 deletions lib/adamantium/freezer.rb
View file
Open in desktop
Expand Up @@ -94,6 +94,12 @@ class UnknownFreezerError < RuntimeError; end
# Error raised when memoizer options contain unknown keys
class OptionError < RuntimeError; end

@freezers = {
noop: Noop,
deep: Deep,
flat: Flat,
}.freeze

# Return freezer for name
#
# @param [Symbol] name
Expand All @@ -103,11 +109,7 @@ class OptionError < RuntimeError; end
#
# @api private
def self.get(name)
case name
when :noop then Noop
when :deep then Deep
when :flat then Flat
else
@freezers.fetch(name) do
fail UnknownFreezerError, "Freezer with name #{name.inspect} is unknown"
end
end
Expand Down
114 changes: 2 additions & 112 deletions lib/adamantium/module_methods.rb
View file
Open in desktop
Expand Up @@ -32,62 +32,6 @@ def memoize(*methods)
self
end

# Test if an instance method is memoized
#
# @example
# class Foo
# include Adamantium
#
# def bar
# end
# memoize :bar
#
# end
#
# Foo.memoized?(:bar) # true
# Foo.memoized?(:baz) # false, does not care if method acutally exists
#
# @param [Symbol] name
#
# @return [true]
# if method is memoized
#
# @return [false]
# otherwise
#
# @api private
def memoized?(name)
memoized_methods.key?(name)
end

# Return original instance method
#
# @example
#
# class Foo
# include Adamantium
#
# def bar
# end
# memoize :bar
#
# end
#
# Foo.original_instance_method(:bar) #=> UnboundMethod, where source_location still points to original!
#
# @param [Symbol] name
#
# @return [UnboundMethod]
# if method was memoized before
#
# @raise [ArgumentError]
# otherwise
#
# @api public
def original_instance_method(name)
memoized_methods[name]
end

private

# Hook called when module is included
Expand All @@ -114,62 +58,8 @@ def included(descendant)
#
# @api private
def memoize_method(method_name, freezer)
method = instance_method(method_name)
if method.arity.nonzero?
fail ArgumentError, 'Cannot memoize method with nonzero arity'
end
memoized_methods[method_name] = method
visibility = method_visibility(method_name)
define_memoize_method(method, freezer)
send(visibility, method_name)
end

# Return original method registry
#
# @return [Hash<Symbol, UnboundMethod>]
#
# @api private
def memoized_methods
@memoized_methods ||= ThreadSafe::Hash.new do |_memoized_methods, name|
fail ArgumentError, "No method #{name.inspect} was memoized"
end
end

# Define a memoized method that delegates to the original method
#
# @param [UnboundMethod] method
# the method to memoize
# @param [#call] freezer
# a freezer for memoized values
#
# @return [undefined]
#
# @api private
def define_memoize_method(method, freezer)
method_name = method.name.to_sym
undef_method(method_name)
define_method(method_name) do ||
memory.fetch(method_name) do
value = method.bind(self).call
frozen = freezer.call(value)
store_memory(method_name, frozen)
end
end
end

# Return the method visibility of a method
#
# @param [String, Symbol] method
# the name of the method
#
# @return [Symbol]
#
# @api private
def method_visibility(method)
if private_method_defined?(method) then :private
elsif protected_method_defined?(method) then :protected
else :public
end
memoized_methods[method_name] = Memoizable::MethodBuilder
.new(self, method_name, freezer).call
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkubb I already was thinking hard about creating a MethodObject mixin that would support transforming:

MethodBuilder.new(self, method_name, freezer).call into MethodBuilder.call(self, method_name_freezer)

Creating an instance and delegating to #call:

def self.call(*arguments)
  new(*arguments).call
end

Would DRY up a lot of places in my code.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbj do you find yourself doing this kind of thing in your gems that dynamically add methods? I really like this approach and will probably even look at doing it in equalizer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbj @dkubb i do that kind of thing very often! i haven't yet thought about extracting something generic from it, but if a nice api crops up, i already know that i'd use it in a ton of places, public and private code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usage of such a method_object gem would be as simple as:

class MethodBuilder
  include MethodObject, Concord.new(:object, :method_name, :freezer)

  def call
     calculate_something_based_on_instance_state
  end
end

MethodBuilder.call(self, method_name, freezer)

end

end # ModuleMethods
Expand Down
15 changes: 0 additions & 15 deletions spec/unit/adamantium/freeze_spec.rb
View file
Open in desktop
Expand Up @@ -22,12 +22,6 @@
.from(false)
.to(true)
end

it 'sets a memoization instance variable' do
expect(object).to_not be_instance_variable_defined(:@__memory)
subject
expect(object.instance_variable_get(:@__memory)).to be_instance_of(Adamantium::Memory)
end
end

context 'with a frozen object' do
Expand All @@ -38,14 +32,5 @@
it 'does not change the frozen state of the object' do
expect { subject }.to_not change(object, :frozen?)
end

it 'does not change the memoization instance variable' do
expect { subject }.to_not change { object.instance_variable_get(:@__memory) }
end

it 'does not set an instance variable for memoization' do
expect(object.instance_variable_get(:@__memory)).to be_instance_of(Adamantium::Memory)
subject
end
end
end