current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I have been playing with Java for around a year and I started writing this API for storing data in files. I wrote this method which will save the parameters in a "key: value" format in a file.

The method is getting fairly long and I am just wondering if there is a better/shorter/more efficient way to do this.

public void set(String key, Object value) {
    try {
        BufferedWriter bw = new BufferedWriter(new FileWriter(this.path, true));
        BufferedReader br = new BufferedReader(new FileReader(this.path));
        ArrayList<String> file = new ArrayList<String>();
        if(this.getValue(key) != null) {
            for(String line; (line = br.readLine()) != null;) {
                if(line.startsWith(this.COMMENT_PREFIX)) {
                    file.add(line);
                } else if(line.isEmpty()) {
                    file.add("");
                } else {
                    try {
                        if(!line.contains(":")) {
                            throw new MyFormatException("Missing a colon!");
                        }
                    } catch(MyFormatException e) {
                        e.printStackTrace();
                        bw.close();
                        br.close();
                        return;
                    }
                    String lineKey = line.substring(0, line.indexOf(":"));
                    String lineValue = line.substring(line.indexOf(":") + 2);
                    if(lineKey.equals(key)) {
                        if(value instanceof ArrayList) {
                            StringBuilder newValue = new StringBuilder(value.toString());
                            newValue.replace(value.toString().lastIndexOf("]"), value.toString().lastIndexOf("]") + 1, "");
                            newValue.replace(value.toString().indexOf("["), value.toString().indexOf("[") + 1, "");
                            bw.append(key + ": " + newValue.toString());
                            bw.newLine();
                        } else {
                            file.add(lineKey + ": " + value);
                        }
                    } else {
                        file.add(lineKey + ": " + lineValue);
                    }
                }
            }
            this.clear();
            for(String line : file) {
                bw.append(line);
                bw.newLine();
            }
        } else {
        if(value instanceof ArrayList) {
                StringBuilder newValue = new StringBuilder(value.toString());
                newValue.replace(value.toString().lastIndexOf("]"), value.toString().lastIndexOf("]") + 1, "");
                newValue.replace(value.toString().indexOf("["), value.toString().indexOf("[") + 1, "");
                bw.append(key + ": " + newValue.toString());
                bw.newLine();
            } else {
                bw.append(key + ": " + value);
                bw.newLine();
            }
        }
        br.close();
        bw.close();
    } catch(Exception e) {
        e.printStackTrace();
    }
}
share|improve this question
4  
Could you give the full class listing, some optimizations may involve changing the way the class is designed. –  bowmore Nov 21 '14 at 19:21

1 Answer 1

up vote 4 down vote

There are many things deeply wrong with this method:

  • It does too much. It seems completely unaware of the single responsibility principle. It's extremely important that every method has one clear main responsibility. This method has so many it's not even clear which one is the most important:

    • Parse a config file?
    • Update a config file?
    • Update an internal hashmap?

    You really need to split this up.

  • The entire method body is wrapped in one giant try-catch block. This is a bad practice. There are many points inside where an exception can be thrown, and by wrapping the entire method like this, you cannot distinguish the failure point and respond gracefully. And since you catch all exceptions and just print on the console (another bad practice by the way), the method will appear to have succeeded, even if it actually failed. As such, the method cannot be trusted.

  • The way you use MyFormatException is absolutely awful. You throw this poorly named custom exception just to catch it yourself. A simple if statement is the right tool for this, better in every way.

  • Methods shouldn't be too long in general. Even when they really have a single responsibility. Long methods are hard to understand, test and debug.

A general purpose utility like the one you described and hinted by this implementation can be certainly accomplished in a simpler way. After you split up this method to smaller functions where each has a clear single responsibility, the resulting code will be longer than this method. But it will be composed of smaller component that are easier to understand and test.

There are many other smaller practical problems too:

  • Declare variables with interface type instead of implementation when possible. For example List instead of ArrayList, like this: List<String> file = new ArrayList<>();

  • file is a very poor name for a list. It's a list, so name it that way. When I saw file at many places in the code my instinct was that it's probably a File instance, but it's far from it. Very confusing.

  • The usage this.COMMENT_PREFIX suggests that COMMENT_PREFIX is an instance variables, despite the all capital name which is a convention reserved for static final constants.

I suggest to completely rework this class, splitting up this one big method to smaller methods where each has precisely one responsibility, and post it for another review.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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