Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am currently going through Codecademy's Python course (I don't come from a coding background but I'm thoroughly enjoying it) and I got to the end of one of the sections and thought, "Well it worked, but that seemed wildly inefficient." I tried shortening unnecessary parts and came up with this:

lloyd = {
    "name": "Lloyd",
    "homework": [90.0, 97.0, 75.0, 92.0],
    "quizzes": [88.0, 40.0, 94.0],
    "tests": [75.0, 90.0]
}
alice = {
    "name": "Alice",
    "homework": [100.0, 92.0, 98.0, 100.0],
    "quizzes": [82.0, 83.0, 91.0],
    "tests": [89.0, 97.0]
}
tyler = {
    "name": "Tyler",
    "homework": [0.0, 87.0, 75.0, 22.0],
    "quizzes": [0.0, 75.0, 78.0],
    "tests": [100.0, 100.0]
}

classlist = [lloyd, alice, tyler]

# Add your function below!

def get_average(student):
    average_homework = sum(student["homework"]) / len(student["homework"])
    average_quizzes = sum(student["quizzes"]) / len(student["quizzes"])
    average_tests = sum(student["tests"]) / len(student["tests"])
    average_total = average_homework * 0.1 + average_quizzes * 0.3 + average_tests * 0.6
    return average_total

def get_letter_grade(score):
    if score >= 90:
        return "A"
    if score < 90 and score >= 80:
        return "B"
    if score < 80 and score >= 70:
        return "C"
    if score < 70 and score >= 60:
        return "D"
    else:
        return "F"

def get_class_average(classlist):
    a = []
    for student in classlist:
        a.append(get_average(student))
    return sum(a) / len(a)

get_class_average(classlist)

Now this bit works, but I was wondering if there are more places I could trim the code down without losing the functionality (I'm guessing there is in the get_average function but the things I tried came back as errors because of the "name" field in the dictionary)? Codecademy seems good for learning the fundamentals, but I wanted to get some insight into the "best" way to do things. Should I not worry about reworking functional code to be more efficient at first and just learn the fundamentals, or should I keep trying to make the "best" code as I learn?

share|improve this question

migrated from stackoverflow.com Jul 7 '13 at 14:46

This question came from our site for professional and enthusiast programmers.

1  
Can you add information about the problem you are trying to solve ? –  Josay Jul 7 '13 at 14:50
    
@Josay Actually he isn't trying to solve any problem. This is the part of the python track on codecademy.com. One introducing to dictionary in Python. –  Aseem Bansal Jul 7 '13 at 15:06

3 Answers 3

  • In the function get_letter_grade you were checking more conditions then necessary. Try to follow the logical difference between your function and my code. It is simple math which I used to optimize it. Also structuring it as an if-elif-...-else is better for readability.
  • In get_average and get_class_average I use a thing called list comprehension. A simple google search will tell you what it is and why it is better. Google it in python tutorials if you have problem.

Edited Functions

def average(values):
    return sum(values) / len(values)

def get_average(student):
    keys = ['homework', 'quizzes', 'tests']
    factors = [0.1, 0.3, 0.6]
    return sum([ average(student[key]) * factor for key, factor in zip(keys, factors)])

def get_letter_grade(score):
    if score >= 90:
        return "A"
    elif score >= 80:
        return "B"
    elif score >= 70:
        return "C"
    elif score >= 60:
        return "D"
    else:
        return "F"

def get_class_average(classlist):
    return average([get_average(student) for student in classlist])

get_class_average(classlist)
share|improve this answer
    
I would vote you up, but I don't have 15 reputation. This is exactly what I was meaning. It works to Codeacademy standards, but could easily be refined. Thanks for the list comprehension pointer as well! –  Raynman37 Jul 7 '13 at 15:21
    
@Raynman37 Edited the answer a bit more. Try to figure out how the return statement of get_average works. Yeah you cannot vote up but you can accept an answer. –  Aseem Bansal Jul 7 '13 at 15:34
3  
Actually you don't need a list-comprehension. A generator expression works too, and avoids the creation of the intermediate list. –  Bakuriu Jul 7 '13 at 15:34
    
@Bakuriu Yeah that would have been better. There are two reasons I didn't do that. FIrstly, the OP is a beginner and I don't think introducing generators to a beginner would be a good idea. Secondly, I am not much experienced in Python either so my explanation might not have been the best. If he is interested here is a link to a question on stackoverflow –  Aseem Bansal Jul 7 '13 at 15:39
1  
I would have used zip in get_average(): return sum([average(student[key]) * factor for key, factor in zip(keys, factors)]). –  fjarri Jul 8 '13 at 3:19

Defining an average function (based on this), you could have things slightly clearer :

def average_list(l)
    return sum(l) / float(len(l))

def average_student(student):
    average_homework = average_list(student["homework"])
    average_quizzes =  average_list(student["quizzes"])
    average_tests =    average_list(student["tests"])
    average_total = average_homework * 0.1 + average_quizzes * 0.3 + average_tests * 0.6
    return average_total

def class_average(students):
    return average_list([average_student(s) for s in students])
share|improve this answer
    
You are not using the description in the link that you provided. You just added a function call which can be used in the other function as well. –  Aseem Bansal Jul 7 '13 at 15:07
    
Thanks for spotting this :-) –  Josay Jul 7 '13 at 15:17
    
Actually I was talking about using reduce and lambda. Anyways you don't need to use float in the average_list function either. All the values are float already so sum would also be. Implicit conversion so explicit isn't needed. –  Aseem Bansal Jul 7 '13 at 15:52
1  
@Josay Are we supposed to use class in the class_average section because it's already a built in Python term (I don't know what you'd call it officially)? –  Raynman37 Jul 7 '13 at 17:49
    
Raynman37 : As per docs.python.org/release/2.5.2/ref/keywords.html this is a valid comment. It completely went out of my mind but I'm going to change this. –  Josay Jul 7 '13 at 18:54

In average_student, you're manually iterating over the keys in a dictionary, and you've hardcoded the weights of the various parts. What if you want to add a "presentation" component to the grade? You've got a lot of places to touch.

Consider exporting the hardcoded information into its own dictionary, and iterating over the keys in that dictionary:

weights = {
    "homework": 0.1,
    "quizzes": 0.3,
    "tests": 0.6
}

def get_average(student):
    return sum(average(student[section]) * weights[section] for section in weights)

def average(x):
    return sum(x)/len(x)

In terms of learning Python, I'd definitely agree with your instinct -- you should keep trying to make your code as clean as possible even while completing exercises. When you're learning fundamentals, it's that much more important to understand good structure.

share|improve this answer
    
Is this the get_average function? I tried but couldn't run it. –  Aseem Bansal Jul 7 '13 at 16:10
    
Oops! I had a syntax brainfart. Fixed it. –  llb Jul 7 '13 at 17:40
    
@llb Thanks for this input too! get_average was the section I didn't like when I got done with it either. Exactly like you said that whole section was basically hardcoded in there. I'll definitely be playing with this today as well! –  Raynman37 Jul 7 '13 at 17:45

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.