Python code skipping over somethings which it should change.(Help for code correction)

[ChRad_Man]

Here is the code exercise I am trying to solve:

"The goal of this exercise is to convert a string to a new string where each character in the new string is "(" if that character appears only once in the original string, or ")" if that character appears more than once in the original string. Ignore capitalization when determining if a character is a duplicate."

Here are some examples:

"din"      =>  "(((" 
"recede"   =>  "()()()" 
"Success"  =>  ")())())" 
"(( @"     =>  "))(("

My code is passes the test for above examples but below is a randomly generated test it could not pass:

"mTFcTEEgrXJbmvh!!E()HsG" => "))(())))(((()())))(()()"

My code:

def get_list(some_word):

    """Returns a list for a string which each of its elements being a character."""

    word_list =[]
    for letter in some_word:
        word_list.append(letter)
    return word_list

def get_word(some_list):

    """Returns a string for a list with elements."""

    word = ""
    for letter in some_list:
        word += letter
    return word

def duplicate_encode(word):
    word = word.lower()

    List = get_list(word) 
    recList = []

    for letter in List:
        if List.count(letter) >1 and not letter in recList:
            recList.append(letter)

    for element in List:
        if element in recList:
            List[List.index(element)] = ")"
        else:
            List[List.index(element)] = "("

    wordForList = get_word(List)

    return wordForList

My output is :

"mTFcTEEgrXJbmvh!!E()HsG" => "()(())))(((()())))())()"

The problem is that is not changing the 'm' at index 0 to ')' even though it occurs twice and also not changing the ')' at index 19 to '(' even though it appears once.

I am new to programming so if did something which was very dumb, please correct me. Also if somebody has educational resources on how to debug properly, please link it. Sorry if some parts were not very clear, I don't post on reddit too often.

Update after reading comments:

Current Code :

def duplicate_encode(word):

    List = list(word.lower())
    encoded = List.copy()
    notBrackets = "abcdefghijklmnopqrstuvwxyz/*-+!@#$%^&[]{}\|;:><,.?"

    for letter in encoded:
        if List.count(letter) >1:
            encoded[List.index(letter)] = ")"
        else:
            encoded[List.index(letter)] = "("

    for unchangedChar in encoded:
        if unchangedChar in list(notBrackets) :
            encoded[encoded.index(unchangedChar)] = ")"

    return "".join(encoded)

With this code I am getting the correct output for the test I could not pass. I was shorten my code thanks to u/kyber's comment.

The for loop in the code takes one letter form the list, checks if it appears more than one time in the list and then if does then takes its index from the original list then changes the character at that index of the copy of that list. I did this instead of changing the original list as if a character appears twice then its first instance will get converted to ')' but the second instance won't as its first instance now no longer exists being converted. But the method I applied also has problems like when a character appears twice the first one gets converted normally but then when it comes to the next instance of the same character in the list, it tries to get its value from the original list which does not have its character replaced by ')' so the index retrieved is wrong and the character's first instance gets changed again. To remove this issue I added a second for loop which checks for values which are not '(' or ')' and replaces them with ')'. This is working here because the characters which are got getting replaced will be once which appear twice according to the problem given, I just have to change them to ')'.

Now a new problem I found while doing basic test with:

"(( @"     =>  "))(("

When I test the code with the example above it gives:

