5
\$\begingroup\$

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
\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Point taken with ownership of files. I think I will do general rewrite to turn the functionality of the class on its head. Is there a general strategy to ensure against collision between processes when manipulating files? This was exactly what I was trying to achieve with my code, by the way :) So I want to approach that feature again, but in a more proper way. \$\endgroup\$ – Reefersleep Sep 7 '14 at 18:25
  • \$\begingroup\$ Also, I had my doubts about the [filename]_old[number] strategy, rather than the simpler [filename][number]. I kind of wanted the "backup" filenames to give an impression of their purpose with the "_old" part. I think I've changed my mind now, and will go for [filename][number] in the next iteration. Also, whoops on the extension thing! Hadn't thought of that. Cheers for the "log.txt.3" tip, I think I'll see if that works out! \$\endgroup\$ – Reefersleep Sep 7 '14 at 18:30
  • \$\begingroup\$ I'd thought about making the class a module myself, and now I feel more certain that it's the right way to go. I have a Java background and feel a little uncertain about the general purpose of modules. Thank you! \$\endgroup\$ – Reefersleep Sep 7 '14 at 18:32
  • 1
    \$\begingroup\$ @Reefersleep Glad it helped! As for avoiding collisions in the file system, user50399's answer points out the flags you can set. An exclusive lock will make the file system complain if you can't get such a lock (or it'll complain to other processes if they try to access the file you've gotten a lock on) \$\endgroup\$ – Flambino Sep 7 '14 at 18:35
  • \$\begingroup\$ I'm against double negations myself because they confuse me, so the "until == false" bit is a brainfart. Calling "append_old_to" again and again might seem dumb from a resource perspective, but I'm generally trying to do things more functionally. Whenever you introduce a variable, you introduce a lifescope that you have to keep in your head when reading the code. If you can avoid this, you save your headspace for more important stuff. I'm inspired by the more functional Clojure, which I'm also learning, and I might be in the wrong when trying to write functional Ruby. \$\endgroup\$ – Reefersleep Sep 7 '14 at 18:40
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Cheers, I'll redesign the class as per your and @Flambino 's suggestions in order to avoid file access collisions :) \$\endgroup\$ – Reefersleep Sep 7 '14 at 22:02
0
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I think that those methods are intended to be atomic operations. \$\endgroup\$ – user50399 Aug 17 '14 at 1:16
  • \$\begingroup\$ Yes @user50399, I believe so, but that doesn't change my advice. \$\endgroup\$ – RubberDuck Aug 17 '14 at 1:46
  • \$\begingroup\$ Cheers for the advice, @RubberDuck. I get your point about "and", and I'm trying to follow it generally in my code from here on out :) I'm not sure I agree with the second point, though - the whole idea with CautiousWriter was to ensure that old files with the same name were not overwritten. I do see that this is not in congruence with the class name. Following the other advice further up, I'm going to redesign the class so that it leaves existing files be and instead finds an available [filename][number] name and writes the file to that. Do you think that would work better? \$\endgroup\$ – Reefersleep Sep 7 '14 at 22:11
  • \$\begingroup\$ A FileWriter in my mind doesn't fuss around with specific names when it renames. It simply renames it to whatever you pass to it. I still recommend a base class. It's been my experience with Ruby that very few classes should be more than a single screen or two. \$\endgroup\$ – RubberDuck Sep 7 '14 at 22:32
  • \$\begingroup\$ I think we're misunderstanding each other regarding levels of abstraction. Think of my class as the client code you're talking about. The lower level of abstraction that you think belongs to CautiousWriter already exists in File. CautiousWriter serves the purpose for me to simply aim a desired filename and content at, and ensure that a) no existing files are overwritten and b) my file is saved. I don't think that this is too complex for one class. I completely agree with you about class size, and my class takes up exactly one screen on my end :) \$\endgroup\$ – Reefersleep Sep 11 '14 at 22:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.