3
\$\begingroup\$

I've done this activity selection piece of code, which is a greedy approach. I need some feedback/suggestions.


# Python 3
import random
import operator
begin = [random.randint(1, 10) for i in range(10)]
end = [x + random.randint(1, 4) for x in begin]
pair = sorted(list(zip(begin, end)), key=operator.itemgetter(1))
ls = list()
ls.append(pair[0])
for i in pair[1:]:
    if i[0] >= ls[-1][1]:
        ls.append(i)
print(ls, '>>>', len(ls), 'out of', len(pair), 'activities.')
\$\endgroup\$
1
\$\begingroup\$

Code readability and style -


I believe that good code should have good style and should be more readable and concise.


Let's introduce you to f-strings -

To create an f-string, prefix the string with the letterf ”. The string itself can be formatted in much the same way that you would with str.format(). f-strings provide a concise and convenient way to embed python expressions inside string literals for formatting.

Which means, instead of using the outdated way of formatting strings -

print(ls, '>>>', len(ls), 'out of', len(pair), 'activities.')

You could make it more concise -

print(f'{ls} >>> {len(ls)} out of {len(pair)} activities.')

NOTE - Requires Python 3.6 and above.

If you don't have Python 3.6 and above, you could use str.format() -

str.format() is one of the string formatting methods in Python 3, which allows multiple substitutions and value formatting. This method lets us concatenate elements within a string through positional formatting.

So you could write it like this -

print('{} >>> {} out of {} activities.'.format(ls, len(ls), len(pair)))

I would also put your code into a function -

A function is a block of organized, reusable code that is used to perform a single, related action. Functions provide better modularity for your application and a high degree of code reusing.

As you already know, Python gives you many built-in functions like print(), etc. but you can also create your own functions. These functions are called user-defined functions.

With this as the function name -

def activity_selection():

You should also consider using an if __name__ == '__main__': guard, like this -

if __name__ == '__main__':
    activity_selection()

Your variables should be more descriptive -

ls = list()

could be -

activity_list = list()  

So, overall, your code would then look like this (in terms of code readability and style) -

import random
import operator

def activity_selection():
    begin = [random.randint(1, 10) for i in range(10)]
    end = [x + random.randint(1, 4) for x in begin]
    pair = sorted(list(zip(begin, end)), key=operator.itemgetter(1))
    activity_list = list()
    activity_list.append(pair[0])
    for i in pair[1:]:
        if i[0] >= activity_list[-1][1]:
            activity_list.append(i)
    print(f'{activity_list} >>> {len(activity_list)} out of {len(pair)} activities.')
    # or
    # print('{} >>> {} out of {} activities.'.format(activity_list, len(activity_list), len(pair)))

if __name__ == '__main__':
    activity_selection()

To remain concise and immaculate in writing Python programs, I suggest you have a look at PEP 8, which is Python's official style guide.

Hope this helps!

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ sometimes I have a Déjà vu reading some of your answers :p \$\endgroup\$ – dfhwze Jun 20 '19 at 16:53
  • 1
    \$\begingroup\$ @dfhwze - They all need to use f-strings. No one uses them. So I just copy-paste stuff from my other answers (for ease of writing answers) :) That's fine right? \$\endgroup\$ – Justin Jun 20 '19 at 17:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy