Program to find unique chars in a String (and also to find number of occurences of chars in a String)

Question

I am new to Java and learning about Strings in Java. So I wrote a program which counts the number of times a character repeats in a String and also identifies unique characters(chars not repeated more than once) in a String.

Is this an efficient way to do this?

public static String uniqueValues(String str){
char[] arr = str.toCharArray();
int[] result = NumberOfOccurences.numberOfOccurencesOfLetters(arr);
StringBuilder sb = new StringBuilder();
for(int i=0;i<result.length;i++){
    if(result[i]==1){
        sb.append((char)(i+97));
        sb.append(" ");
        }
    }
return sb.toString();
}

//in the other class i wrote this method
public static int[] numberOfOccurencesOfLetters(char[] arr){
int[] result = new int[26];
int num;
for(int i=0;i<arr.length;i++){
    num =(arr[i]-97);
    result[num]++;
    }
return result;
}

Answer 1

There are a couple of improvements that can be made, mostly readability improvements as I believe your solution is quite efficient.

• Format your code with proper indentation and spacing

Indentation is important for readability, so is spacing; don't be afraid to throw in plenty of whitespace if it makes things easier to read. Notice how I indented on every bracket level and put in white space in between some lines and put spaces in for loops etc.

public static String uniqueValues(String str) {
    char[] arr = str.toCharArray();
    int[] result = NumberOfOccurences.numberOfOccurencesOfLetters(arr);
    StringBuilder sb = new StringBuilder();

    for(int i = 0; i < result.length; i++) {
        if(result[i]==1) {
            sb.append((char)(i+97));
            sb.append(" ");
        }
    }

    return sb.toString();
}

//in the other class i wrote this method
public static int[] numberOfOccurencesOfLetters(char[] arr) {
    int[] result = new int[26];
    int num;

    for(int i = 0; i < arr.length; i++) {
        num = (arr[i]-97);
        result[num]++;
    }

    return result;
}

• Pass in a string to numberOfOccurencesOfLetters instead of a char[].

Assuming you'll be using this method mostly of strings, you should pass in a string and do the conversion inside numberOfOccurencesOfLetters rather than before calling it every time.

public static int[] numberOfOccurencesOfLetters(String str) {
        char[] arr = str.toCharArray();
        int[] result = new int[26];
        int num;

        for(int i = 0; i < arr.length; i++) {
            num = (arr[i]-97);
            result[num]++;
        }

        return result;
    }

• Declare variables closest to where they're used and limit their scope as much as possible.

For example int num; can exist only within the for loop instead of the whole function scope as we only use it inside the for loop.

        for(int i = 0; i < arr.length; i++) {
            int num = (arr[i]-97);
            result[num]++;
        }

• Return a HashMap instead of int[] for numberOfOccurencesOfLetters.

Google what is a map in Java. This sacrifices some speed, but you'll get much more readable code with a return type that's easier to manipulate. Also, you won't be limited to the 26 characters that you have now and you'll be able to have upper-case letters and special characters as well.

This is how your whole code would look like with this implementation. If it doesn't compile I'll fix it when I get home.

import java.util.HashMap;

public static String uniqueValues(String str) {
    HashMap<Character, Integer> result = NumberOfOccurences.numberOfOccurencesOfLetters(str);
    StringBuilder sb = new StringBuilder();

    for (Character key : result.keySet()) {
        sb.append(key);
        sb.append(" ");
    }

    return sb.toString();
}

public static Map<Character, Integer> numberOfOccurencesOfLetters(String str) {
        HashMap<Character, Integer> hmap = new HashMap<Character, Integer>();
        char[] arr = str.toCharArray();

        for(int i = 0; i < arr.length; i++) {
            Character key = arr[i];
            hmap.put(key, hmap.getOrDefault(key, 0) + 1);
        }

        return hmap;
 }

Answer 2

The operations you want are already included in the Streams API.

// code updated with suggestion from Nevay
public static void main(String[] args) {
    String string = "feifjwo";
    Map<Character, Long> frequencies = string.chars()
            .mapToObj(i -> (char) i)
            .collect(Collectors.groupingBy(i -> i, Collectors.counting()));

    System.out.println(frequencies);
    System.out.println(frequencies.keySet().stream()
            .distinct()
            .collect(Collectors.toSet()));
}

---

EDIT adding some actual review:

@Majiick already gave good advice.

I would add that naming variables is important. For example, instead of result, occurrences would have been better.

It seems there is a bug with if (result[i] == 1) in uniqueValues. I think you meant if (result[i] >= 1) which would also print characters that appear more than once.

Otherwise I don't think there is any performance problem.

Answer 3

As addition to the @Majiick comments.

There is few more things that could improve the readability of the code:

• The name of the method #numberOfOccurencesOfLetters is too verbose and adds too many 'noice' and since it is used in the context of the NumberOfOccurences class, the prefix 'numberOfOccurrences' could be removed.

I would also propose you the following implementation of NumberOfOccurences class, which would make it more functional

class CharSequence {

    private final char[] sequence = sequence;

    public CharSequence(String sequence) {
        this(sequence.toCharArray());
    }

    public CharSequence(char[] sequence) {
        this.sequence = sequence;
    }

    public Set<Character> uniques() {
        Set<Character> uniques = new HashSet<Character>();

        Map<Character, Integer> occurences = this.occurences();
        for (Map.Entry<Character, Integer> entry : occurences.entrySet()) {
            if (entry.getValue() == 1) {
                uniques.add(entry.getKey());
            }
        }

        return uniques;
    }

    public Map<Character, Integer> occurences() {
        Map<Character, Integer> occurences = new HashMap<>();

        for (int i = 0 ; i < sequence.length ; i++) {
            Character key = sequence[i];

            occurences.put(key, occurences.getOrDefault(key, 0)++);
        }

        return occurences;
    }
}

I have implemented it with Map, as @Majiick suggest, I also took his aproach of implementation. In addition I have also used Map.Entry, which you could also take a look.

In the end your code would look like this one:

public static String uniqueValues(String str) {
    CharSequence sequence = new CharSequence(str);

    StringBuilder sb = new StringBuilder();
    for (Character character : sequence.uniques()) {
        sb.append(character).append(" ");
    }

    return sb.toString();
}

Since Java 8 you could also use the new join method of String class, with which you could get rid of StringBuilder usage. With its usage, the code will look like this:

public static String uniqueValues(String str) {
    CharSequence sequence = new CharSequence(str);

    return String.join(sequence.uniques(), " ");
}

Please note that by implementing it in this way:

• It is implemented in a OOP way
• You could hide the string conversion into the class, this also make the class more functional, since you could make instances of the class using many different types of objects
• There is also more places of performance improvments e.g. you could cache the result that is returned from occurrences() method and add more functionality on it
• The methods are less verbose and still easy to understand