7
\$\begingroup\$

I want to check if the given string is a Panagram or not.

A Panagram is a sentence containing every letter of the alphabet at least once.

This is what I've come up with as solution:

import java.util.Scanner;

public class Panagram {

    public static void main(String[] args) {

        Scanner sc = new Scanner(System.in);
        String inputString = "";

        System.out.print("Enter the input: ");
        inputString = sc.nextLine();
        sc.close();

        if (inputString != null && inputString.length() > 0) {
            System.out.print("The given string, \"" + inputString + "\", is " + (isPanagram(inputString) ? "" : ("not ")) + "a Panagram");
        } else {
            System.out.print("Not a valid string!");
        }
    }

    /**
     * Checks if the given input is a panagram.
     * 
     * @param inputString
     * @return boolean
     */

    private static boolean isPanagram(String inputString) {

        if (inputString.length() < 26) {
            return false;
        }

        inputString = inputString.toLowerCase();

        for (char i = 'a'; i <= 'z'; i++) {
            if (!(inputString.contains(i + ""))) {
                return false;
            }
        }

        return true;
    }

}

Any review comments or suggestions?

List of well known Panagrams.

\$\endgroup\$
7
\$\begingroup\$

Your solution is actually quite decent, and the code style is good, etc. There are a couple of style changes I would recommend:

  • In a for loop, the variable i typically indicates an int, and people tend to use c to indicate a char. So, I would write for (char c = 'a'; c <= 'z'; c++) {...

  • your isPanagram method should be public.

Now, about the algorithm, your code will scan the result as many as 26 times. In reality, this will happen fast, and there's no pressing need to change that, but it could be faster, but would require a little work to get it right.

If you reverse the logic, and just iterate through each characters in the String once, then you can reference it back to an index for the characters.... It would go something like:

boolean[] seen = new boolean[26]; // or a constant for the number of characters.
int stillNeed = seen.length;
for (int i = 0; i < inputString.length(); i++) {
    char c = Character.toLowerCase(inputString.charAt(i));
    if (c >= 'a' && c <= 'z') {
        c = c - 'a';
        if (!seen[c]) {
            seen[c] = true;
            stillNeed--;
            if (stillNeed == 0) {
                return true;
            }
        }
    }
}
return false;

Now, for long strings, the above will be more efficient than your solution. It goes through each character in the string just once, but it has to create a list of what's been seen, or not. If it is a new character, it has to count it as seen.

Your mileage may vary.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I'm wondering, is there a reason why you do toLowerCase for each character in the loop instead of once on the String before you start iterating? \$\endgroup\$ – IEatBagels Sep 23 '14 at 14:26
  • 2
    \$\begingroup\$ @TopinFrassi - the toLowerCase only converts the characters you need, and does not take up any more space than a single char. The loop I have can exit early when all characters are found, but converting the whole string to lower and then comparing would require the whole string to be processed even if it was huge, and the first 26 letters were the complete alphabet. \$\endgroup\$ – rolfl Sep 23 '14 at 14:29
  • \$\begingroup\$ @TopinFrassi I an only guess but do you really need to lower all the string if you find the 26 letters before the end of the string. It's really a small optimization, but on big string it could be important. (Well I was right) \$\endgroup\$ – Marc-Andre Sep 23 '14 at 14:29
  • \$\begingroup\$ @rolfl Oh, seems legit! \$\endgroup\$ – IEatBagels Sep 23 '14 at 14:33
  • \$\begingroup\$ What about inputString.length() < 26 check? \$\endgroup\$ – CodeYogi Sep 29 '15 at 4:30
5
\$\begingroup\$

First, there are no fatal problems!

  1. inputString can never be null - nextLine will throw a NoSuchElementException otherwise.
  2. Please use inputString.isEmpty() instead for readabillity and DRY-Principle.
  3. You should avoid the assignment to = "", merge declaration of inputString and assignment by sc.nextLine().
| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

I have little to say, but I think you should join the declaration of inputString with the assignation :

System.out.print("Enter the input: ");
String inputString = sc.nextLine();

And you use the "magic number" 26, I'm a good example of why you shouldn't use it. I always have a hard time remembering if there's 26 or 27 letters in the alphabet and reading your code I wasn't sure why 26 was there (so I had to count from a to z). You should create a constant in order to make it clear :

private final static int numberOfLettersInAlphabet = 26;
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I shirk the constants because a and z must be constants too consequentlike. \$\endgroup\$ – Grim Sep 23 '14 at 14:13
  • \$\begingroup\$ Well I think a and z are self explanatory. I fear the day a won't be the first letter of the alphabet and z the last ;) \$\endgroup\$ – IEatBagels Sep 23 '14 at 14:27
  • \$\begingroup\$ BTW the 27th character is the en.wikipedia.org/wiki/Ampersand \$\endgroup\$ – Grim Sep 23 '14 at 14:30
  • \$\begingroup\$ I didn't learn that in elementary school! \$\endgroup\$ – IEatBagels Sep 23 '14 at 14:34
  • \$\begingroup\$ Me too, i just ranter ;) \$\endgroup\$ – Grim Sep 23 '14 at 14:36

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.