Constructing a long string

Question

I have a task to construct a long properly formatted string in Java. This string will be then used as a header of the logfile that we write during the program execution. As the program runs on server continuously, this string may need to be returned via the overridden method more than once. I have used multiple StringBuilders and some flags to make sure I don't do anything repeatedly or unnecessarily. But I am still looking for ways to improve the performance of this code.

@Plugin(name = "property", category = StrLookup.CATEGORY)
public class CustomLookup extends AbstractLookup {

    private static final String LINE_SEPARATOR = System
            .getProperty("line.separator");

    private static AtomicLong aLong = new AtomicLong(0);

    private StringBuilder formattedString;
    private StringBuilder javaPropsString;
    private StringBuilder pathPropsString;
    private StringBuilder osPropsString;
    private StringBuilder otherPropesString;
    // flag to determine if we already have formatted string ready
    // as we don't want to create new string every-time.
    private boolean isStringReady = false;

    // flag to determine if environment variables are added to system
    // properties.
    private boolean isEnvPropsSet = false;

    private Properties properties;

    @Override
    public String lookup(LogEvent event, String key) {

        // if this is the first call (i.e. system startup) we
        // don't log the system properties.
        if (aLong.getAndIncrement() == 0)
            return "";
        // on any subsequent calls i.e. on file rollover
        // we return the formatted string with system properties.
        else if (key.equalsIgnoreCase("customKey")) {
            prepareCustomLog();
            return formattedString.toString();
        } else
            return "";

    }

    /**
     * This method prepares the custom string in a readable format. It checks if
     * the string is already prepared with the {@link #isStringReady} flag.
     */
    private void prepareCustomLog() {

        // If already prepared a string in previous call then do nothing here
        // as the string will be ready.
        if (!isStringReady) {
            formattedString = new StringBuilder();
            javaPropsString = new StringBuilder();
            osPropsString = new StringBuilder();
            pathPropsString = new StringBuilder();
            otherPropesString = new StringBuilder();

            // If environment properties are not added to system properties add
            // them now.
            if (!isEnvPropsSet)
                addEnvProps();

            properties = System.getProperties();
            Set<Object> set = properties.keySet();
            set.removeAll(getIgnoreList());
            Iterator<Object> i = set.iterator();

            formattedString.append(LINE_SEPARATOR)
                    .append("/*********** System properties *************/")
                    .append(LINE_SEPARATOR);

            javaPropsString.append(LINE_SEPARATOR).append("        ")
                    .append("JAVA Properties").append(LINE_SEPARATOR);

            osPropsString.append(LINE_SEPARATOR).append("        ")
                    .append("OS Properties").append(LINE_SEPARATOR);

            pathPropsString.append(LINE_SEPARATOR).append("        ")
                    .append("PATH variables and properties")
                    .append(LINE_SEPARATOR);

            otherPropesString.append(LINE_SEPARATOR).append("        ")
                    .append("Other Properties").append(LINE_SEPARATOR);

            while (i.hasNext()) {
                String key = i.next().toString();

                // filter out any keys those may contain privacy sensitive
                // details.
                if (!(key.toLowerCase().contains("user"))
                        && !(key.toLowerCase().contains("home"))
                        && !(key.toLowerCase().contains("system"))
                        && !(key.toLowerCase().contains("program"))) {
                    if (key.contains("java"))
                        appendJavaPropsString(key);
                    else if (key.contains("os"))
                        appendOsPropsString(key);
                    else if (key.contains("path"))
                        appendPathPropsString(key);
                    else
                        appendOtherPropesString(key);
                }
            }
            formattedString.append(javaPropsString).append(osPropsString)
                    .append(pathPropsString).append(otherPropesString);
            formattedString.append(
                    "/*********** System properties *************/").append(
                    LINE_SEPARATOR);
            isStringReady = true;
        }
    }

    private void addEnvProps() {
        for (Map.Entry<String, String> entry : System.getenv().entrySet()) {
            System.setProperty(entry.getKey(), entry.getValue());
        }
    }

    private void appendJavaPropsString(final String key) {
        javaPropsString.append("                ").append(key).append(" : ")
                .append(properties.get(key)).append(LINE_SEPARATOR);
    }

    private void appendOsPropsString(final String key) {
        osPropsString.append("                ").append(key).append(" : ")
                .append(properties.get(key)).append(LINE_SEPARATOR);
    }

    private void appendPathPropsString(final String key) {
        pathPropsString.append("                ").append(key).append(" : ")
                .append(properties.get(key)).append(LINE_SEPARATOR);
    }

    private void appendOtherPropesString(final String key) {
        otherPropesString.append("                ").append(key).append(" : ")
                .append(properties.get(key)).append(LINE_SEPARATOR);
    }

