Duplicate characters in a string-CodePudding

Hello I a beginner in python. I am building a small program that can find any duplicate characters in a string. However there's something i don't understand.

Code:

def is_isogram(string):
    dict = {}
    for letter in string:
        dict[letter] = 1
    if letter in dict:
        dict[letter]  = 1
    return dict


print(is_isogram("Dermatoglyphics"))

OUTPUT {'D': 1, 'e': 1, 'r': 1, 'm': 1, 'a': 1, 't': 1, 'o': 1, 'g': 1, 'l': 1, 'y': 1, 'p': 1, 'h': 1, 'i': 1, 'c': 1, 's': 2}

I set an empty dictionary. I then used a for loop to iterate over the string, and then in each iteration it should assign 1 to a dictionary key, "letter"

Then used "if...in" to check if letter has already appeared, and if it has then the the "letter" key should be incremented by 1.

I tried it on a word, Dermatoglyphics, but each time the last key value pair is always 2, even though this word only contains 1 of each letter. Does anyone know why?

CodePudding user response：

if statement applies after finishing for loop, so that it adds 1 only in last character. Its a problem of indentation. Even if you write if condition inside loop, it won't be right because of your logic. You assign dict[letter] = 1 for every letter. Then check if letter in dict, so that it will add 1 two times. Use else condition instead.

def is_isogram(string):
    dict = {}
    for letter in string:
        if letter in dict:
            dict[letter]  = 1
        else:
            dict[letter] = 1
    return dict

print(is_isogram("Dermatoglyphics"))

Or you can use count function like this

def is_isogram(string):
    dict = {}
    for letter in string:
        dict[letter] = string.count(letter)
    return dict

print(is_isogram("Dermatoglyphics"))

CodePudding user response：

You are setting 1 for each, and then you increment the last letter. I think you meant to put if inside for block.

Here is a working version:

def is_isogram(string):
    dct = {}
    for letter in string:
        if letter in dct:
            dct[letter]  = 1
        else:
            dct[letter] = 1
    return dct


print(is_isogram("Dermatoglyphics"))

The logic behind: If the letter already exists, increment counter. Otherwise initialize it with counter=1.

Edit: Changed dict to dct as dict is a python built-in name as @Michael suggested.

CodePudding user response：

As your function is named is_isogram() it should return a boolean. Either the string is an isogram either it isn't. A big benefit is that you stop iterating as soon as you find a duplicate.

You don't need to use a dict. It's not a bad idea but to detect an isogram you don't have the need to count occurrences of each letter. You just have to test the membership. A set is better suited. Like a dict but without the values.

def is_isogram(word: str) -> bool:
    used_letters = set()
    for letter in word:
        if letter in used_letters:
            return False
        else:
            used_letters.add(letter)
    return True

is_isogram("Dermatoglyphics")  # True
is_isogram("DDermatoglyphics")  # False

CodePudding user response：

Your code works exactly as it is expected it assigns 1 to every letter then since your if condition is out of the loop, it increments the last character (letter) by one.

I made some changes to your code.

def is_isogram(string):
    dict = {}
    for letter in string:
        dict[letter] = 0
    for letter in string:
        dict[letter]  = 1
    return dict


print(is_isogram("telegram"))

What I have done is that first it adds all the letters to the dictionary then uses another scan to count each letter.

This function has a complexity of O(n) which is faster than other answers I think

Here is a timed execution of both
This answer: https://onlinegdb.com/lMC-Qn76D
Other answers: https://onlinegdb.com/eeV0IFN5J

Please correct me if I am wrong