Ruby Repository Pattern

Question

I am trying to teach myself about repository pattern in ruby. Could you please review my code.

adapter.rb

module Adapter
 class Main
 def prebuild(obj)
 self.set_table_name = obj.table_name.to_s
 variables = obj.instance_variables
 @values = []
 @keys = []
 variables.reject! { |c| c.to_s == '@table_name' }.map do |key, value|
 @keys.push(key.to_s.gsub('@', ''))
 @values.push(obj.send(@keys.last))
 end
 end
 def get_table_name
 @table_name
 end
 def get_keys
 @keys
 end
 def get_values
 @values.map { |value| "'#{value}'" }
 end
 def set_table_name=(table_name)
 @table_name = table_name
 end
 end
 class Sqlite < Main
 require 'sqlite3'
 def initialize
 @db = SQLite3::Database.new 'file.db'
 end
 def persist(obj)
 prebuild(obj)
 sql_string = "INSERT INTO #{get_table_name} (#{get_keys.join(', ')}) VALUES (#{get_values.join(', ')})"
 @db.execute sql_string
 obj.id = @db.execute "SELECT last_insert_rowid()"
 obj
 end
 def retrieve(entity, id)
 self.set_table_name = entity.table_name
 rows = @db.execute "SELECT * FROM #{get_table_name} WHERE id=#{id}"
 fields = get_fields
 new_entity = entity.new
 for i in (0..fields.size-1)
 new_entity.send("#{fields[i]}=", rows[0][i])
 end
 new_entity
 end
 def get_fields
 rows = @db.execute "PRAGMA TABLE_INFO (#{get_table_name});"
 rows.map { |name| name[1] }
 end
 def delete(obj)
 prebuild(obj)
 @db.execute "DELETE FROM #{get_table_name} WHERE id=#{obj.id}"
 end
 end
end

entity.rb

module Entity
 class Member
 attr_reader :table_name
 attr_accessor :username
 attr_accessor :email
 attr_accessor :password
 class << self
 def table_name
 @table_name ||= 'members'
 end
 end
 def table_name
 @table_name ||= 'members'
 end
 def id=(new_id)
 @id = new_id
 end
 def id
 @id
 end
 def username=(new_username)
 @username = new_username
 end
 def username
 @username
 end
 def email=(new_mail)
 @email = new_mail
 end
 def email
 @email
 end
 def password=(new_password)
 @password = new_password
 end
 def password
 @password
 end
 end
end

repository.rb

module Repository
 class Member
 def initialize(adapter)
 @adapter = adapter.new
 end
 def save(member)
 @adapter.persist(member)
 end
 def get(id)
 @adapter.retrieve(Entity::Member, id)
 end
 def delete(member)
 @adapter.delete(member)
 end
 end
end

test.rb

require './lib/entity'
require './lib/adapter'
require './lib/repository'
member = Entity::Member.new
member.username = 'ahmet'
member.email = '[email protected]'
member.password = '12345'
repository = Repository::Member.new(Adapter::Sqlite)
repository.save(member)
member = repository.get(5)
repository.delete(member)

• Kid Diamond: Welcome to Code Review. Adding some specific questions would more then likely increase the chance of getting the reviews you're looking for.
• Flambino: The code seems incomplete and, well, kinda broken. Incomplete isn't necessarily a problem, but broken code would technically make it off-topic. For instance, the SQL query generated in Sqlite#persist uses get_keys for both column names and column values. So it'll never actually store any of your data. I'd suggest you spend a little more time making sure your code actually does what you want, and then post it again so we can review it properly.

Answer 1

Starting with the end: test.rb barely tests anything. Yes, it'll exercise your code, and make it crash if there's a mistake, but it doesn't actually verify that your code does what it's supposed to do.

So, I'd highly suggest you write proper unit test using one of the many test libraries out there (Ruby's own Test::Unit, minitest, rspec, etc...)

You seem to be confused about accessor methods. The purpose of attr_accessor is to have Ruby automatically generate the methods to get and set an instance variable. In your Member class, you're using attr_accessor, but you're also (redundantly) providing your own getters and setters. Since they literally just get and set, they're exactly the same as what attr_accessor has already made for you.

