4
\$\begingroup\$

I completed the "Nested Lists" challenge on HackerRank, and would love any feedback on my code.

My program is fed text with the number of students in a classroom, the name of a student, and their grade. For example:

4
Shadab
8
Varun
8.9
Sarvesh
9.5
Harsh
10

The code should return the student with the second lowest grade. Here's the output using the above input:

Varun

Here is my code:

def secondlow(students):
    grades = []
    for student in students: 
        grades.append(student[1])
    sort_grades = sorted(grades)
    seclow_grade = sort_grades[0]
    for grade in sort_grades:
        if grade != seclow_grade:
            seclow_grade = grade
            break
    seclow_stud = []
    for student in students: 
        if student[1] == seclow_grade:
            seclow_stud.append(student[0])
    for name in sorted(seclow_stud): 
        print(name)



students = []
for pupil in range(int(input())):
    new_stud = [input(), float(input())]
    students.append(new_stud)
secondlow(students)
\$\endgroup\$
5
\$\begingroup\$

The code mostly follows usual coding style and reads good. I would just change secondlow to second_low and avoid using abreviations in variable names.

Now for the improvements:

Print vs return

Do not code too much behaviour into your functions. Let them compute their stuff and return the result so the caller can do whatever they want with it. It doesn't add much here, but it is a good habit to have.

Getting the value you are looking for

seclow_stud = []
for student in students: 
    if student[1] == seclow_grade:
        seclow_stud.append(student[0])

You do not need to store the only value you are looking for into a list. Especialy when using a for loop after that to extract the element out of the list:

for student in students: 
    if student[1] == seclow_grade:
        return student[0]

Sorted

sort_grades = sorted(grades)
seclow_grade = sort_grades[0]
for grade in sort_grades:
    if grade != seclow_grade:
        seclow_grade = grade
        break

The sorted builtin returns a list of values in increasing order. What is important to note here is that sort_grades is a list. So sort_grade[0] is the lowest grade and sort_grade[1] is the second lowest. No need to search for the value yourself, sorted already did the work for you.

seclow_grade = sorted(grades)[1]

List-comprehensions

grades = []
for student in students: 
    grades.append(student[1])

Python have these construct to easily build lists out of other iterables:

grades = [student[1] for student in students]

You can also use the same construct when building lists out of the input. Overall code would look like:

def secondlow(students):
    grades = [student[1] for student in students]
    seclow_grade = sorted(grades)[1]
    for student in students: 
        if student[1] == seclow_grade:
            return student[0]


students = [[input(), float(input())] for _ in range(int(input()))]
print(secondlow(students))

Dictionaries

I know that the challenge explicitly mentionned lists of lists, but Python have dictionaries that can map a value to another. You can use them as a replacement:

def secondlow(students):
    seclow_grade = sorted(students.values())[1]
    for student in students.items():
        if student[1] == seclow_grade:
            return student[0]


students = {}
for _ in range(int(input())):
    name = input()
    grade = float(input())
    students[name] = value
print(secondlow(students))

Or you can take advantage of the way you can retrieve items associated to others by switching the way you store your data:

def secondlow(students):
    second_low = sorted(students)[1]
    return students[second_low]


students = {}
for _ in range(int(input())):
    name = input()
    grade = float(input())
    students[value] = name  # Note the switch here
print(secondlow(students))

Edit

After reading the challenge description again, I realised that there can be multiple students with the second lowest grade. And in fact, there might be cases where there are multiple student with the lowest grade. Making my point about having the second lowest grade as sorted(grades)[1] irrelevant… Unless you remove duplicates using a set:

def second_low(students):
    second_low_grade = sorted(set(students.values()))[1]
    for student in students.items():
        if student[1] == second_low_grade:
            yield student[0]


students = {}
for _ in range(int(input())):
    name = input()
    grade = float(input())
    students[name] = value

for second_lowest_student in second_low(students):
    print(second_lowest_student)

Here I used the yield keyword to avoid building a list in second_low. What it does is allowing the function to return several results: one each time there is a match (if student[1] == second_low_grade). Here is the version using list of lists:

def second_low(students):
    grades = set(student[1] for student in students)
    seclow_grade = sorted(grades)[1]
    for student in students: 
        if student[1] == seclow_grade:
            yield student[0]


students = [[input(), float(input())] for _ in range(int(input()))]
for second_lowest_student in second_low(students):
    print(second_lowest_student)

