3
\$\begingroup\$

This is a class/method that manages files being written to and read from. I have a lot of code, and it grew as I tried to get it to work properly. Is there a better way to write this (i.e. trim the fat) or is it written properly?

import java.io.*;

public class DataBase {
public static String getExtension(File f) {
    String ext = null;
    String s = f.getName();
    int i = s.lastIndexOf('.');

    if (i > 0 && i < s.length() - 1) {
        ext = s.substring(i + 1).toLowerCase();
    }
    return ext;
}

public static String readFile(String file) {
    String data = "";
    if (!new java.io.File(file).exists()) {
        return data;
    }
    File f = new File(file);
    FileInputStream fStream = null;
    BufferedInputStream bStream = null;
    BufferedReader bReader = null;
    StringBuffer buff = new StringBuffer();

    try {
        fStream = new FileInputStream(f);
        bStream = new BufferedInputStream(fStream);
        bReader = new BufferedReader(new InputStreamReader(bStream));
        String line = null;

        while ((line = bReader.readLine())!=null) {
            //line = bReader.readLine();

            if (line.length() > 0) {
                if (line.contains("<br/>")) {
                    line = line.replaceAll("<br/>", " ");
                    String tempLine = "";
                    while ((tempLine.trim().length() < 1)
                            && bStream.available() != 0) {
                        tempLine = bReader.readLine();
                    }
                    line = line + tempLine;
                }
                buff.append(line += "\n");

            }
        }

        fStream.close();
        bStream.close();
        bReader.close();

    } catch (FileNotFoundException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }
    return buff.toString();

}

public static boolean writeToFile(String fileName, String data,
        boolean append) {
    boolean isWrite = false;
    int dirIndex = fileName.lastIndexOf(getPathSeparator());
    if (dirIndex != -1) {
        String dir = fileName.substring(0, dirIndex) + getPathSeparator();
        java.io.File fDir = new java.io.File(dir);
        if (!fDir.exists()) {
            if (!fDir.mkdirs()) {
                return false;
            }
        }
    }
    try {
        java.io.FileOutputStream fout = new java.io.FileOutputStream(
                fileName, append);
        java.nio.channels.FileChannel fChannelWriter = fout.getChannel();
        byte[] bytesToWrite = data.getBytes();
        java.nio.ByteBuffer bBuffW = java.nio.ByteBuffer.wrap(bytesToWrite);
        fChannelWriter.write(bBuffW);
        fChannelWriter.close();
        fout.close();
        isWrite = true;
    } catch (java.io.IOException ex) {
        System.out.println("Error Occured: Unable to write to file ("
                + fileName + ")");
        isWrite = false;
    }
    return isWrite;
}


public static String getPathSeparator() {
    return java.io.File.separator;
}
}
\$\endgroup\$
3
\$\begingroup\$

DataBase is a poor name for a collection of file I/O convenience routines. Especially one whose readFile() function swallows empty lines and strips <br/> tags as it reads. What exactly are you trying to accomplish with this class? I suspect that you have decomposed your problem poorly, and this class shouldn't exist.


The getExtension() function could be more succinct.

public static String getExtension(File f) {
    String s = f.getName();
    int i = s.lastIndexOf('.');

    return (i > 1 && i < s.length() - 1) ?
        s.substring(i + 1).toLowerCase() :
        null;
}

I've changed the condition from i > 0 to i > 1 since the names of Unix dotfiles (e.g. .profile) are not usually thought of as file extensions. That's a judgement call for you to make.


I think your readFile() should take a File rather than a String parameter. If you design your API so that you consistently use File to represent file paths and String to represent file content data, then it will be harder to accidentally call your function with swapped arguments (especially for writeToFile()). Also, if you call the other function writeToFile(), then you should rename readFile() to readFromFile() for consistency.

Swallowing exceptions is usually a bad idea, even if you print the stack trace. I would just let any IOExceptions propagate.

You're just using the default character encoding, so you don't need to bother with constructing InputStreams — just use Readers.

Testing for String.contains() before doing String.replaceAll() just repeats the work needlessly. String.replaceAll() will just return the original string anyway if no replacement needs to be performed.

I don't know what you are trying to accomplish with the tempLine stuff. Since you initialize tempLine = "", it will always be true that tempLine.trim().length() is 0, so all of that is dead code.

Assuming you don't need thread safety, just use StringBuilder instead of StringBuffer.

Doing a string concatenation in buff.append(line + "\n") defeats the efficiency gains of using a StringBuffer.

All together…

public static String readFromFile(File f) throws IOException {
    if (!f.exists()) {
        // I find this behavior surprising; I would have expected a
        // FileNotFoundException to be thrown instead.
        return "";
    }

    FileReader r = new FileReader(f);
    BufferedReader br = null;
    try {
        br = new BufferedReader(r);
        StringBuilder buff = new StringBuilder();

        String line;
        while (null != (line = br.readLine())) {
            if (line.length() > 0) {
                line = line.replaceAll("<br/>", " ");
                buff.append(line).append('\n');
            }
        }
        return buff.toString();
    } finally {
        if (br != null) br.close();
        r.close();
    }
}

The code could be a bit less clumsy if you use Java 7's try-with blocks.


writeToFile() can be simplified by a lot. For simplicity, I would just use a FileWriter instead of the java.nio API.

public static void writeToFile(File f, String data, boolean append) throws IOException {

    File parent = f.getParentFile();
    if (!parent.exists()) {
        parent.mkdirs();
    }

    FileWriter w = new FileWriter(f, append);
    try {
        w.write(data);
    } finally {
        w.close();
    }
}

With the simplification to writeToFile(), getPathSeparator() might not be necessary anymore.

\$\endgroup\$
2
\$\begingroup\$

First of all name your variables properly. Name of the variable should describe what it is, but f is just a random letter not a name for variable.

Consider using logger instead of printing to sout.

Stream closing should go to the finally block not in try.

You are nesting a lot of loops and ifs. Try to separate it into methods soo you and up with

while (line = ...) {
  processLine(line);
}

void processLine(String) {
 if (...) {
  method1();
 } else {
  method2();
 }
}

I also think that methods like writeToFile() Should take a File instance as an argument - it should be responsible for writing to the file not opening it or creating some directories. Same for readFile method.

\$\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.