HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonModerate

Find first unique character in string

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
uniquecharacterfirstfindstring

Problem

I intentionally avoided Python tools which would make this even more trivial. I would like to be reviewed on efficiency, style, and obviously if there is a bug I would like to know.

def unique_char(s):
  ''' Find first non-repeated char in a string '''
  myDict = {}
  first = len(s)
  for c in s:
    if c in myDict.keys():
      myDict[c] += 1
    else:
      myDict[c] = 1
  if 1 in myDict.values():
    for k,v in myDict.items():
      if v == 1:
        if s.index(k) < first:
          first = s.index(k)
    return s[first]
  return(False)

Solution

-
The docstring is not quite right: "Find first non-repeated char in a string" — and then what? It should say explicitly that it returns the character.

-
The function returns False if all characters are repeated. This is a bad idea: it would be easy for the caller to forget to check. It's better to raise an exception in exceptional cases.

-
myDict is an uninformative choice of variable name. This dictionary contains counts of characters, so it should have name like counts or character_counts.

-
c in myDict.keys() can be simplified to c in myDict.

-
Building a dictionary of counts can more easily done using the built-in collections.Counter. It's not clear why you would want to avoid using this. Trivial functions should be trivial.

-
There's no point in testing if 1 in myDict.values(). Dictionaries have efficient lookup by key, but not by value, so the in operator here has to look through all the values. Since you're going to look through all the values anyway, this doesn't save you anything.

-
The runtime is \$Θ(n^2)\$ because s.index(k) has to search through the string for the character k. But there is a \$Θ(n)\$ algorithm:

from collections import Counter

def unique_char(s):
    """Return the first non-repeated character in the string s, or raise
    ValueError if all characters are repeated.

    """
    counts = Counter(s)
    for c in s:
        if counts[c] == 1:
            return c
    raise ValueError("all characters are repeated")

Code Snippets

from collections import Counter

def unique_char(s):
    """Return the first non-repeated character in the string s, or raise
    ValueError if all characters are repeated.

    """
    counts = Counter(s)
    for c in s:
        if counts[c] == 1:
            return c
    raise ValueError("all characters are repeated")

Context

StackExchange Code Review Q#109485, answer score: 10

Revisions (0)

No revisions yet.