Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm new in Python and consequently I am searching for a specific solution for code development. I already asked some pythonic question today and want to present my whole problem. I have found some solution, but I really have interest to develop easier and clearer code in Python, so all improvements are welcome.

For example, in some school, the students are ordered to some of 3 different classes. Now I have some set of graduated students and want to know, how many graduated students are in some class.

I create next code:

physic = ['id_1', 'id_2', 'id_4']
chemistry = ['id_5', 'id_7']
math = ['id_3', 'id_8', 'id_9']

graduatedPhysic = []
graduatedChemistry = []
graduatedMath = []

classes = [physic, chemistry, math]
graduated = [graduatedPhysic, graduatedChemistry, graduatedMath]

Every class has the different number of students (given by students IDs) and gratuated Classes are at the first empty. I combine it also to list of lists.

Then I use my code:

graduatedlist = ['id_2', 'id_5', 'id_7']

for stud in graduatedlist:
    for pos1 in range(len(graduated)):
        for pos2 in range(len(classes[pos1])):
            if stud.find(classes[pos1][pos2]) != -1:
                graduated[pos1].append((stud))

This code works, but does not look great... I would be very grateful for comments! For example, I tried to implement the idea with itertools.product():

for item_1, item_2 in itertools.product(range(len(graduated)), range(len(classes[item_1])):

I have a problem with a declaration of item_1.

I have also a feeling that the lists isn't the best solution for implementation, but for DataFrame I need the column of similar length and every class can have the different number of the students.

For the special interest are two points:

  1. How I really can improve my for-loop?
  2. What I can use instead of list declaration?
share|improve this question
    
Hi, welcome to CodeReview.SE! Can you please provide your actual code and what it is supposed to do ? (At the moment, I have doubts that your code is working at all : there is a typo in gratuated and the variable item is not defined anywhere). –  Josay Jul 28 at 14:46
    
sorry, item is stud, I have to replace it... –  Guforu Jul 28 at 14:52

2 Answers 2

up vote 6 down vote accepted

Re-organising the logic

As a rule of thumb, there is almost no need for range(len(x)) in Python. Thus :

for stud in graduatedlist:
    for pos1 in range(len(graduated)):
        for pos2 in range(len(classes[pos1])):
            if stud.find(classes[pos1][pos2]) != -1:
                graduated[pos1].append((stud))

definitly looks wrong and can easily be improved :

for stud in graduatedlist:
    for pos1, grad_list in enumerate(graduated):
        for stud2 in classes[pos1]:
            if stud.find(stud2) != -1:
                grad_list.append(stud)

Now, instead of iterating over graduatedlist, it might make more sense to iterate over the classes we are considering and then, for each class, consider the student and for each student, check if he has graduated.

for pos1, grad_list in enumerate(graduated):
    for stud in classes[pos1]:
        if stud in graduatedlist:
            grad_list.append(stud)

Even better, you don't need to prepopulate graduated = [graduatedPhysic, graduatedChemistry, graduatedMath] : it makes things akward as you have to deal with indices. You could just add elements to the list as you go :

graduated = []

for c in classes:
    graduated_class = []
    for stud in c:
        if stud in graduatedlist:
            graduated_class.append(stud)
    graduated.append(graduated_class)

Making things more beautiful

Now, in Python, this looks typically like something can be written in a more concise way using list comprehension :

graduated = []

for c in classes:
    graduated_class = [stud for stud in c if stud in graduatedlist]
    graduated.append(graduated_class)

Or in a more concise way :

graduated = []
for c in classes:
    graduated.append([stud for stud in c if stud in graduatedlist])

And here we have the same pattern again :

graduated = [[stud for stud in c if stud in graduatedlist] for c in classes]

Using the right tool/data structure

Now, things can probably be done in a much smarter way using the right data structure. You want to quickly change if some element belongs or not to something. sets is what you should be using and you want to compute intersections.

physic = {'id_1', 'id_2', 'id_4'}
chemistry = {'id_5', 'id_7'}
math = {'id_3', 'id_8', 'id_9'}
graduatedlist = {'id_2', 'id_5', 'id_7'}

classes = [physic, chemistry, math]
graduated = [c.intersection(graduatedlist) for c in classes]

Or more succintly :

graduated = [c & graduatedlist for c in classes]
share|improve this answer
    
Thank you very much, this is just great! –  Guforu Jul 28 at 15:24
    
Nice. I like this one better than my alternative. Thanks for sharing. –  dss539 Jul 28 at 15:34

I would strongly recommend creating a class.

class Student:
    def __init__(self, id, graduated):
        self.id = id
        self.graduated = graduated

Then your problem is easier.

graduatedchemistry = [student for student in chemistry if student.graduated]

To cleanly get graduates from all classes then you can make a function

def getgraduates(students):
    return [student for student in students if student.graduated]

graduatedchemistry = getgraduates(chemistry)
graduatedmath = getgraduates(math)
share|improve this answer
    
ok, thank you. Will try it. –  Guforu Jul 28 at 13:30
    
Respect PEP-8, fields should be lowercase. So, student.graduated, not student.Graduated. –  Stefano Sanfilippo Jul 28 at 15:03
    
@StefanoSanfilippo thank you, I edited it to be PEP8 conformant. I always forget that specific guideline because I personally find it extremely stupid. If you have a link to some convincing argument about why lowercase fields are preferable, I'd love to see it. I want to believe. :) –  dss539 Jul 28 at 15:11
1  
It's just a convention: classes are CapitalizedCamelCase, modules, variables and functions are lower_snake_case, unless they are module level variables, which means UPPER_SNAKE_CASE. There is no specific reason it was choosen and it's not the best, simply is what most folks around have agreed to use. By adhering to it, you make it simpler for others to identify the elements of your code. –  Stefano Sanfilippo Jul 28 at 15:16
2  
@dss539 - I think it's actually somewhat redundant to do that, since in Python, you always prefix fields and methods with self anyways. As a result, you can always immediately identify whether or not something is a field or method regardless of what naming scheme you pick. In any case, style guidelines are always going to be somewhat arbitrary -- the real benefit of only occurs when everybody actually uses them. IMO the specific guidelines don't matter as much, but the consistency does. –  Michael0x2a Jul 28 at 17:25

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.