9
\$\begingroup\$

I am somewhat new to python (yes I know, a cliché statement) and I wrote this code to scroll through a tuple of strings using the arrow keys. I know that the code is sloppy but I don't know how. Any feedback would be greatly appreciated.

import keyboard
from os import system
from time import sleep


class Menu:
    def __init__(self, *items):
        self.items = items
        self.selections = 6
        self.start = 0
        self.stop = self.start + self.selections
        self.slice = items[self.start:self.stop]
        self.selected = 0
        self.beg = '>'
        self.end = '<'

    def get_input(self):
        if keyboard.is_pressed('up'):
            if self.selected >= 0:
                self.selected += -1
            self.show()
        elif keyboard.is_pressed('down'):
            if self.selected < self.selections:
                self.selected += +1
            self.show()

    def show(self):
        system('cls')

        # shift slice up if the bottom of list is reached
        if self.selected == self.selections:
            if self.stop < len(self.items):
                self.start += 1
                self.stop += 1
                self.slice = self.items[self.start:self.stop]
            self.selected = self.selections-1

        # shift slice down if top of list is reached
        elif self.selected < 0:
            if self.start > 0:
                self.start += -1
                self.stop += -1
                self.slice = self.items[self.start:self.stop]
            self.selected = 0

        # print the slice with the selected item printed like >this<
        for i in range(len(self.slice)):
            if i == self.selected:
                print(self.beg + self.slice[i] + self.end)
            else:
                print(self.slice[i])


test_menu = Menu('item 0',
                 'item 1',
                 'item 2',
                 'item 3',
                 'item 4',
                 'item 5',
                 'last item')
test_menu.show()


while 1:
    test_menu.get_input()
    sleep(0.1)
\$\endgroup\$
1
  • 2
    \$\begingroup\$ this works only on windows, since linux doesn't have a cls command \$\endgroup\$ – hjpotter92 Oct 21 '20 at 22:53
3
\$\begingroup\$

You should organise the code in such way that each function performs one function (hence the name ;)) and what it does should be what its name says. For example, show should be printing (i.e. showing) the menu, not modifying internal state.

Only this fits in:

def show(self):
    system('cls')

    # print the slice with the selected item printed like >this<
    for i, item in enumerate(self.slice):
        if i == self.selected:
            print(self.beg + item + self.end)
        else:
            print(item)

The rest should be done in the function which handles moving up or down.

That said, I would change get_input as well, to something like this:

def process_input(self):
    if keyboard.is_pressed('up'):
        self.move_cursor(-1)
    elif keyboard.is_pressed('down'):
        self.move_cursor(1)

And then:

def move_cursor(self, offset):
    self.selected = min(max(self.selected + offset, 0), self.selections-1)

    # handle moving the slice here

    self.show()

I hope I got that min/max limit right :)

\$\endgroup\$
2
  • \$\begingroup\$ BTW, is there need to have beg , end and selections as attributes? I would hard-code beg and end and have print(f'>{item}<1') \$\endgroup\$ – zvone Oct 23 '20 at 18:13
  • \$\begingroup\$ This helps a lot. I didn't think to put move cursor in it's own function. That does look cleaner. I don't know what you did with min/max functions lol, but I can either figure that out or find another way to do it. Thank you for your input! \$\endgroup\$ – Robert Alvies Oct 24 '20 at 9:48
3
\$\begingroup\$

get_input should be called handle_input. get_input suggests that it returns input values, whereas really, it's a "step" function used to handle what's going on at a slice in time.

Also in that function, self.selected += -1 is confusing. Why add a negative number instead of subtracting? I would change that and the other similar instances to self.selected -= 1. Personally, I'd also get rid of the prefix + in +1. I don't find it adds anything.


Sorry, I wrote another review before this and my brain's tired. Hopefully someone else can give better suggestions.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thankyou for your feedback! Naming does seem to be my biggest hurdle. \$\endgroup\$ – Robert Alvies Oct 22 '20 at 9:15
  • 1
    \$\begingroup\$ @RobertAlvies To be fair, naming is hard. I'll rename a finction many times before I'm finally happy with it. \$\endgroup\$ – Carcigenicate Oct 22 '20 at 13:13

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.