10
\$\begingroup\$

I wrote this blog post, but received some feedback that there are better ways to handle the situation. I'd like to get some more opinions. Here is the topic:

Given a list of letters, I want to find the most commonly seen one.

# create the list of letters
my_list = ['b', 'b', 'b', 'c', 'a', 'b']

# simple sentence to make all more "user friendly"
sentence = '\nLetter which repeat the most is: '

# 3 lists declared globally which will store our values
a_list = []
b_list = []
c_list = []

# for loop which control the letters in the list and append them to the respective list
for v in my_list:
    if v == 'a':
        a_list.append(v)

    elif v == 'b':
        b_list.append(v)

    elif v == 'c':
        c_list.append(v)

# create a variable which store the name of the letter and how many times appear in the main list
A = a_list[0], len(a_list)
B = b_list[0], len(b_list)
C = c_list[0], len(c_list)

# variable which store length values
total_list = A[1], B[1], C[1]

# find the max value
largest = max(total_list)

# in this loop we are going to compare each value from total_list to the largest one
for i, val in enumerate(total_list):
    if total_list[i] == largest:
        x = total_list[i]  # assign to "x" this value

        # do a check for all the values
        # if the condition is satisfied, then print the sentence and letter (to uppercase)
        if x == A[1]:
            print(sentence + str(A[0]).upper())
        elif x == B[1]:
            print(sentence + str(B[0]).upper())
        elif x == C[1]:
            print(sentence + str(C[0]).upper())

I've heard that collections.Counter could be used for this, but I'm not sure that the output will be the same. I might be able to use it to replace some of my code above, but will I end up with the same result?

\$\endgroup\$
13
\$\begingroup\$

There are a few problems with this; mainly that you only support the letters a, b, and c. It isn't necessarily a problem if those are the only letters you'll ever get, but what is a problem is that you're making identical variables, with identical behavior, for each letter. Then for every letter you want to add in the future, you have to repeat yourself. Anytime you see yourself with a pattern like this, it is screaming for you to use a dictionary, or situationally a list of lists.

Making that change, we'd end up with something like this

counter = {}
for letter in my_list:
    if letter not in counter:
        counter[letter] = 0
    counter[letter] += 1

largest = 0
best_letters = []
for letter, count in counter.items():
    if count > largest:
        largest = count
        best_letter = [letter]
    elif count == largest:
        best_letter.append(letter)

print()
for letter in best_letters:
    print("Letter which repeats the most is: {}".format(letter.upper()))

Now we don't need the separate logic for each possible letter; we also don't need that hacky loop where you find the largest and then loop through.

We can make this better though, as you implied with collections.Counter. A Counter already does a lot of this work. For example, to get the count we just do

counter = Counter(my_list)

Then to get the best one, we just do

best_letter, largest = counter.most_common(1)[0]

This makes your total program look like this

best_letter, _ = Counter(my_list).most_common(1)[0]
print()
print("Letter which repeats the most is: {}".format(best_letter.upper()))

In general, it is always good to write functions to encapsulate behavior, but in this case you can probably just inline the use of the Counter.

Now if we want to get all of the letters that are most frequent, we can change it pretty easily. We'll do this instead:

most_common = counter.most_common(len(counter)) 
largest = most_common[0][1]

for letter, count in most_common:
    if count < largest:
        break

    print("Letter which repeats the most is: {}".format(letter.upper())

That'll grab all of values in the order of most to least frequent, and then you loop through all of the ones that are most commonly seen. Note that there is no implicit sorting within the ones that share a frequency; if you want further constraints on that it'll take additional logic.

\$\endgroup\$
  • \$\begingroup\$ Thank you. In a list of 6 chars, if I put 2 identical char (a,a,b,b,c,c), the program you posted will return the firs char which repeat the most ? Just tested it and return A using the above example.. \$\endgroup\$ – Link Feb 9 '18 at 19:04
  • 1
    \$\begingroup\$ @Link please don't accept this answer yet; wait for other people to answer as well. Give it at least a day. \$\endgroup\$ – Dannnno Feb 9 '18 at 19:05
  • 1
    \$\begingroup\$ @Link I'm not sure I follow; do you want it to print the one that occurred first in the case of a tie? Or do you want it to print all of the ones with the most? Both are pretty straightforward \$\endgroup\$ – Dannnno Feb 9 '18 at 19:07
  • \$\begingroup\$ I was just asking. It's the second you said, but I was curious about the output. If you give the same ammount of (different) letters, it will print out the first one. \$\endgroup\$ – Link Feb 9 '18 at 19:08
  • 1
    \$\begingroup\$ @Link See my update \$\endgroup\$ – Dannnno Feb 9 '18 at 19:12
0
\$\begingroup\$

I would use buildin max function for this:

my_string = 'abcaaacccbbaaabaccaa'
count_these = 'abc'
most_popular = max(count_these, key=my_string.count)

btw. Learned this on checkio.org :)

\$\endgroup\$
  • \$\begingroup\$ It's concise but also inefficient compared to Counter. \$\endgroup\$ – Eric Duminil Feb 11 '18 at 22:27

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.