Python Forum

Hi guys, the code is working, but I want know, what I can improve on my code. It is like implements the simulation of shipment clearance in the depot according to priority and available deliveries. How can I do it better? Can u show me? Thnx for your time.

[input]
24 K
13 H
25 R
1 M
-4 T0
15 G
4 C
-1 T1
-3 T2
12 Y
[/input]

import sys
chain = sys.stdin.read().splitlines()
array_z = list()
for line in chain:
    row=list(map(str, line.split()))
    array_z.append(row)
#every line in input changes into 2D array

def checking(chain):
    "checking if characters are numbers or not"
    for char in chain:
        if char not in "0123456789-":
            return False
    return True

class MaxHeap:
    def __init__(self):
        """heap __init__ constructor"""
        self.heap =[]
    def bubble_up(self, i):
        """"bubble the element up if condition is ok """
        while i > 0:
            j = (i - 1) // 2
            if self.heap[i] <= self.heap[j]:
                break
            self.heap[j], self.heap[i] = self.heap[i], self.heap[j]
            i = j
    def insert(self, k):
        """insert element in heap"""
        self.heap += [k]
        self.bubble_up(len(self.heap) - 1)
    def peek(self):
        """return the biggest element"""
        return self.heap[0]
    def size(self):
        """return quantity of elements in heap"""
        return len(self.heap)
    def is_empty(self):
        """is heap empty?"""
        return self.size() == 0
    def bubble_down(self, i):
        """bubble down the element"""
        n = self.size()
        while 2 * i + 1 < n:
            j = 2 * i + 1
            if j + 1 < n and self.heap[j] < self.heap[j + 1]:
                j += 1
            if self.heap[i] < self.heap[j]:
                self.heap[i], self.heap[j] = self.heap[j], self.heap[i]
            i = j
    def pop(self):
        """delete the biggest element and change the heap"""
        element = self.heap[0]
        self.heap[0] = self.heap[-1]
        self.heap.pop()
        self.bubble_down(0)
        return element

for i in range (len(array_z)):
    for j in range (len(array_z[i])):
        if checking(array_z[i][j]) is True:
            array_z[i][j]=int(array_z[i][j])

Heap=MaxHeap()
for a in range (len( array_z)):
    if  array_z[a][0]>0:
        Heap.insert( array_z[a])
    if  array_z[a][0] < 0:
        print( array_z[a][1]+": ",end="") #print name of delivery
        index_of_package= array_z[a][0]
        while index_of_package<0 and Heap.is_empty() is False:
            delivery_package=Heap.pop()
            print(delivery_package[1],end=" ") #print name of package in delivery
            index_of_package+= 1
        print("")
print("Depo: ",end="")
while Heap.is_empty() is False:
    depo_package=Heap.pop()
    print(depo_package[1],end=" ") #print name of package in depo

Output:
T0: R K H M
T1: G
T2: C
Depo: Y

First off, I wish you had explained better. When you say "heap enhancements" in the subject are you saying you have them and want them reviewed? Or you want us to enhance your heap? Does that mean you don't want comments on the non-heap parts?

Here are my thoughts, although I should say that they reflect my opinions and there's definitely some subjectivity.

First, I don't like mixing logic and function/class definitions. So the first thing I'm doing with your code is to move those definitions to the top, and introduce a main function to your code

def main():
    chain = sys.stdin.read().splitlines()
    array_z = list()
    for line in chain:
        row=list(map(str, line.split()))
        array_z.append(row)
 
    for i in range (len(array_z)):
        for j in range (len(array_z[i])):
            if checking(array_z[i][j]) is True:
                array_z[i][j]=int(array_z[i][j])
     
    Heap=MaxHeap()
    for a in range (len( array_z)):
        if  array_z[a][0]>0:
            Heap.insert( array_z[a])
        if  array_z[a][0] < 0:
            print( array_z[a][1]+": ",end="") #print name of delivery
            index_of_package= array_z[a][0]
            while index_of_package<0 and Heap.is_empty() is False:
                delivery_package=Heap.pop()
                print(delivery_package[1],end=" ") #print name of package in delivery
                index_of_package+= 1
            print("")
    print("Depo: ",end="")
    while Heap.is_empty() is False:
        depo_package=Heap.pop()
        print(depo_package[1],end=" ") #print name of package in depo

if __name__ == "__main__":
    main()

The next thing I'm examining is this:

    chain = sys.stdin.read().splitlines()
    array_z = list()
    for line in chain:
        row=list(map(str, line.split()))
        array_z.append(row)

