# The console prints a list of items for the user to scroll through using the up and down arrow keys

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

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])

'item 1',
'item 2',
'item 3',
'item 4',
'item 5',
'last item')

while 1:
sleep(0.1)

• this works only on windows, since linux doesn't have a cls command – hjpotter92 Oct 21 '20 at 22:53

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 :)

• 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') – zvone Oct 23 '20 at 18:13
• 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! – Robert Alvies Oct 24 '20 at 9:48

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.

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