4
\$\begingroup\$

These classes are to count keys in a text file. The file has key, value pair like these examples:

John, 12
Sara, 2
Adam, 19
John, 1
Adam, 3

and the main class FileKeyCounter prints output as:

The total for John is 13. The total for Sara 2. The total for Adam is 22.

It consists of 3 classes:

  • FileKeyCounter: This class is responsible to read given .txt file line by line and control the flow.
  • HashMapHandler: This class is responsible to build hashmap with keys(John, Adam, Sara). Since the .txt file may have a big string number that overflows Integer, it calculates the addition using two string value.
  • LineData: This class is responsible to validate the line, the key and value pair.

Please review this.

public class FileKeyCounter {


        private HashMapHandler hash;

        public FileKeyCounter() {
            hash = new HashMapHandler();
        }

        public void countKeys(String fileName) {

            FileReader fileReader = null;
            BufferedReader reader = null;

            try {
                fileReader = new FileReader(fileName);
                reader = new BufferedReader(fileReader);

                String line = reader.readLine();
                while (line != null) {
                    LineData lineData = new LineData(line);
                    if (lineData.isValidLine()) {
                        hash.buildHash(lineData);
                    }
                    line = reader.readLine();
                }
                hash.printHash();
            } catch (IOException e) {
                e.printStackTrace();
            } finally {
                close(fileReader, reader);
            }
        }

        private void close(FileReader fileReader, BufferedReader reader) {
            try {
                if (fileReader != null) {
                    fileReader.close();
                }
                if (reader != null) {
                    reader.close();
                }
            } catch (IOException e) {
                e.printStackTrace();
            }
        }

        public HashMapHandler getHash() {
            return hash;
        }

        public void setHash(HashMapHandler hash) {
            this.hash = hash;
        }

    }

    public class LineData {

        private static final String COMMA = ",";

        private String key;
        private String value;
        private boolean validLine;

        public LineData(String line) {
            String[] pair = line.split(COMMA);
            if (validateLine(pair)) {
                key = pair[ArrayEnum.KEY.getIndex()].trim();
                value = pair[ArrayEnum.VALUE.getIndex()].replaceFirst("^0*","").trim();
                setValidLine(true);
            } else {
                setValidLine(false);
            }
        }

        /**
         * The method return false under below condition
         * 1. If the given array is null or the array length is not 2 
         * 2. If the first element of the array is empty string
         * 3. If the second element of the array is not positive number or zero
         *  
         * @param pair 
         * @return
         */
        public boolean validateLine(String[] pair) {

            if (pair == null || pair.length != ArrayEnum.SIZE.getIndex()) {
                return false;
            }

            return validateKey(pair[ArrayEnum.KEY.getIndex()].trim())
                    && validateValue(pair[ArrayEnum.VALUE.getIndex()].trim());
        }

        /**
         * validate if the key has meaningful value. For example, emptyString ,"",
         * would consider faulty input
         * 
         * @param key
         * @return true if key has certain value false if key does not have any
         *         value
         */
        private boolean validateKey(String key) {
            if (key.length() == 0) {
                return false;
            } else {
                return true;
            }
        }

        private boolean validateValue(String value) {
            return value.matches("\\d+") && !value.startsWith("-") && !value.matches("0+");
        }

        public String getKey() {
            return key;
        }

        public void setKey(String key) {
            this.key = key;
        }

        public String getValue() {
            return value;
        }

        public void setValue(String value) {
            this.value = value;
        }

        public boolean isValidLine() {
            return validLine;
        }

        public void setValidLine(boolean validLine) {
            this.validLine = validLine;
        }

    }

    public class HashMapHandler {

        Map<String, String> map = new HashMap<String, String>();

        public void printHash() {
            StringBuffer buffer = new StringBuffer();
            for (Map.Entry<String, String> element : map.entrySet()) {
                buffer.append("The total for ").append(element.getKey())
                        .append(" is ").append(element.getValue()).append(". ");
                System.out.print(buffer.toString());

                buffer.delete(0, buffer.length());
            }
        }

        public void buildHash(LineData data) {
            if (map.containsKey(data.getKey())) {
                String cnt = addTwoString(map.get(data.getKey()) , data.getValue());
                map.put(data.getKey(), cnt);
            } else {
                map.put(data.getKey(), data.getValue());
            }
        }

        public Map<String, String> getMap() {
            return map;
        }

        public void setMap(Map<String, String> map) {
            this.map = map;
        }

        private String addTwoString(String a, String b){
            int[] result = new int[Math.max(a.length(), b.length()) + 1];
            char[] aChar = a.toCharArray();
            char[] bChar = b.toCharArray();

            int overflowDigit = 0;
            int i = a.length()-1;
            int j = b.length()-1;
            int k = result.length -1;
            while ( i >= 0 || j >= 0  ){
                int sum = 0;

                if ( i < 0 ){
                    sum = overflowDigit + Character.getNumericValue(bChar[j]);
                }else if ( j < 0 ){
                    sum = overflowDigit +  Character.getNumericValue(aChar[i]);
                }else{
                    sum = overflowDigit +  Character.getNumericValue(aChar[i]) + Character.getNumericValue(bChar[j]);
                }

                overflowDigit = 0;
                if ( sum > 9 ){
                    overflowDigit++;
                    result[k] = sum - 10;
                }else{
                    result[k] = sum;
                }
                i--;
                j--;
                k--;
            }
            if ( overflowDigit > 0 ){
                result[0] = overflowDigit;
                k--;
            }

            StringBuffer number = new StringBuffer();
            for ( int m = k +1 ; m < result.length ; m++){
                number.append(result[m]);
            }

            return number.toString();
        }

    }
