Importing markdown files

Question

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
Link: http://cnn.com/article-post-references
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 += "\nLink: #{post.link}" if post.link.present?
  file_text += "\nTags: #{post.tag_list}" if post.tag_list.present?
  file_text += "\nImg: #{post.header_img}" if post.header_img.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 == 'link'
        post.link = value
      elsif key == 'img'
        post.header_img = value
      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
    posts_added = Post.pluck(:file_name) - SyncRecord.last.posts_present
    if posts_added.present?
      posts_added.each do |added_post_filename|

        post = Post.find_by_file_name(added_post_filename)
        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.string   "link"
  t.string   "header_img"
  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

Answer 1

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
  HEADER_ATTRS = {
    "Title" => :title,
    "Date"  => :publish_time,
    "Link"  => :link,
    "Tags"  => :tags,
    "Img"   => :header_img
  }.freeze

  BODY_ATTR = :body

  # Dump the post to its file
  def write_to_file
    File.open(file_name, 'w+') do |file|
      HEADER_ATTRS.each do |name, method|
        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
      # read the rest
      attributes[BODY_ATTR] = io.read.strip
    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.