So Member can be reduce to this:

module Entity
 class Member
 attr_accessor :id, :username, :email, :password
 class << self
 def table_name
 @table_name ||= 'members'
 end
 end
 def table_name
 @table_name ||= 'members'
 end
 end
end

(Of course, id shouldn't be easily settable, but I can't cover everything in this review)

The whole table_name construction is odd too. If anything, this would make more sense:

module Entity
 class Member
 attr_accessor :id, :username, :email, :password
 def self.table_name
 'members'
 end
 end
end

Since table_name should probably be constant, just return a string. And don't duplicate the method for both class and instance. If anyone needs the table name from an instance, they can do some_member_object.class.table_name.

Your Main class also has some funny ideas about data access. Here, you're not using attr_reader :keys and attr_accessor :table_name although you might as well. Then you can skip the methods. The other reason to do this is that naming methods get_* or set_* is not very Ruby-like.

get_values (which should just be named values), is also problematic for a few reasons:

• Main is supposed to be pretty abstract. Why does it assume that values should be returned as quoted strings? What is the derived class isn't meant for SQLite or any kind of SQL? Perhaps you just want the raw values?

• Yes, in fact you do want the raw values! Poor string-quoting is the oldest attack vector in the book. For instance, what if, say, the password value is a string like this: "sup'); DROP TABLE members;"?

With your code, you'll end up running the following the current query when trying to save:

INSERT INTO members (email, username, password)
VALUES ('[email protected]', 'foo', 'sup'); DROP TABLE members; ');

A basic SQL injection attack. As you can see from the syntax highlighting, the SQL suddenly contains TWO commands: INSERT and DROP TABLE. You insert a row... and it happens to delete your entire table. Oops.

Long story short: Use proper parameter substitution (which the sqlite3 gem has built in):

db.execute("INSERT INTO members (email, username, password) VALUES (?, ?, ?)", [email, username, password])

That let's sqlite take care of doing proper string quoting/escaping and all that. It's safe.

And you can replace the get_values method with attr_reader :values.

Of course, the only reason attr_readers make sense is that you have the prebuild method. Which doesn't really make much sense. If must be called before anything else works, so if anything it should be a constructor.

But really, it's simpler to just drop Main entirely, and instead add a superclass for Member:

class Record
 def to_hash
 instance_variables.each_with_object({}) do |hash, name|
 key = name.to_s.sub('@', '') }
 value = send(key)
 hash[key] = value;
 end
 end
end
class Member < Record
 ...
end

And then the sqlite adapter becomes:

require 'sqlite3' # require statement should be at the top of the file
class Sqlite
 def initialize
 @db = SQLite3::Database.new 'file.db' # the filename should really be an argument
 end
 # formerly named "persist", but "persist" can mean either insert *or* update
 def insert(record)
 hash = record.to_hash.delete("id")
 markers = ["?"] * hash.keys.count
 sql = "INSERT INTO #{record.class.table_name} (#{hash.keys.join(',')} VALUES (#{markers.join(','))"
 @db.execute(sql, hash.values);
 record.id = @db.execute("SELECT last_insert_rowid()")
 record
 end
 # presumably you'll only want 1 record returned when searching by id
 def retrieve(klass, id)
 columns = columns_in_table(klass.table_name)
 rows = @db.execute("SELECT * FROM #{klass.table_name} WHERE id=? LIMIT 1", id)
 return nil if rows.empty? # nothing found
 instance = klass.new
 rows.first.zip(columns).each do |key, value|
 instance.send("#{key}=", value)
 end
 instance
 end
 def delete(record)
 @db.execute("DELETE FROM #{record.class.table_name} WHERE id=#{record.id}"
 end
 private
 def columns_in_table(table)
 @db.execute("PRAGMA TABLE_INFO (#{table})").map { |info| info[1] }
 end
end

Of course, there's still a long way from here to a well-known and battle-tested implementation like Rails' ActiveRecord. For instance, you currently have to duplicate the names of columns in your objects. If an object doesn't have the same instance variables as the table's columns - or if they're named differently, or of the wrong type... - things will break.

So, in all, a good exercise, but not something I'd rely on.