Unfortunately, such requirements prevent you from using dictionaries the other way around.

\$\endgroup\$
4
\$\begingroup\$

You create too many new lists without using list comprehensions, and you iterate over your loops too many times. You also don't validate your input.

Validation

Users are bad/silly/annoying people. They have these nasty habits of giving you things you didn't ask for or didn't want, that neatly fit into the exact edge cases that break everything. Thus we practice defensive programming. Your first problem is that you do literally no validation whatsoever - your app will just crash and burn if fed malicious or invalid input. I'd do something like this.

def validate_input(validator, message="Invalid input - please try again", *, retry=True, show_exception=False):
    while True:
        try:
            return validator(input())
        except Exception as e:
            if show_exception:
                print(e)
            print(message)

        if not retry:
            raise Exception("Input was invalid.")

Essentially, you have some validator that will attempt to transform the input into the desired shape (i.e. int, float, class, whatever) and raise an error otherwise. You can control how much error is displayed to the user, if it should retry, etc.

Too many lists/iterations

You create three lists when you need at most. You loop over your list 4 times (not including sorting, and you don't take advantage of your sorting either), when you only need to do it once. You should process things as you get them instead. First, write a generator that gets you all of your students

def get_students(num_pupils):
    for pupil in range(num_pupils):
        yield validate_input(str), validate_input(float)

then write your function to get the second lowest

def second_lowest(pupils):
    worst_students = []
    second_worst_students = []

    worst_student_score = None
    second_worst_student_score = None

    for name, score in pupils:   
        if worst_student_score is None:
            worst_students.append(name)     
            worst_student_score = score
        elif second_worst_student_score is None:
            second_worst_students.append(name)
            second_worst_students_score = score
        else:
            if score < worst_student_score:
                second_worst_student_score = worst_student_score
                second_worst_students = worst_students
                worst_students = [name]
                worst_student_score = score
            elif score == worst_student_score:
                worst_students.append(name)
            elif score < second_worst_student_score:
                second_worst_student_score = score
                second_worst_students = [name]
            elif score == second_worst_student_score:
                second_worst_students.append(name)

    return second_worst_students, second_worst_Student_score

If you know what the maximum possible value will be then you can condense this code a bit, and if you're a little clever I think you'll be able to condense it even further, but I'll leave that as an exercise for the reader.

After that, all you need is this:

if __name__ == '__main__':
    print second_lowest(validate_input(int))

As an aside, your description of the requirements (return) and your implementation don't match (you just print). You also didn't mention anything about sorting, but you print the sorted values. You also didn't mention what to do if there were ties, or how to handle floating point irregularities, so I left that alone.

\$\endgroup\$
3
\$\begingroup\$

Your naming scheme is clear and I like that you have used a function to find the second lowest grade. A small suggestion is to use if __name__ == "__main__": module in your code. It makes it easier to reuse for later and is a good standard.

Coding style

If you want to read about best practices for handling errors and standard ways to approach such problems see the answer to Dannnno

The biggest problem with your code is the logic you use. You first make a list of students great! But then you split them into grades and students, why? The idea here is surely to use dictionaries, and this makes the problem easy to solve. See Mathias Ettinger answer for an excellent solution using pythons dictionaries. IF you want to know a bit more about dictionaries see here.

A different algorithm

Imagine the input is 10 million names, storing the variables and sorting is very expensive. Eg it takes a long time. Luckily there is a faster approach! Since the names are inputted one by one, we need to keep track of the lowest and second lowest names.

students_lowest = []
lowest_grade = float("inf")

students_second_lowest = []
second_lowest_grade = float("inf")

for pupil in range(int(input())):
    name, grade = input(), float(input())
    if grade < lowest_grade:
        students_second_lowest = list(students_lowest)
        students_lowest = [name]
        second_lowest_grade = lowest_grade
        lowest_grade = grade
    elif grade == lowest_grade:
        students_lowest.append(name)
    elif grade < second_lowest_grade:
        students_second_lowest = [name]
        second_lowest_grade = grade
    elif grade == second_lowest_grade:
        students_second_lowest.append(name)

for name in sorted(students_second_lowest):
    print(name)

This keeps a list of the names with the lowest and second lowest scores. At the very end before printing it sorts the list of students with the second lowest grade. This is a much smaller list. If you have any questions regarding the code above feel free to ask.

\$\endgroup\$

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.