"chain" is a very unintuitive name; why not just "lines"? "array_z" I would change as well, though I'm less sure what to. Stylistically, I would use [] instead of list(). The first line of the loop could be simplified to row = line.split(). I would simplify this whole block to

    lines = sys.stdin.read().splitlines()
    array_z = [line.split() for line in lines]

And here's the next block we're examining:

    for i in range (len(array_z)):
        for j in range (len(array_z[i])):
            if checking(array_z[i][j]) is True:
                array_z[i][j]=int(array_z[i][j])

The first line raises a red flag for me - iterating over range(len(iterable)) is usually an anti-pattern, you should usually just be iterating over the iterable contents directly. Looking more closely, I see why you were doing it. That said, you only need that style in the inner loop, not the outer, so I would get rid of using an index in the outer loops entirely, if you were keeping this. But actually I don't like the fact that we modify a list in-place, so I'd omit this block of code and update the prior block to this

    def as_int_if_possible(x):
        try:
            return int(x)
        except ValueError:
            return x

    lines = sys.stdin.read().splitlines()
    array_z = [[as_int_if_possible(e) for e in line.split()] for line in lines]

I leave as_int_if_possible within main, but you can move it to the top level if you prefer. It doesn't seem like something we intend to be part of our API if someone imports this, so I personally prefer it to not "leak out". With this, we can also remove the checking function entirely.

I'm not sure if this is kosher, but I'd actually simplify a bit more:

    def transform_line(line: str) -> Tuple[int, str]:
        index, identifier = line.split()
        return (int(index), identifier)

    lines = sys.stdin.read().splitlines()
    array_z = [transform_line(line) for line in lines]

Here's what we're going to examine next:

    Heap=MaxHeap()
    for a in range (len( array_z)):
        if  array_z[a][0]>0:
            Heap.insert( array_z[a])
        if  array_z[a][0] < 0:
            print( array_z[a][1]+": ",end="") #print name of delivery
            index_of_package= array_z[a][0]
            while index_of_package<0 and Heap.is_empty() is False:
                delivery_package=Heap.pop()
                print(delivery_package[1],end=" ") #print name of package in delivery
                index_of_package+= 1
            print("")

The first thing I notice is your naming. PEP 8 recommends variables be snake_case, but it looks like you're using CamelCase. CamelCase is typically used for classes, so someone used to PEP 8 may have more trouble reading your code because Heap looks like a class instead of an instance. I'll also take this time to point out that your whitespace is inconsistent as well; tools like Flake8 can help with this.

Next, I'm back to range(len()) not being good. It's trivial to change it to a regular for-each loop, but I'd go a step further and unpack your nested list in the for loop header, like so

    heap = MaxHeap()
    for index_of_package, identifier in array_z:
        if index_of_package > 0:
            heap.insert((index_of_package, identifier))
        elif index_of_package < 0:
            print(identifier + ": ", end="") #print name of delivery
            while index_of_package < 0 and not heap.is_empty():
                _, delivery_package = heap.pop()
                print(delivery_package, end=" ") #print name of package in delivery
                index_of_package += 1
            print("")

While I was at it, I replaced heap.is_empty() is False with not heap.is_empty(). The reason is that is specifically checks if it's the same object, when you don't care about that - you just care about the true/falseness. You should also avoid is True; simply omitting that code is usually equivalent, and preferable.

We should also note that index=0 is not handled here. I would personally add a line that raises an exception, at least, if you expect that that should never happen.

I don't have anything new to say about the next bit of code, so next I'll talk about the heap. The first thing I notice is that you're mixing up in terms of ordering the methods that are meant to be part of the interface and ones that are not, e.g. the bubble methods are only used internally. I would put the interface-methods at the top, and then put a comment between them and the internal methods saying the ones below the comment are internal. PEP 8 also recommends prefixing the internal methods with an underscore; I do this sometimes, but it's not a big deal to me when internal methods are simply separated.

I recommend replacing

self.heap += [k]

with

self.heap.append(k)

What you had creates an intermediate list without that being necessary, and I think people will read the append call more naturally anyway.

bubble_up is only called from one place. I'd inline it, or nest it within the one method that uses it. Same comments for bubble_down.

I would replace

def is_empty(self):
        """is heap empty?"""
        return self.size() == 0

with

def __bool__(self): return self.heap

as I think it's more Pythonic. (I suggest "magic" methods like __bool__ be placed after __init__ and before non-"dunder" (double underscore) methods.) I would use __len__ instead of your size as well.

That's all I have for the moment. I'll happily answer any follow-up questions, I left a lot of details out that would have made this even longer.