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;
}

• gyre: 97 is a magic number. I would suggest using 'a' instead because that clarifies your intent.

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;
 }

• LoneStar: Thanks! I dont have knowledge about Maps as of now. Taking baby steps. But yeah, I'll keep proper indentation and naming convention in mind from now.
• gyre: You might want to declare your map variables using the generic interface Map instead of the class HashMap. That way, your code is not tied to a specific implementation of Map should you ever decide to change it.
• mtj: @Majiick You can get rid of the old map-get-if-null-create-and-put construct via using the Java 8 function getOrDefault. (hmap.put(key, hmap.getOrDefault(key, 0) + 1))
• Majiick: @mtj Thanks for the suggestion, I put it in.

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.

• LoneStar: Thanks. The main aim of all this was to code most of it myself, so that I have a better understanding, as I'm new to Java. I don't have knowledge about maps and other data structures, so I'll have to look into that before I can use them.
• toto2: OK, sure. I'll leave it here since it could be interest to others. It's not just Maps, but Streams, which is quite recent (Java 8). You probably want to wait before playing with those.
• LoneStar: Great! Anyway, how is my code performance wise? Any way I can improve it?
• toto2: @LoneStar I added some actual review comments.
• Nevay: Your stream version is unnecessary verbose, string.chars().mapToObj(i -> (char) i).collect(groupingBy(i -> i, counting())) is sufficient to create a Map<Character, Long>.

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