Ruby Repository Pattern

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.