2
\$\begingroup\$

I have to develop a class that creates new files. If a file with the given name already exists, it should be renamed by addition of the timestamp to the file name. The file name should be human-readable, so I can use neither UUID nor createTempFile. The code works, but looks too bulky.

public class FileBackup {

    /**
     * Creates new file. If file with given name exists, then backups
     * it by renaming and adding time stamp to file name;
     *  
     * @param filePath the fully qualified name of file

     * @return the file on success; otherwise returns null
     * 
     * @throws IOException 
     */
    public static File backupAndCreateFile(String filePath) throws IOException  {

        File fileToCreate = new File(filePath);

        boolean backupRequired =  fileToCreate.isFile();

        if (backupRequired && !backupFile(fileToCreate)) {
            return null;
        }

        File file = new File(filePath);
        return file.createNewFile() ? file : null;
    }

    private static final String pattern = "-dd_M_yyyy-hh_mm_ss";
    private static final DateFormat dateFormat = new SimpleDateFormat(pattern);     
    private static String generateBackupName (String filePath) {        
        String name = getFileName (filePath);
        String ext = getFileExtention (filePath);
        return name + dateFormat.format(new Date()) + ext;
    }

    private static String getFileName (String filePath) {   
        int i = filePath.lastIndexOf('.');
        return i == -1 ? filePath : filePath.substring(0, i);
    }

    private static String getFileExtention (String filePath) {  
        int i = filePath.lastIndexOf('.');
        return i == -1 ? "" : filePath.substring(i);
    }

    private static boolean backupFile (File file) {

        String filePath = file.getAbsolutePath();

        while (true) {

            String backupName = generateBackupName (filePath);
            File backup = new File(backupName); 

            if (!backup.isFile()) return file.renameTo(backup);

            // File with generated name exists, so 
            // sleep for 0.5 second and try another name
            try {
                Thread.sleep(500);
            } catch (InterruptedException e) {return false;}; // never should be here       
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ It doesn't really fit as an answer because concurrency isn't really a goal here, but do note that SimpleDateFormat as a static final field, is a bad idea: since that class isn't thread-safe, it's bound to cause failures when used concurrently by multiple threads. Prefer to store them as local variables (and if the cost of creation is too much, as ThreadLocal<SimpleDateFormat>. \$\endgroup\$ – Tunaki Oct 25 '16 at 19:52
4
\$\begingroup\$

In my opinion your code it's not so bulky but I would do something like below with few changes. I have not tested it too much but it should works.

I will use FilenameUtils for easy manipulation with files like getting basename or extension of file - it will save you some of the lines and also it is one of the most used library for file manipulation.

I would remove String getFileName and String getFileExtention and create self-described function and delete in code comments. I also removed return false for InterruptedException - I would like to get another draft of the name in the next iteration.

I dont like spaggetti code when some string is creating like (with +):

return name + dateFormat.format(new Date()) + ext;

so here I would use StringBuilder or String.format or something like that.

Below is what I have done in few minutes.

import org.apache.commons.io.FilenameUtils;

import java.io.File;
import java.io.IOException;
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.Date;

public class FileBackup {

    private static final String pattern = "-dd_M_yyyy-hh_mm_ss.";
    private static final DateFormat dateFormat = new SimpleDateFormat(pattern);

    /**
     * Creates new file. If file with given name exists, then backups
     * it by renaming and adding time stamp to file name;
     *
     * @param filePath the fully qualified name of file

     * @return the file on success; otherwise returns null
     *
     * @throws IOException
     */
    public static File backupAndCreateFile(String filePath) throws IOException {
        return failedBackuOfExistingFile(filePath) ? null : new File(filePath);
    }

    private static boolean failedBackuOfExistingFile(String filePath) {
        File fileOnPath = new File(filePath);
        return  fileOnPath.exists() && !successBackupFile(fileOnPath);
    }

    private static boolean successBackupFile(File fileToBackup) {

        while (true) {

            File backupDraft = new File(generateBackupName(fileToBackup.getAbsolutePath()));
            if (!backupDraft.exists()) {
                return fileToBackup.renameTo(backupDraft);
            }
            fileWithGeneratedNameExistsNowSleepForSomeTime();
        }
    }

    private static void fileWithGeneratedNameExistsNowSleepForSomeTime(){
        try {
            Thread.sleep(500);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }

    private static String generateBackupName (String filePath) {
        String fileName = FilenameUtils.getBaseName(filePath);
        String fileExtension = FilenameUtils.getExtension(filePath);
        return new StringBuilder().append(fileName)
                .append(dateFormat.format(new Date()))
                .append(fileExtension)
                .toString();
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! Good job on your first post; we look forward to seeing more in the future! \$\endgroup\$ – TheCoffeeCup Oct 25 '16 at 19:19
2
\$\begingroup\$
public static File backupAndCreateFile(String filePath) throws IOException  {
  File fileToCreate = new File(filePath);
  boolean backupRequired =  fileToCreate.isFile();
  if (backupRequired && !backupFile(fileToCreate)) {
    return null;
  }
  File file = new File(filePath);
  return file.createNewFile() ? file : null;
}
  • never return NULL unless it is a valid value of the result set. In this case I'd argue. You should throw an (Runtime-)Exception where you return the NULLs.

  • There is no need to create a second File object, just return the first one.

  • do not rely on side efects in if conditions. Make it explicit, that the backup method will be called only if needed.

  • isFile() seems to me a bit suspicious, what if the given name points to a directory?

    public static File backupAndCreateFile(String filePath) throws IOException  {
        File fileToCreate = new File(filePath);
        boolean backupRequired =  fileToCreate.isFile();
        if (fileToCreate.exists())
            if(fileToCreate.isFile()) {
                if(!backupFile(fileToCreate)) 
                    throw new IoException("could not create new backup for file "+fileToCreate);
            } else {
                throw new IoException( fileToCreate+" is not a regular file and cannot be backuped");
            }
        if(  fileToCreate.createNewFile())
            return  fileToCreate;
        throw new IoException("could not create new file "+fileToCreate);
    }
    
\$\endgroup\$
0
\$\begingroup\$

Directories

File's can also be directories. You probably want to check this.

Thread.sleep()

Why don't use Random to do this faster?

important-25_10_2016-21_03_55-1526373.txt

isn't less readable than

important-25_10_2016-21_03_55.txt

.

getFileName and getFileExtension

They both use lastIndexOf and so pretty much the same. So why don't bring those two little methods together with theor only use.

Random random = new Random();
private static String generateBackupName(String fileName){
  int period = fileName.lastIndexOf(".");
  if(period==-1){
    return fileName+dateFomat.format(new Date())+"-"+random.nextInt();
  }
  String basename = fileName.substring(0,period);
  String extension = filename.substring(period);
  return basename+dateFomat.format(new Date())+"-"+random.nextInt()+extension;
}

I often need functionallity like this and always did it worse than yours.

\$\endgroup\$

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.