Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

Note that comparison doesn't have to return 1 or -1. For example returning 123 would have the same effect as returning 1. With this in mind, the firstGreaterCmp function could be somewhat simpler:

def firstGreaterCmp(a, b):
    difference = topicToDistance[a] - topicToDistance[b]
    if abs(difference) <= .001:
        if a.id < b.id:
            return 1
    return int(difference)

On the other hand, the comparison function must return an int, and since your value is a float, I had to cast it. I don't really know if that has a non-negligible cost or not.

UPDATE

Also, as @janne-karila@janne-karila pointed out in a comment, int(difference) is 0 for a range of values (due to integer truncation), while your original code never returns 0. It depends on your use case whether this is acceptable or not.


In the Topic class, I find it somewhat strange that the id parameter comes last in the parameter list. Normally I would expect an id field to be the first.


Your code doesn't follow PEP8. It would be better this way:

def firstGreaterCmp(a, b):
    if abs(topicToDistance[a] - topicToDistance[b]) <= .001:
        if a.id < b.id:
            return 1
    if topicToDistance[a] > topicToDistance[b]:
        return 1
    return -1

# sorting by key may be faster than using a cmp function
topics.sort(cmp=firstGreaterCmp)

Note that comparison doesn't have to return 1 or -1. For example returning 123 would have the same effect as returning 1. With this in mind, the firstGreaterCmp function could be somewhat simpler:

def firstGreaterCmp(a, b):
    difference = topicToDistance[a] - topicToDistance[b]
    if abs(difference) <= .001:
        if a.id < b.id:
            return 1
    return int(difference)

On the other hand, the comparison function must return an int, and since your value is a float, I had to cast it. I don't really know if that has a non-negligible cost or not.

UPDATE

Also, as @janne-karila pointed out in a comment, int(difference) is 0 for a range of values (due to integer truncation), while your original code never returns 0. It depends on your use case whether this is acceptable or not.


In the Topic class, I find it somewhat strange that the id parameter comes last in the parameter list. Normally I would expect an id field to be the first.


Your code doesn't follow PEP8. It would be better this way:

def firstGreaterCmp(a, b):
    if abs(topicToDistance[a] - topicToDistance[b]) <= .001:
        if a.id < b.id:
            return 1
    if topicToDistance[a] > topicToDistance[b]:
        return 1
    return -1

# sorting by key may be faster than using a cmp function
topics.sort(cmp=firstGreaterCmp)

Note that comparison doesn't have to return 1 or -1. For example returning 123 would have the same effect as returning 1. With this in mind, the firstGreaterCmp function could be somewhat simpler:

def firstGreaterCmp(a, b):
    difference = topicToDistance[a] - topicToDistance[b]
    if abs(difference) <= .001:
        if a.id < b.id:
            return 1
    return int(difference)

On the other hand, the comparison function must return an int, and since your value is a float, I had to cast it. I don't really know if that has a non-negligible cost or not.

UPDATE

Also, as @janne-karila pointed out in a comment, int(difference) is 0 for a range of values (due to integer truncation), while your original code never returns 0. It depends on your use case whether this is acceptable or not.


In the Topic class, I find it somewhat strange that the id parameter comes last in the parameter list. Normally I would expect an id field to be the first.


Your code doesn't follow PEP8. It would be better this way:

def firstGreaterCmp(a, b):
    if abs(topicToDistance[a] - topicToDistance[b]) <= .001:
        if a.id < b.id:
            return 1
    if topicToDistance[a] > topicToDistance[b]:
        return 1
    return -1

# sorting by key may be faster than using a cmp function
topics.sort(cmp=firstGreaterCmp)
added 322 characters in body
Source Link
janos
  • 113.1k
  • 15
  • 154
  • 396

Note that comparison doesn't have to return 1 or -1. For example returning 123 would have the same effect as returning 1. With this in mind, the firstGreaterCmp function could be somewhat simpler:

def firstGreaterCmp(a, b):
    difference = topicToDistance[a] - topicToDistance[b]
    if abs(difference) <= .001:
        if a.id < b.id:
            return 1
    return int(difference)

