# Importing markdown files

I'm building a rails app that, among other things, imports text markdown files as blog posts. The idea is that the truth is in the markdown files, so any time those are edited, created, or deleted, the blog should reflect this. Of course, I had to make this more complicated and insist that I should be able to edit, add, or delete a post online as well. So I had to build out a sync mechanism between the local markdown files and the remote database.

In seemed to me that the best way to do this is to just create a few methods that handle file import, file export, and one that syncs the local files with the remote db. The markdown files have metadata that's encoded very simply.

Example:

Title: My Post
Date: 2012-03-20 9:10
Tags: politics, tech, etc
Draft: true

My post just starts here; as you'll note, there's no indicator for the body. It just proceeds in markdown.

New paragraphs are set off according to markdown, etc.


I've built the helper and the code is below. I'm not very seasoned, and it seems that some of this is a bit redundant (especially how I iterate over the meta-data) and was wondering if anyone has any suggestions for improvements.

def self.update_or_create_file(post)
file_text = "Title: #{post.title}"
file_text += "\nDate: #{post.publish_time}"
file_text += "\nTags: #{post.tag_list}" if post.tag_list.present?
file_text += "\n \n#{post.body}" if post.body.present?

File.open(post.file_name, 'w+') do |file|
file.write(file_text)
end
end

def self.update_or_create_post(file)

body = ''

# Iterate over each file
File.open(file, 'r').each_line do |line|

# If there's already a post, use that; if not, create one
post = Post.find_by_file_name(file) || Post.create(file_name:file)

# If the line has a ':', check for meta-data
if line.match(/.*:.*/)

key, value = line.split(":", 2)
key = key.strip.downcase
value = value.to_s.strip

if key == 'title'
post.title = value
elsif key == 'date'
post.publish_time = value
elsif key == 'img'
elsif key == 'published'
if value.strip.downcase == 'false'
post.published = false
else
post.published = true
end
elsif key == 'tags'
post.tag_list = value
else
body += line
end

elsif line.match(/^[-][-]/)
# I don't want these lines, so do nothing
else
# If the line has gotten this far, add it to the body
body += line
end

post.body = body.strip
post.save
end
end

def self.sync_posts(path)

Dir.chdir(path)

# Use Sync Records to check deletion of posts on server or deletion of post files
# Note that the order these are checked in is important
if SyncRecord.last.present? # skip if this is the first sync

# Check for deleted posts; if so, then delete file
posts_deleted = SyncRecord.last.posts_present - Post.pluck(:file_name)
if posts_deleted.present?
posts_deleted.each do |deleted_post_filename|
File.delete(deleted_post_filename) if File.exists?(deleted_post_filename)
end
end

# Check for created posts; if so, then create the file

update_or_create_file(post)

end
end

# Check for deleted files; if so, then delete posts
# Note here that I'm checking local vs. server and resolving in favor of local
# this is in case there are any odd states in the sync records
files_deleted = Post.pluck(:file_name) - Dir.glob('*.{markdown,md}')
if files_deleted.present?
files_deleted.each do |deleted_filename|
Post.find_by_file_name(deleted_filename).delete
end
end

end

# Now let's sync the content of the local files
# Grab all markdown files
Dir.glob('*.{markdown,md}').each do |file|

if SyncRecord.last.present? && Post.find_by_file_name(file).present?

post = Post.find_by_file_name(file)

# check if the file or post was updated since the last sync
if File.mtime(file) > SyncRecord.last.updated_at || post.updated_at > SyncRecord.last.updated_at

# if the file was updated more recently, overwrite the post
if File.mtime(file) > post.updated_at
update_or_create_post(file)

# if the post was updated more recently, overwrite the file
elsif post.updated_at > File.mtime(file)
update_or_create_file(post)
end

end
else
# If this is the first sync or if there's a new file, just create it
update_or_create_post(file)
end
end

# Save the current state as a new Sync Record
record = SyncRecord.new
record.files_present = Dir.glob('*.{markdown,md}')
record.posts_present = Post.pluck(:file_name)
record.save

end


It may also be helpful to see my schema.rb; the tags/taggings I cheated on by just using the acts_as_taggable_on gem.

create_table "posts", force: true do |t|
t.string   "title"
t.text     "body"
t.datetime "publish_time"
t.datetime "created_at"
t.datetime "updated_at"
t.string   "file_name"
t.boolean  "published"
end

create_table "sync_records", force: true do |t|
t.string   "files_present", default: [], array: true
t.string   "posts_present", default: [], array: true
t.datetime "created_at"
t.datetime "updated_at"
end

create_table "taggings", force: true do |t|
t.integer  "tag_id"
t.integer  "taggable_id"
t.string   "taggable_type"
t.integer  "tagger_id"
t.string   "tagger_type"
t.string   "context",       limit: 128
t.datetime "created_at"
end

add_index "taggings", ["tag_id"], name: "index_taggings_on_tag_id", using: :btree
add_index "taggings", ["taggable_id", "taggable_type", "context"], name: "index_taggings_on_taggable_id_and_taggable_type_and_context", using: :btree

