Cautious File Writer

Question

I'm trying to follow the path of Sandi Metz and others who state that aiming for small and simple is good for writing understandable code. I've written a method that takes care of writing content to a file without overwriting any existing file of the same name. I wrote it TDD style and, once the tests passed, I refactored the rather large methods into smaller methods, hoping that it would improve readability.

I hope you can tell me whether I succeeded, or how I could further improve on my code. I've considered adding modules (such as a "Appender" module) to my class in order to improve the code according to the single responsibility principle, but can't figure out whether it would actually clarify things more, or whether the added lines would make the intended clarification void.

class CautiousWriter
  def self.write(filename, content)
    if File.exists?(filename) 
      rename_existing_file_beginning_with(filename)
    end
    create_file(filename, content)
  end

  private

  def self.rename_existing_file_beginning_with(filename)
    if File.exists? append_old_to(filename)
      rename_with_old_and_number_appended(filename)
    else
      rename_with_old_appended(filename)
    end
  end

  def self.rename_with_old_and_number_appended(filename)
    old_and_num_appended = find_available_filename(filename)
    rename(filename, old_and_num_appended)
  end

  def self.find_available_filename(filename)
    number = 0
    until File.exists?(append_old_to(filename) + number.to_s) == false
      number += 1
    end
    append_old_to(filename) + number.to_s
  end

  def self.rename_with_old_appended(filename)
    rename(filename, append_old_to(filename))
  end

  def self.rename(filename, new_filename)
    File.rename(filename, new_filename)
  end

  def self.create_file(filename, content)
    File.open(filename, 'w') { |file| file.write(content) }
  end

  def self.append_old_to(filename)
    filename + '_old'
  end
end

Answer 1

Before looking at the code, I have a comment on the overall functionality of it. The idea is nice, but it commits a cardinal sin: It messes with files that do not belong to it.

This is A Very Bad Idea™.

Say you want to write the file "log.txt", but it already exists. Your code will rename the existing file, but that file may be in use. Some other process might expect it to be there, and fail when it gets renamed. Or your process will fail if the existing file is owned by another user or is locked. Or the other process will just start overwriting your file, since, hey, that's the one with the correct name. Or you get weird race conditions where you rename a file, and immediately after that, another process - finding the file gone - creates a new one, after which you overwrite the other process' new file; exactly what you're trying not to do. (see user50399's answer for more)

In other words, I wouldn't use this, regardless of the code itself.

But speaking of code, let's look at that.

You've certainly been diligent in dividing everything into methods - kudos. But, as ckuhn203 notes, a method with and in it indicates that the method is in fact doing several things.

I have to wonder why you're using a two-stage method of resolving file name conflicts: One is to append "_old", and the other is to also append a number. Just appending "_old" may not be enough, whereas numbers can simply increment forever (more or less). So you have one way of renaming that may or may not work, and one which will always work. So why use both? It just complicates the code.

Besides, maybe you don't want "_old" in the filename. Maybe you want "-backup" or just nothing. Right now the string is hard coded in a method body, so it's not exactly configurable.

I don't see any handling of file extensions, or anything to indicate that it's been considered. Though not required, it could be nice to rename "file.txt" to "file_old4.txt" instead of "file.txt_old4". Especially since you're using an underscore, meaning that the renamed file's extension is suddenly ".txt_old4", which isn't a real extension.
However, dealing with file extensions is a huge hassle, so if you look at something like the logrotate *nix utility, it just appends a dot and a number, instead of trying to parse file extensions, so you get files like "log.txt.3". Note that it's not using an underscore; it's simply adding an entirely new extension that's all-numeric. While not perfect, it's less likely to confuse anyone.

There's also the question of why this is a class. All its methods are class methods, and it has no state. Yet making it a class implies you can instantiate it - but that doesn't really do anything. You basically just get an instance of Object with no discernible functionality. I'd probably prefer to write it as a module instead (here are a number of ways to do that).

On a micro-level, I noticed this:

until File.exists?(append_old_to(filename) + number.to_s) == false

Saying "until something's not true" is the same as just saying "while something's true". It's cool that Ruby has the until keyword, but in this case you want a good old-fashioned while loop.
You could also avoid calling append_old_to again and again, since its return value isn't going to change. And personally, I'd use string interpolation here

number = 0
base_filename = append_old_to(filename)
while File.exists?("#{base_filename}#{number}")
  ...

Incidentally, this single method may be the most useful, if you "flip it" to produce a new filename you can use instead. I.e. don't try to rename existing files: Make sure that you have a non-conflicting filename before you write anything. Again, there's a risk of race conditions, if you get the file name before writing, but it's less risky than renaming existing files.

You might simply monkey patch the IO class with an open_uniq (or some such) method, which you pass the usual name, mode and block, but which - instead of returning the number of bytes written - returns whatever filename it actually ended up writing. So you give it "log.txt" and some data to write, and it might return "log.4.txt" because there happened to be a few of log*.txt files already.

Answer 2

This code suffers from Time-of-check vs. Time-of-use problems. Even if you check that the file you intend to create doesn't exist, some other process might create the file in the split-second between the check and the file operation. In some cases, these kinds of bugs might even be considered security vulnerabilities.

Instead of (or in addition to) checking beforehand, check the return values to see whether an operation succeeded. Set flags on File.open(), for example, to CREAT | EXCL to ensure that it will never open an existing file.

Answer 3

I think some of your methods may be trying to do a little too much. For example:

def self.rename_with_old_and_number_appended(filename)

And

def self.rename_with_old_appended(filename)

1. Seeing the word "and" in a method name is always a red flag.
2. Neither of these seem to be actually related to writing files.

I imagine that you take these actions often, so you (correctly) created methods for them. The problem is they don't really belong to a file writer and shouldn't be part of the API. They belong in the client code. When you go to reuse CautiousWriter in a different project, you'll find that you need different methods, perhaps with similar functionality. After several iterations of this, you'll end up with "one class to rule them all" and it will be an ummaintainable jungle of code.

Write a new class that inherits from CautiousWriter to implement these functions.