2
\$\begingroup\$

The task:

Determine if a word or phrase is an isogram. An isogram (also known as a "nonpattern word") is a word or phrase without a repeating letter, however spaces and hyphens are allowed to appear multiple times.

Examples of isograms:

  • lumberjacks
  • background
  • downstream
  • six-year-old

The word isograms, however, is not an isogram, because the s repeats.

Even though I'm aware that questions about isograms have already been posted, in this variation of the problem spaces and hyphens are allowed to appear multiple times so that you can not use a solution such as len(string) == len(set(string))

What bothers me about my solution is hardcoding bounds of ascii character ranges and using collections library for such a small problem. I'm wondering if there is a better way to do it.

Here is my code:

from collections import Counter

ASCII_LOWER_BOUND = 97
ASCII_UPPER_BOUND = 123

def is_isogram(string):
    char_counts = Counter(string.lower())
    return all(
        char_counts[char] == 1 
        for char in char_counts 
        if ord(char) in range(ASCII_LOWER_BOUND, ASCII_UPPER_BOUND + 1)
        )

And a test suite provided by exercism:

import unittest

from isogram import is_isogram

# Tests adapted from `problem-specifications//canonical-data.json` @ v1.7.0


class IsogramTest(unittest.TestCase):
    def test_empty_string(self):
        self.assertIs(is_isogram(""), True)

    def test_isogram_with_only_lower_case_characters(self):
        self.assertIs(is_isogram("isogram"), True)

    def test_word_with_one_duplicated_character(self):
        self.assertIs(is_isogram("eleven"), False)

    def test_word_with_one_duplicated_character_from_the_end_of_the_alphabet(self):
        self.assertIs(is_isogram("zzyzx"), False)

    def test_longest_reported_english_isogram(self):
        self.assertIs(is_isogram("subdermatoglyphic"), True)

    def test_word_with_duplicated_character_in_mixed_case(self):
        self.assertIs(is_isogram("Alphabet"), False)

    def test_word_with_duplicated_character_in_mixed_case_lowercase_first(self):
        self.assertIs(is_isogram("alphAbet"), False)

    def test_hypothetical_isogrammic_word_with_hyphen(self):
        self.assertIs(is_isogram("thumbscrew-japingly"), True)

    def test_hypothetical_word_with_duplicated_character_following_hyphen(self):
        self.assertIs(is_isogram("thumbscrew-jappingly"), False)

    def test_isogram_with_duplicated_hyphen(self):
        self.assertIs(is_isogram("six-year-old"), True)

    def test_made_up_name_that_is_an_isogram(self):
        self.assertIs(is_isogram("Emily Jung Schwartzkopf"), True)

    def test_duplicated_character_in_the_middle(self):
        self.assertIs(is_isogram("accentor"), False)

    def test_same_first_and_last_characters(self):
        self.assertIs(is_isogram("angola"), False)


if __name__ == "__main__":
    unittest.main()
```
\$\endgroup\$
2
\$\begingroup\$

ord()

Do not define constants like ASCII_LOWER_BOUND, use ord('a'). Easy to read, no uncertainty about the value.

character range/set

Do not use an integer range and ord(). It is error prone and hard to review.

if ord(char) in range(ASCII_LOWER_BOUND, ASCII_UPPER_BOUND + 1)

rewrites to

import string
if char in string.ascii_lowercase

No off by one, easy to read. If the test has to be very fast prepare a set.

generator expression

Counter is derived from dict. So instead of

(char_counts[char] == 1 for char in char_counts if char in string.ascii_lowercase)

we do use dict().items()

(count == 1 for char, count in char_counts.items() if char in string.ascii_lowercase)

Naming

In general you should try to avoid names that hide common python names. As we want to import string we need a different name for the parameter.

from collections import Counter
import string

def is_isogram(word):
    char_counts = Counter(word.lower())
    return all(count == 1 for char, count in char_counts.items() if char in string.ascii_lowercase)
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Actually string paramater name is used by Exercism. Don't understand why you loop through char_counts.items() instead of original char_counts. \$\endgroup\$ – Konstantin Kostanzhoglo Jun 24 at 1:13
  • 2
    \$\begingroup\$ looping - By iterating over char_counts I get the keys only. By iterating over char_counts.items() I get key/value pairs. I can use count directly without having to dereference char_counts[char]. This is more efficient. \$\endgroup\$ – stefan Jun 24 at 16:51
  • \$\begingroup\$ a set is better for a containment check than a str, so I would do lower_cases = set(string.ascii_lowercase) and then later if char in lower_cases \$\endgroup\$ – Maarten Fabré Jun 25 at 6:44
  • \$\begingroup\$ @Maarten. My words above :-) \$\endgroup\$ – stefan Jun 25 at 7:14
3
\$\begingroup\$

You can iterate once over text and short-circuit right away if any character is not unique.

import string


def is_isogram(text):
    seen = set()
    for char in text:
        if (lower := char.lower()) in string.ascii_lowercase:
            if lower in seen:
                return False
            else: 
                seen.add(lower)
    return True
| improve this answer | |
\$\endgroup\$

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.