0

I'm writing a simple Java program that asks the user to input a string, and then counts and displays the number of times each letter in the alphabet appears in that string. When I compile, I get this error:

Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: -25
at StringLetters.countLetters(StringLetters.java:43)
at StringLetters.main(StringLetters.java:23)

I have looked at other solutions to problems similar to mine, but none of them helped. Does anyone have any ideas? Thank you.

    import java.util.Scanner;

    public class StringLetters
    { 
    public static void main(String[]args) 
    { 
        Scanner scan = new Scanner(System.in);
        System.out.println("Please enter a string of words.");
        String s = scan.nextLine();

        int[] counts = countLetters(s.toUpperCase());

        for(int i = 0; i < counts.length; i++)
        {
            if (counts[i] != 0) 
            {
                System.out.println((char)('a' + i) + " appears " + counts[i] + ((counts[i] == 1) ? "time" : " times"));
            }
        }
    }


    public static int[] countLetters(String s)
    {
        int[] counts = new int[26];

        for (int i = 0; i < s.length(); i++)
        {
            if(Character.isLetter(s.charAt(i)))
            {
                counts[s.charAt(i) - 'a']++;
            }
        }

        return counts;
    }

}
Improve this question
5
  • 1
    What do you think the exception is trying to tell you??? Commented Jul 27, 2014 at 21:20
  • Step though your code with a debugger. I would suggest you use a Map rather than an int[]. Commented Jul 27, 2014 at 21:21
  • How do you account for capital letters? Seems like a good use for HashMap Commented Jul 27, 2014 at 21:21
  • 1
    Why do you do this: countLetters(s.toUpperCase()); and then this: s.charAt(i) - 'a'?? Commented Jul 27, 2014 at 21:23
  • 1
    BTW, if you weren't an obvious newbie I'd give you a downvote for not identifying line 43 in your listing. This is something you should always do. Commented Jul 27, 2014 at 21:25

5 Answers 5

Reset to default
2

In your argument String which you pass to the counting method, you have letters which are not lowercase English letters and that breaks your code.

In fact none of them are lowercase because you're calling s.toUpperCase() and it seems you meant to call s.toLowerCase(). Also, you need to filter out punctuation and any non-letter characters. You're already doing it here: if (Character.isLetter(s.charAt(i))).

So just change s.toUpperCase() to s.toLowerCase() and you'll be fine.

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

NONE of the letters are lowercase!!
This is the best answer here. It also seems to be one of the only ones to earn a downvote. Some people behave oddly.
@DavidWallace - It was downvoted because he was wrong until I told him so, then he edited in a correction. And SO doesn't show that edit because it randomly doesn't show answer edits done within the first 15 minutes or so.
OK, fair enough. I take it back about odd behaviour. @HotLicks
2

Well, obviously the index in counts[s.charAt(i) - 'a'] gets above 25. It looks like you are assuming all your inputs would be lower case letters, even though you are passing to your countLetters method an upper case String (countLetters(s.toUpperCase());).

You should probably change it to counts[s.charAt(i) - 'A']. Of course, that would only work for capital letters, but that's what you are currently passing to this method.

Improve this answer

Comments

1

First, your issue - how do you deal with capital letters?

Obviously in your use case you do countLetters(s.toUpperCase()) then you do - 'a'. This gives very odd results:

(int)'A' - (int)'a' = -25
(int)'Z' - (int)'a' = -7

An easier way to do this is using a Map. With Java 8:

    final Map<Character, Integer> counts = input.toUpperCase().chars().
            collect(HashMap::new, (m, l) -> m.merge((char) l, 1, Integer::sum), Map::putAll);
Improve this answer

4 Comments

It's elegant. But for the performance point of view, a simple array is better than a map in this particular case because there is only 26 distinct values and we know them.
@mcoolive premature optimization is the root of all evil - Donald Knuth. There is no reason to turn readable and clear code into array based mess unless there has been benchmarking that results in tangible proof of performance problems.
My initial comment was NOT about the readability, I said : a MAP is NOT the best algorithmic structure in this case. By the wy, in my opinion, your example is not more readable than an array. But this point is more subjective...
@mcoolive you can write FORTRAN in any language.
1

isLetter can return true in many case (have a look on its javadoc). In particular for uppercase character (peter>petrov is right, you probably don't want to convert to upper cases...)

I advice to implement a more basic test:

        Char c = s.charAt(i);
        if('a' <= c && c <= 'z')
        {
            counts[c - 'a']++;
        }
        else if('A' <= c && c <= 'Z')
        {
            counts[c - 'A']++;
        }
Improve this answer

7 Comments

A "more basic" test? You can use Character.isLowerCase and Character.isUpperCase rather than your mess.
I prefer to use inline tests. It's save a static method call and two comparisons is NOT a mess for me. It's more an habbit because the gain in performance is very very low... My opinion.
This looks clean. Compared to the version postet in the question. If someone wants to substract characters which is an operation designed for numbers, this is as proper as you can get. Best, but because of the toUpperCase() only the part in the else if is needed.And: "isLetter can return true in many case " is the correct answer to this question,as this is the source of the negativ index.
It's save a static method call. This is never a reason to do anything. Character.isLowerCase is clear and readable - it spells out your intent. The combination of implicit coercion of char to int and integer comparison is far less readable.
Yes. Just like substracting chars. The asker obivously does not intent to write clean code at all. Rather he has some confusing ideas of how to to things. So I think this answer is most near to his way of thinking as you can get. And it is clear. Even though the mentioned Character.isUpperCase is better. And 'A' - 'a' will give an negative index. If using an Ascii - Table.
|
0

How do you account for capital letters? Seems like a good use for HashMap. Capital letters have a lower ASCII value than lower case letters. It looks like your index out of bounds is caused by a capital letter.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.