    /**
     * This method returns a set of keys that we do not require to log, or the
     * keys which may have some sort of private information associated with it.
     * 
     * @return set of keys to ignore when logging.
     */
    private Set<Object> getIgnoreList() {
        Set<Object> ignore = new HashSet<Object>();

        ignore.add("M2");
        ignore.add("COMPUTERNAME");
        ignore.add("ProgramData");
        ignore.add("HOMEPATH");
        ignore.add("LOGONSERVER");
        ignore.add("LOCALAPPDATA");
        ignore.add("PUBLIC");
        ignore.add("APPDATA");
        return ignore;
    }
}

Answer 1

Thread Safety

This code is not tread safe. Something as low level as a logging framework will be called in multiple threads.

isStringReady is checked at the very beginning and not set until the very end. Nothing stops multiple threads from getting passed the first check of isStringReady before the first thread completes the prepare operation from completing.

Additionally, since the string builders are instance variables and set to new instances at the beginning of execution it is possible to return nothing after setting up all the values. Thread-1 runs through prepareCustomLog() up until isStringReady is set. Thread-2 starts executing prepareCustomLog() and re-initializes all of the sting builders. Then, Thread-1 returns and calls formattedString.toString(); Here formattedString has just been initialize and never appended to.

---

Memory Leak

The used memory won't keep growing, but you are holding on to a number of resources that are only meant to be used once. Once prepareCustomLog() returns, formattedString is the only builder that will be used from that point on. However, all of the other builders will be in scope until this class is no longer used (based on the description, that never happens).

---

Extra work

Calling toString() on a builder is a costly operation. If you don't plan on changing the contents of the builder, you should store that resulting string instead of continually reconstructing the result.

---

Repeated code

The private helper methods for appending to the various builders are all the same. The only difference is which builder they append to. It would be better to have one method that is passed a builder as an argument instead of four methods.

Your check to see if a key should be appended also repeats the same operation for a number of constant values. You could replace that with a static array that you loop over and check. This should also be it's own stand alone method as it is hard to tell where the one if clause ends and the next one starts.

---

Collecting the last few points together

private final Object lock = new Object();
private final String[] keysToIgnore = {"user", "home", "system", "program"};
private String customLog;


@Override public String lookup(LogEvent event, String key) {

    // if this is the first call (i.e. system startup) we
    // don't log the system properties.
    if (aLong.getAndIncrement() == 0) {
        return "";
    } else if (key.equalsIgnoreCase("customKey")) {
        synchronized (lock) {
            if (customLog == null) {
                customLog = makeCustomLog();
            }
        }
        return customLog;
    } else {
        return "";
    }
}

private String makeCustomLog() {
    StringBuilder formatted = new StringBuilder();
    StringBuilder javaProps = new StringBuilder();
    StringBuilder osProps = new StringBuilder();
    StringBuilder pathProps = new StringBuilder();
    StringBuilder otherPropes = new StringBuilder();

    // ... Basic initialization ...

    while (i.hasNext()) {
        String key = i.next().toString();

        //filter out any keys those may contain privacy sensitive details.
        if (!shouldIgnore(key)) {
            if (key.contains("java"))
                append(javaProps, key);
            else if (key.contains("os"))
                append(osProps, key);
            else if (key.contains("path"))
                append(pathProps, key);
            else
                append(key);
        }
    }
    formatted.append(javaProps).append(osProps)
            .append(pathProps).append(otherPropes);
    formatted.append(
            "/*********** System properties *************/").append(
            LINE_SEPARATOR);
    return formatted.toString();
}

private boolean shouldIgnore(String key) {
    String loweredKey = key.toLowerCase(); //only do this once
    for (String match : keysToIgnore) {
        if (loweredKey.contains(match)) {
            return true;
        }
    }
    return false;
}

private void append(final StringBuilder builder, final String key) {
    builder.append("                ").append(key).append(" : ")
            .append(properties.get(key)).append(LINE_SEPARATOR);
}

---

Smaller Points

• The "String" suffix on your builder variables don't add anything. A case could be made for them actually being bad as the variables are not Strings.

• isEnvPropsSet is never set, so addEnvProps() will never be called.

• Always including braces around one line if blocks. It makes it clearer where one ends and the next one begins. There is also the advantage of allowing you to always add one line to a block and not needing to worry about if the second line will not execute unconditionally.

• properties is another reference that you won't need once the string is created.

Answer 2

• Comments should be used to describe why something is done, What is done should be described by the code itself using meaninful names for variables, parameters, methods and classes. If you need a comment to describe what the purpose of a variable is, you have named the variable not that good.

• you shouldn't shorten variables names, because this will remove readability. Mr.Maintainer still needs to understand the code in 6 months at first glance.

// flag to determine if environment variables are added to system
// properties.
private boolean isEnvPropsSet = false;  

here you need a comment to describe what for the variable is used. So a better name would be areEnvironmentPropertiesSet.

• for making your code less errorprone you should use braces {} for single if, else statements and also for single statements of loops.