On the other hand, the comparison function must return an int, and since your value is a float, I had to cast it. I don't really know if that has a non-negligible cost or not.

UPDATE

Also, as @janne-karila pointed out in a comment, int(difference) is 0 for a range of values (due to integer truncation), while your original code never returns 0. It depends on your use case whether this is acceptable or not.


In the Topic class, I find it somewhat strange that the id parameter comes last in the parameter list. Normally I would expect an id field to be the first.


Your code doesn't follow PEP8. It would be better this way:

def firstGreaterCmp(a, b):
    if abs(topicToDistance[a] - topicToDistance[b]) <= .001:
        if a.id < b.id:
            return 1
    if topicToDistance[a] > topicToDistance[b]:
        return 1
    return -1

# sorting by key may be faster than using a cmp function
topics.sort(cmp=firstGreaterCmp)

Note that comparison doesn't have to return 1 or -1. For example returning 123 would have the same effect as returning 1. With this in mind, the firstGreaterCmp function could be somewhat simpler:

def firstGreaterCmp(a, b):
    difference = topicToDistance[a] - topicToDistance[b]
    if abs(difference) <= .001:
        if a.id < b.id:
            return 1
    return int(difference)

On the other hand, the comparison function must return an int, and since your value is a float, I had to cast it. I don't really know if that has a non-negligible cost or not.


In the Topic class, I find it somewhat strange that the id parameter comes last in the parameter list. Normally I would expect an id field to be the first.


Your code doesn't follow PEP8. It would be better this way:

def firstGreaterCmp(a, b):
    if abs(topicToDistance[a] - topicToDistance[b]) <= .001:
        if a.id < b.id:
            return 1
    if topicToDistance[a] > topicToDistance[b]:
        return 1
    return -1

# sorting by key may be faster than using a cmp function
topics.sort(cmp=firstGreaterCmp)

Note that comparison doesn't have to return 1 or -1. For example returning 123 would have the same effect as returning 1. With this in mind, the firstGreaterCmp function could be somewhat simpler:

def firstGreaterCmp(a, b):
    difference = topicToDistance[a] - topicToDistance[b]
    if abs(difference) <= .001:
        if a.id < b.id:
            return 1
    return int(difference)

On the other hand, the comparison function must return an int, and since your value is a float, I had to cast it. I don't really know if that has a non-negligible cost or not.

UPDATE

Also, as @janne-karila pointed out in a comment, int(difference) is 0 for a range of values (due to integer truncation), while your original code never returns 0. It depends on your use case whether this is acceptable or not.


In the Topic class, I find it somewhat strange that the id parameter comes last in the parameter list. Normally I would expect an id field to be the first.


Your code doesn't follow PEP8. It would be better this way:

def firstGreaterCmp(a, b):
    if abs(topicToDistance[a] - topicToDistance[b]) <= .001:
        if a.id < b.id:
            return 1
    if topicToDistance[a] > topicToDistance[b]:
        return 1
    return -1

# sorting by key may be faster than using a cmp function
topics.sort(cmp=firstGreaterCmp)
Source Link
janos
  • 113.1k
  • 15
  • 154
  • 396

Note that comparison doesn't have to return 1 or -1. For example returning 123 would have the same effect as returning 1. With this in mind, the firstGreaterCmp function could be somewhat simpler:

def firstGreaterCmp(a, b):
    difference = topicToDistance[a] - topicToDistance[b]
    if abs(difference) <= .001:
        if a.id < b.id:
            return 1
    return int(difference)

On the other hand, the comparison function must return an int, and since your value is a float, I had to cast it. I don't really know if that has a non-negligible cost or not.


In the Topic class, I find it somewhat strange that the id parameter comes last in the parameter list. Normally I would expect an id field to be the first.


Your code doesn't follow PEP8. It would be better this way:

def firstGreaterCmp(a, b):
    if abs(topicToDistance[a] - topicToDistance[b]) <= .001:
        if a.id < b.id:
            return 1
    if topicToDistance[a] > topicToDistance[b]:
        return 1
    return -1

# sorting by key may be faster than using a cmp function
topics.sort(cmp=firstGreaterCmp)