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.