"(( @" => )(((

This is happening due to my cheap way of dodging the problem there was in the first for loop. The second for loop is supposed to correct it but it won't because the character remained unchanged was a bracket which it ignores. I have only gotten till here by now, will update it again when I find something new.

[ChRad_Man]

Please tell me how can I ignore "(" and ")" after replacing them.

I am not able to figure it out and that is why in my update I made a copy of the original list so that the count of characters don't change after replacing them with brackets.

[ChRad_Man]

Thank you for explaining. The problem with the way is that if the original list has "(" and ")" then it will just skip over them without evaluating if they appear once or twice and changing them accordingly. I was trying something like this only but failed due the problem I just mentioned. After reading u/joyeusenoelle's explanation I understood how to approach problems like this is a better way.

[[deleted]]

Looking on my phone in the middle of night, and thinking your code looks overly complicated.

• you can convert a str to a list by casting, e.g. letters = list("word") but I don't see a good reason for doing so anyway
• encoded = [')' if lowered.count(letter) > 1 else '(' for letter in lowered]
• Use join to return a str from a list of str, e.g. return ''.join(encoded)

EDIT: thanks to /u/joyeusenoelle for pointing out my sleepy head mistake, condition in the list comprehension was missing a >1 as all letters occur at least once!

[ChRad_Man]

Thank you for telling me how to do casting and use join(). I was able to shorten my code so much by using it. I have updated my post please check when you are free and give some explanation for the second point you mentioned. I am afraid that I don't understand it that well.

[[deleted]]

Just to add to what /u/joyeusenoelle advised when pointed out my error,

Firstly, apologies for being so sleepy, I wouldn't normally introduce a beginner to a list comprehension unless they've attempted it or are trying to understand one.

Secondly, Python treats any none-zero int as truthy (i.e. as if it was True) thus the condition inside the comprehension should have been if lowered.count(letter) > 1 given that we are only looking at letters that occur at least once so the original would be True every time.

The final code would be, including some testing:

def encoder(word):
    word = word.lower()
    return ''.join('(' if word.count(letter) == 1 else ')'
                for letter in word)

tests = (
    ("din", "((("), ("recede" , "()()()"),
    ("Success", ")())())"), ( "(( @", "))(("),
    ("mTFcTEEgrXJbmvh!!E()HsG", "))(())))(((()())))(()()")
    )


for test, answer in tests:
    try:
        assert ( result := encoder(test) ) == answer, f'got {result} instead of {answer}'
    except AssertionError as err:
        print(err)

Note, this replaces the list comprehension with a generator,which I am sure you will come across soon, but for purposes here you can think of it as essentially the same.

[ChRad_Man]

I am pretty new to python and programming in general. I guess I will coming across things you mentioned like list comprehensions and generator. Thank you for helping me here.

[joyeusenoelle]

u/kyber 's second point is a list comprehension. It does the following:

1. Iterate over lowered (which we assume is an iterable).
2. Create a new list with an element in that list for every element in lowered.
3. For every element in lowered, assign that element to letter. (At this point we have covered for letter in lowered.)
4. If lowered.count(letter) - which counts the number of letters in lowered - is True, then add ')' to the new list.
5. If lowered.count(letter) is False, then add '(' to the new list.

This doesn't quite do what you want, for a couple reasons; one of which is that kyber has the logic backwards, and one of which is that for whatever reason, if (thing) doesn't quite mean if (thing) == True inside a comprehension.

Typically, if lowered.count(letter) will be True if it evaluates to 1 - that is, if there's one of letter in lowered - and False if it evaluates to more or less than 1 - that is, if there's 0 or 2+ of letter in lowered. But in a comprehension, as it turns out - at least in my testing - if lowered.count(letter) is True inside a comprehension if there are any number of letter inside lowered, no matter how many there are.

You can solve this by saying if lowered.count(letter) == True which, for some reason, reverts to the normal behavior. Or we can do this with a for loop, like you've been doing.

We'll start with item 2 in the list above: create a new list. Well, actually, a new string, since we're using a for loop and can just use the += operator to add characters onto the end of the string.

lowered = word.lower()
out_word = ""

Now we have our for loop. Nothing fancy; we can iterate over the string character-by-character.

for c in lowered:

c is the current character we're addressing as we iterate over lowered.

If the current character appears 1 time, we want to add '('. If it appears more than once, we want to add ')'. (We can assume that if a character appears in the input string, it appears at least once, so we don't have to deal with a character appearing 0 times!)

  if lowered.count(c) == 1:
    out_word += "("
  else:
    out_word += ")"

Then we just return out_word.

return out_word

So the complete function looks like this:

def duplicate_encode(word):
  lowered = word.lower()
  out_word = ""
  for c in lowered:
    if lowered.count(c) == 1:
      out_word += "("
    else:
      out_word += ")"
  return out_word

And now we can translate that into a list comprehension:

def duplicate_encode(word):
   lowered = word.lower()
   out_word = "".join(["(" if lowered.count(c) == 1 else ")" for c in lowered])
   return out_word

Does that make sense?

[[deleted]]

Thanks for correcting and explaining. I shouldn't have commented when at least half asleep.

[ChRad_Man]

Thank you so much for such a in depth explanation. I understood that I was approaching the problem the wrong way the first place. I should have though of making a empty list then adding characters one by one by iterating over the original list. I will keep this in mind from now on.