# Ruby Repository Pattern

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

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_accessor :email
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

end

end

def email=(new_mail)
@email = new_mail
end

def email
@email
end

end

end
end
end


repository.rb

module Repository
class Member
end

def save(member)
end

def get(id)
end

def delete(member)
end
end
end


test.rb

require './lib/entity'
require './lib/repository'

member = Entity::Member.new
member.email = 'ahmet@kapikiran.com'
repository.save(member)

member = repository.get(5)

repository.delete(member)

• Welcome to Code Review. Adding some specific questions would more then likely increase the chance of getting the reviews you're looking for. – Kid Diamond Sep 22 '14 at 20:01
• 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. – Flambino Sep 22 '14 at 20:30
• @Flambino thank u. I edited the question. – mollyjane Sep 22 '14 at 22:28

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

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

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 ('foo@example.com', '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.