4 Replies - 305 Views - Last Post: 12 March 2019 - 07:05 PM Rate Topic: -----

Poll: optional poll title (1 member(s) have cast votes)

do you like to Critique code?

  1. yes!!!!!!!!!!!!! (0 votes [0.00%])

    Percentage of vote: 0.00%

  2. NOOOOOOOOOOOO -_- (1 votes [100.00%] - View)

    Percentage of vote: 100.00%

Vote Guests cannot vote

#1 Bill Bates   User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 5
  • Joined: 10-March 19

please critique this code on merge sort

Posted 12 March 2019 - 12:54 PM

my intentions are to make this code short if possible. any suggestion on the same or anything i could have done better with inbuilt functions is appreciated. Thank you for your time
def merge(arr):
    # base case
    length = len(arr)
    half = length//2
    if length == 1 or length == 0:
        return arr
    # recursive case
    firstHalf = merge(arr[:half])  # sort first half
    secondHalf = merge(arr[half:])  # sort second half
    length_firstHalf = len(firstHalf)
    length_secondHalf = len(secondHalf)
    sortedList = []

    i, j = 0, 0  # variables for iteration
    while i != length_firstHalf and j != length_secondHalf:  # add elements into the new array until either of then two sub arrays is completely traversed
        if firstHalf[i] > secondHalf[j]:
            sortedList.append(secondHalf[j])
            j += 1
            continue
        if firstHalf[i] < secondHalf[j]:
            sortedList.append(firstHalf[i])
            i += 1
    return(sortedList + firstHalf[i:] + secondHalf[j:])  # return the array after adding whats left of the sub array which wasnt completely traversed 
print(merge([20,19,18,17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,1]))



Is This A Good Question/Topic? 0
  • +

Replies To: please critique this code on merge sort

#2 jon.kiparsky   User is online

  • Beginner
  • member icon


Reputation: 11423
  • View blog
  • Posts: 19,473
  • Joined: 19-March 11

Re: please critique this code on merge sort

Posted 12 March 2019 - 01:56 PM

"as short as possible" is not a goal that I love. "As clean as possible" is what I tend to go for. That means doing things DRY and using good names. Also using functions to name discrete actions. Also, using constructions that are idiomatic to your language of choice and obeying language-local style conventions. With those things in mind, here's a few general suggestions.

- this function is 24 lines. 10 is the usual acceptable max - anything that you can't do in ten lines is almost certainly more than one discrete action
- in python, prefer snake_case to dromedaryCase
- in python, prefer iterating over items (for item in lst: rather than for i in len(lst)).
- make your names more meaningful. This function is called "merge" but it's actually the full mergesort, not just the merge step.
Was This Post Helpful? 2
  • +
  • -

#3 Bill Bates   User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 5
  • Joined: 10-March 19

Re: please critique this code on merge sort

Posted 12 March 2019 - 02:07 PM

jon.kiparsky
thank you for you reply. i got the point about dividing a huge function into smaller one if it exceeds 10 lines. for the above particular code where can i break it? which part can be made into another function?
Was This Post Helpful? 0
  • +
  • -

#4 DK3250   User is offline

  • Pythonian
  • member icon

Reputation: 507
  • View blog
  • Posts: 1,597
  • Joined: 27-December 13

Re: please critique this code on merge sort

Posted 12 March 2019 - 02:12 PM

The forum is intended for code problems, your question here is borderline.
Nevertheless, I have only few comments:
1. line 15: use '<' in stead of '!=', this is really a tad more precise.
2. the two variables in line 10, 11 can be avoided, use len() directly in line 15
3. I really don't like the two if-statements in line 16/20. Use of if/elif/else will avoid the 'continue' statement and potentially catch any double values.

My version of your code:
def merge(arr):
    # base case
    length = len(arr)
    half = length//2
    if length == 1 or length == 0:
        return arr
    # recursive case
    firstHalf = merge(arr[:half])  # sort first half
    secondHalf = merge(arr[half:])  # sort second half
    sortedList = []

    i, j = 0, 0  # variables for iteration
    while i < len(firstHalf) and j < len(secondHalf):  # add elements into the new array until either of then two sub arrays is completely traversed
        if firstHalf[i] > secondHalf[j]:
            sortedList.append(secondHalf[j])
            j += 1
        elif firstHalf[i] < secondHalf[j]:
            sortedList.append(firstHalf[i])
            i += 1
        else:
            print("Two elements of same value..!")  # you can add more info to this print statement...
            break
    return(sortedList + firstHalf[i:] + secondHalf[j:])  # return the array after adding whats left of the sub array which wasnt completely traversed 

print(merge([20,19,18,17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,1]))


Was This Post Helpful? 1
  • +
  • -

#5 jon.kiparsky   User is online

  • Beginner
  • member icon


Reputation: 11423
  • View blog
  • Posts: 19,473
  • Joined: 19-March 11

Re: please critique this code on merge sort

Posted 12 March 2019 - 07:05 PM

View PostBill Bates, on 12 March 2019 - 04:07 PM, said:

for the above particular code where can i break it? which part can be made into another function?



The first place I usually look for a function to extract is any place where I make a scope, like a loop or a conditional. If you think about it, that is by definition a spot where we're isolating an action or perhaps a set of actions. "IF so-and-so, THEN do_this_thing", "WHILE condition, REPEAT some_action". Typically we spell out the steps, but we can always define a function for the purpose, and it would usually make the code clearer. Less obviously, you can also generally extract the condition of a while loop or an if-statement, and this can produce a surprising benefit in clarity, particularly when the conditional is a complex one.
So that gives you one easy spot to extract two functions, at your while loop. The trick is to come up with good names.

From there, you might also go the extra step of wrapping the block including the while loop and the final clean-up step in another function. This will seem excessive, most likely, but that is clearly a step that is a unitary action. You can tell me exactly what it does in a few words, so you can give it a clear name.

Looking at the first half, I don't see a clear place to extract, which probably means the code needs to be moved around a little until it tells a clear story. Or you could step away from the code and try to tell yourself the clear story that you wish you were seeing here. What exactly does this function do? Write out the logic in something like English, without looking at the code, and then go back and try to make the code tell that story.
Was This Post Helpful? 3
  • +
  • -

Page 1 of 1