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

Unique letter counter for string in Python

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

Problem

I'm trying to create a program that creates a record of how many times letters appear in a given string (ignoring blank spaces) using recursion.

s = "Hello there this is the string"

new_s = ""
for i in s:
    if i != " ":
        new_s += i
new_s = new_s.lower()
com_dict = {}
def merge_count(s):
    global com_dict
    if len(s) == 1:
        if s[0] in com_dict:
            com_dict[s[0]] += 1
        else:
            com_dict[s[0]] = 1
    else:
        merge_count(s[:int(len(s)/2)])
        merge_count(s[int(len(s)/2):])

merge_count(new_s)
for i in com_dict:
    print i, com_dict[i]


Is the global variable necessary? Should I be using a different data structure instead of a dictionary? I will need to rank the results based on the most frequently appearing letters. Is using recursion in this scenerio generally a bad idea? Would iterating be a better choice?

Solution

Is the global variable necessary?

No. Global variables are almost never necessary, and rarely the best solution. Consider this:

def merge_count(s, com_dict):
    if len(s) == 1:
        if s[0] in com_dict:
            com_dict[s[0]] += 1
        else:
            com_dict[s[0]] = 1
    else:
        merge_count(s[:int(len(s)/2)], com_dict)
        merge_count(s[int(len(s)/2):], com_dict)

def main():
    s = "Hello there this is the string"

    new_s = ""
    for i in s:
        if i != " ":
            new_s += i
    new_s = new_s.lower()

    com_dict = {}
    merge_count(new_s, com_dict)
    for i in com_dict:
        print i, com_dict[i]

main()


In this, you pass a dictionary to merge into into to the function.

You do

new_s = ""
for i in s:
    if i != " ":
        new_s += i


Addition of strings in loops is always almost bad, because it may reallocate the string on every addition. Instead, one should do a join:

new_s = "".join(i for i in s if i != " ")


However, in this case you might as well just do

new_s = s.replace(" ", "")


Recursively splitting in merge_count is pointless. Just do

def merge_count(s, com_dict):
    for i in s:
        if s in com_dict:
            com_dict[s[0]] += 1
        else:
            com_dict[s[0]] = 1


which affords the better interface of

def merge_count(s):
    com_dict = {}

    for i in s:
        if s in com_dict:
            com_dict[i] += 1
        else:
            com_dict[i] = 1

    return com_dict


One can simplify things with defaultdict:

from collections import defaultdict

def merge_count(s):
    com_dict = defaultdict(int)

    for i in s:
        com_dict[i] += 1

    return com_dict


Or even further with Counter:

from collections import Counter

def merge_count(s):
    return Counter(s)


Instead of

for i in com_dict:
    print i, com_dict[i]


One should do

for i, v in com_dict.iteritems():
    print i, v


Then one fixes up the naming

from collections import Counter

def main():
    string = "Hello there this is the string"

    lower_chars = string.replace(" ", "").lower()
    char_counts = Counter(lower_chars)

    for char, count in char_counts.items():
        print(char, count)

main()

Code Snippets

def merge_count(s, com_dict):
    if len(s) == 1:
        if s[0] in com_dict:
            com_dict[s[0]] += 1
        else:
            com_dict[s[0]] = 1
    else:
        merge_count(s[:int(len(s)/2)], com_dict)
        merge_count(s[int(len(s)/2):], com_dict)

def main():
    s = "Hello there this is the string"

    new_s = ""
    for i in s:
        if i != " ":
            new_s += i
    new_s = new_s.lower()

    com_dict = {}
    merge_count(new_s, com_dict)
    for i in com_dict:
        print i, com_dict[i]

main()
new_s = ""
for i in s:
    if i != " ":
        new_s += i
new_s = "".join(i for i in s if i != " ")
new_s = s.replace(" ", "")
def merge_count(s, com_dict):
    for i in s:
        if s in com_dict:
            com_dict[s[0]] += 1
        else:
            com_dict[s[0]] = 1

Context

StackExchange Code Review Q#94612, answer score: 9

Revisions (0)

No revisions yet.