6
\$\begingroup\$

Scenario: Count the total number of times a char occurs in a word.

For instance, "aaabbcccdddaaaa" should print out "a7b2c3d3".

Can you please critique my code?

import java.util.HashMap;
import java.util.Map;

public class Demo {

    public static void main(final String[] args) {
        final Demo d = new Demo();
        final String name = "aaabbcccdddaaaa";
        System.out.println(d.countNoOfInteger(name));
    }

    public String countNoOfInteger(final String str) {
        if (str.equals(null)) {
            throw new IllegalArgumentException();
        }
        if (str.isEmpty()) {
            return "";
        }

        final Map<Character, Integer> map = new HashMap<>();
        int count = 0;
        final char[] charArray = str.trim().toCharArray();

        for (final char element : charArray) {
            if (map.get(element) != null) {
                count = map.get(element);
                map.put(element, ++count);
            } else {
                count = 0;
                map.put(element, ++count);
            }
        }

        int total;
        char ch;
        final Map<Character, Boolean> visited = new HashMap<>();
        final StringBuilder sb = new StringBuilder();
        for (final char element : charArray) {
            if ((visited.get(element) == null)) {
                ch = element;
                sb.append(ch);
                total = map.get(element);
                sb.append(total);
                visited.put(element, true);
            }
        }
        return sb.toString();
    }
}
\$\endgroup\$
2
\$\begingroup\$
  • x.equals(null) is either false or throws a NullPointerException. Use x == null, or even better Objects.requireNonNull (which returns the value, if you need it).
  • don't guard against empty values if the code does the right thing anyway
  • use a LinkedHashMap when you need to preserve order
  • maybe a Multiset (e.g. from the Guava library) would be even better here
  • don't use if-else if the ternary operator ? : is easier to read
  • it is controversial if final local vars are a good idea, personally I don't like them
  • don't predefine vars you use in loops etc. - don't worry, HotSpot will do it for you
  • check out the new Java 8 methods on Map (I didn't use them here as I don't know if you can use them)
  • if you have "utility-like" methods like this, consider to make them static

Here is my take:

public static String countNoOfInteger(final String str) {
    Objects.requireNonNull(str);

    Map<Character, Integer> map = new LinkedHashMap<>();
    char[] charArray = str.trim().toCharArray();

    for (char element : charArray) {
        Integer count = map.get(element);
        map.put(element, count == null ? 1 : ++count);
    }

    StringBuilder sb = new StringBuilder();
    for (Map.Entry<Character, Integer> entry : map.entrySet()) {
        sb.append(entry.getKey());
        sb.append(entry.getValue());
    }
    return sb.toString();
}
\$\endgroup\$
3
\$\begingroup\$

The use of Map is unnecessary overhead. You can leverage the fact that chars can be mapped as ints along with a simple array to solve this problem.

private static final int OFFSET = 'a';

public String countNoOfInteger(final String str) {
    final int[] counts = new int[26];  // or 'z' - 'a' + 1, if you prefer
    for (final char character : str.toCharArray()) {
        counts[character - OFFSET]++;
    }

    final StringBuilder output = new StringBuilder();
    for (int i = 0; i < counts.length; i++) {
        if (counts[i] == 0) {
            continue;
        }
        output.append((char) (i + OFFSET));
        output.append(counts[i]);
    }

    return output.toString();
}

Some notes:

  1. OFFSET could probably have a better name. I'll leave that as an exercise for the reader.
  2. Contrary to other respondents, I heartily encourage the use of final as a statement of intent.
  3. In a "real" system, it would be preferable to separate the business logic (find the letter counts) from the display logic (print them to the screen). In such a system, your method would return the counts int[] (or Map) and the caller (main(), in this case) would be responsible for displaying it (everything after the first for loop).
  4. Note that this solution will fail spectacularly for invalid input. If that's something you need to handle, add a check to the first loop to ensure that character is between offset and offset + counts.length.
\$\endgroup\$
  • \$\begingroup\$ The added comment strikes me as insufficient. His example input shows only lower-case letters, but at least to me that seems insufficient basis to rule everything else (even upper-case letters) as "invalid". \$\endgroup\$ – Jerry Coffin Jan 11 '16 at 17:59
  • \$\begingroup\$ @JerryCoffin That's fair. It's unclear what the input set looks like. I'm making assumptions based on one input and having seen this style of problem before. In the general case where every character has to be supported, the int[] would be a poor choice. It'll work well for a reasonable fixed set of chars. Expanding to include 'A' - 'Z' is trivial and the OP should be able to handle that. \$\endgroup\$ – Eric Stein Jan 11 '16 at 18:01
1
\$\begingroup\$

I would suggest you use final only whenever declaring a field in a class. Otherwise, it does not make it any easier to read your code. Also, what comes to the actual routine, you could do something like

public String countNoOfInteger(String str) {
    Objects.requireNonNull(str, "The input string is null.");
    Map<Character, Integer> map = new TreeMap<>();
    char[] chars = str.trim().toCharArray();

    for (char c : chars) {
        map.put(c, map.getOrDefault(c, 0) + 1);
    }

    StringBuilder sb = new StringBuilder();

    map.entrySet()
       .forEach((e) -> { sb.append(e.getKey())
                           .append(e.getValue()); });

    return sb.toString();
}

Note the usage of TreeMap instead of HashMap: this will get your characters lexicographically sorted regardless how they appear in the input string.

\$\endgroup\$
1
\$\begingroup\$

Are many ways to accomplish the character counting task. Could choose to work with the numeric form and convert back.

    public String getCountAsString()
    {
        final List<Integer> characterCodes = Lists.newArrayList();

        for( int x=0; x < _input.length(); x++ ) 
        {
            Integer codeIn = _input.codePointAt( x );

            characterCodes.add( codeIn );

        }//for

        String output = "";

        final Set<Integer> uniqueCharacterCodes = 
                new ImmutableSet.Builder<Integer>()
                                .addAll( characterCodes )
                                .build();

        final Iterator<Integer> uniqueCharacterCodesIterator = 
                uniqueCharacterCodes.iterator();

        while(uniqueCharacterCodesIterator.hasNext())
        { 
            Integer codeCurrent = uniqueCharacterCodesIterator.next();

            output +=  new String(Character.toChars(codeCurrent)) 
                + Collections.frequency( characterCodes, codeCurrent );

        }//while

        return output; 

    }
\$\endgroup\$
  • \$\begingroup\$ You should mention that you use the Google collections from Guava. \$\endgroup\$ – Landei Jan 12 '16 at 7:59
  • \$\begingroup\$ Wasn't positive since not essential to solution. One could be very difficult and argue reinvention of the wheel; i.e., use was an ImmutableSet and Builder of the same interface, retrieved from some much lesser known Maven repository. \$\endgroup\$ – kph0x1 Jan 12 '16 at 13:53

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.