\$\endgroup\$
  • 1
    \$\begingroup\$ Use BigInteger if you really have vaules > 2000000000 in your text file. Don't reinvent additon of big numbers and don't do fast things (addition) with slow String manipulation if you can help it. \$\endgroup\$ – AlexR May 30 '15 at 1:03
3
\$\begingroup\$

Use better abstractions

As I start reading the countKeys method, it's hard to understand the purpose of this code.

From a method named countKeys and taking a file name, I would expect something that counts keys in a file and returns the number of keys. But it's a void method and does something else.

Looking at the implementation, I understand that it parses lines into LineData objects, but what is this HashMapHandler, and what does it mean to buildHash and printHash?

My first guess would be that this is something to do with cryptography or making a digest, but that's not the case. So HashMapHandler, hash, buildHash, printHash are all poorly named elements that don't help the reader understand the code.

Re-think your abstractions

What is the real purpose of "LineData" ? It stores some kind of data. What kind of data? A name and a count. The data comes from a line, but that does that define the data? Would the meaning change if it comes from a web service? No. So NameCount would be a better name.

What is the real purpose of "HashMapHandler" ? Is it really about hash maps? What does it even mean, "handle" ? The main purpose seems to merge NameCount objects, and adding up the counts. It looks like a counter. So Counter would be a better name. The buildHash would be better named add.

Side note: this section should really have been the first. I had to take a closer look at the code to really understand how to do things better, so I wrote this section after. Nonetheless, initial impressions can be interesting too, so I'm leaving it there above for the record.

File handling

When you close the BufferedReader, that will close the underlying FileReader too, you don't need to close the latter explicitly.

You can simplify the file reading using try-with-resources:

    try (BufferedReader reader = new BufferedReader(new FileReader(fileName))) {
        String line;
        while ((line = reader.readLine()) != null) {
            LineData lineData = new LineData(line);
            if (lineData.isValidLine()) {
                hash.buildHash(lineData);
            }
        }
        hash.printHash();
    } catch (IOException e) {
        e.printStackTrace();
    }

With this version, the close method is not needed anymore, you can safely delete it.

If you are not on Java 7 yet, you should be, as versions below that are no longer supported.

I also moved the reader.readLine() into the while condition itself, so that you don't need to write this statement twice.

Java Bean-itis?

There are a lot of setters in this code, but what for? You don't use most of them, and you don't need any of them. Try to write code without setters, and also make fields final whenever possible. See the next section for a concrete example.

LineData

This class is really horrible in many ways.

  • Pointless setters
  • Constructor calling a setter instead of setting a variable directly
  • validLine not needed, a getter with return key != null would do
  • ... it would be even better to prevent creating an invalid object in the first place
  • Constant named COMMA, but what happens if you change its value to something else, for example ; ? The program could work with appropriate data, but it won't make sense to see a variable named "COMMA" in it. Name constants for their purpose, not literally for their value. In this example, SEPARATOR would seem natural.
  • boolean validateSomething is not great naming. A function that returns boolean should typically be named isValidSomething. A function that validates that some assumptions are true would make sense as either:
    • named validateSomething, return void, and throw exception if an assumption is false
    • named getValidSomething or similar, return an object or null if an assumption is false
  • ArrayEnum seems to be used as a store of independent constants, which is a misuse of enums: independent constants don't belong in an enum. The name ArrayEnum is also very poor, as it doesn't describe its purpose.

Consider this alternative implementation:

class LineData {

    private static final String SEPARATOR = ",";

    private static final int KEY_INDEX = 0;
    private static final int VALUE_INDEX = 1;
    private static final int TOKEN_COUNT = 2;

    private final String key;
    private final String value;

    private LineData(String key, String value) {
        this.key = key;
        this.value = value;
    }

    public static LineData fromLine(String line) {
        String[] tokens = parseLine(line);
        if (tokens == null) {
            return null;
        }

        String key = parseKey(tokens[KEY_INDEX]);
        String value = parseValue(tokens[VALUE_INDEX]);
        if (key != null && value != null) {
            return new LineData(key, value);
        }
        return null;
    }

    private static String parseKey(String token) {
        return !token.isEmpty() ? token : null;
    }

    private static String parseValue(String token) {
        if (token.matches("\\d+") && !token.startsWith("-") && !token.matches("0+")) {
            return token.trim();
        }
        return null;
    }

    private static String[] parseLine(String line) {
        String[] tokens = line.split(SEPARATOR);
        if (tokens.length != TOKEN_COUNT) {
            return null;
        }
        return tokens;
    }

    public String getKey() {
        return key;
    }

    public String getValue() {
        return value;
    }
}

Notes:

  • LineData cannot be created directly. It can only be created from a line using the factory method LineData.fromLine
  • The factory method LineData.fromLine tries to parse the line, and either return a valid LineData object, or return null if parsing failed at some point
  • The methods have a single responsibility and hide their implementation details
    • fromLine doesn't know what is a valid key/value and how to extract them. It just knows that a line contains tokens, which include a key and a value, and delegates the parsing of all those to helpers, which return null if something goes unexpected
    • parseLine splits the line to tokens, and if it looks valid so far, it returns the tokens otherwise null to signal the caller that something went wrong
    • parseKey checks the token and returns it if it's a valid key
    • parseValue checks the token, and returns it if it's a valid value
  • The parameters for parsing, such as the separator, the required number of tokens, the index of the key and value are private constants, as no other code uses these. (If they do, you can move them somewhere else.)
  • At no point can an invalid LineData exist. If a valid object cannot be created, the factory method returns null, and this is what callers can use to check if parsing was successful

Poor javadoc

 * @param pair
 * @return

A pair of what? Return what? A good javadoc should explain these elements.

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