create_table "tags", force: true do |t|
t.string "name"
end


There are actually 2 kinda distinct pieces of code to review here: The file I/O, and the syncing.

For now, this'll be a partial review, looking only at the file I/O, as I haven't had time to look at the sync code. Update: Added some thoughts on the syncing at the end of this answer. These first general notes should be of use in either case.

First off, as a rule of thumb, in Ruby your methods should be short. Like, really short. There are of course different schools of thought on what the limit is, but a maximum of 10 lines is a common one. Some say 5 lines maximum.

Again, this is just a general rule/something to aim for, so you're free to do disregard it, but it forces you to justify (and thus rethink) your code if it's longer. And in your case, you've got methods that are very long, so, yeah, that might need some looking into. The benefit of keeping things short is that you end up with code that's easy to read, maintain, and extend.

Second, more specifically, I see a lot of repetition. For instance, the metadata names are repeated (with different casing even) in both the file writer and the file reader methods. So maintenance will be difficult and extending it requires a lot of extra code.

As for your file format, it's actually not that different from plain HTTP: Header and Body separated by a blank line, with headers declared using colons. Easy to parse. The trick, though, is to stick to that format. If you've already got a bunch of files, I'd suggest you go through them now (before you have more), and make sure they're consistent. The less flexibility the parser has to deal with, the better (this includes deciding on a file extension; yes, both ".md" and ".markdown" are valid, but pick one and stick to it. Your example also has a "Draft" header that's not handled anywhere)

Now, practically, I'd consider moving the file I/O code to the Post model (if it isn't there already), since that's what needs the I/O, and separating your syncing code.

Also: Tests. Learn to love 'em, if you haven't.

Here's how I might write the Post methods. Note that they all directly or indirectly call file methods that may raise exceptions.

class Post < ActiveRecord::Base
"Title" => :title,
"Date"  => :publish_time,
"Tags"  => :tags,
}.freeze

BODY_ATTR = :body

# Dump the post to its file
def write_to_file
File.open(file_name, 'w+') do |file|
io.puts "#{name}: #{send(method)}" if send(method).present?
end
file.puts # separate headers and body with a blank line
io.puts send(BODY_ATTR) if send(BODY_ATTR).present?
end
end

# Initialize/find a post by and with a file
# Note that Post record is not persisted by this method;
# that's left to the caller
def self.find_or_initialize_from_file(path)
attributes = self.parse_file(path)
post = Post.find_or_initialize_by_file_name(file_name)
post.assign_attributes attributes
post
end

# Parse the given file, returning an attribute hash
# (this still looks a tad messy to me)
def self.parse_file(path)
attributes = {}
File.open(path, 'r') do |io|
# read lines until we hit something that doesn't look like a header
# (i.e. the blank line in a properly formatted file)
while io.gets =~ /\A([^:]+): (.+)\Z/
attribute = HEADER_ATTRS[$1] attributes[attribute] =$2.strip if attribute
end
end
attributes
end
end


The weakness lies in backwards/forwards compatibility. If you add an attribute to the HEADER_ATTRS hash, older files won't get read with a default value. Similarly, if you remove an attribute, it'll be ignored in existing files. This can all be fixed, but write some tests for this basic behavior first.

Update: Syncing

Again, I'd roll all this into a separate class. That would allow you to refactor the logic into separate methods. Start with the repetitive bits, like globbing the markdown files or getting the latest SyncRecord, and move on to the distinct "steps" in the process.
Favor methods that just return stuff for the most part, but do not actually modify files or database - keep those potentially destructive operations separate (similar to the code above, where Post.find_or_initialize_from_file returns a new instance but doesn't assume it should be persisted).

Various notes:

• Tests. Aim for a high tests-to-code ratio, as bugs here could be super-destructive. E.g. a bug causes all files to be deleted, next sync faithfully deletes all post records. Oops.
• Your code will, as far as I can tell, always update either a file or a post record, because you don't sync the file's mtime and the record's timestamp. So one or the other will always be ahead. Obviously, this very inefficient.
• Dir.chdir(), when used without a block, changes the working director for your entire app. So avoid that(!) in favor of absolute paths.
• Favor any? over present? when checking arrays. Functionally it's the same, but any? makes it obvious what we're dealing with.
• Use post.destroy instead of post.delete. Again, no real difference, but destroy is the Rails convention.
• Speaking of, I might prefer to set a deleted flag on the Post model and delete the file, but otherwise keep the data in the database to avoid accidental data loss. Also makes it simple to find out which post records have been deleted (by the way: Tests!)
• You could consider collecting the tainted file names, and do the updating/deleting in a single (or at least centralized) operation, rather than individual ones. This will be easier if you give yourself methods like deleted_posts or updated_files. It'll also limit (but not eliminate) your exposure to race conditions where things change while syncing is in progress (which is something you should be wary of. Also, tests)
• Beware of exceptions, especially with all those file operations
• Tests? Tests.