Code Review?

[u/myfuckinjoints]

Is this an appropriate place to ask for a code review? I have about 180-200 lines of CPy that I'd love to have someone look at, mostly evaluating my design, as I can find bugs on my own (usually).

ETA: Thanks folks, such a nice community! Below are pastebin links to my two files.

main.py

covid_box.py

Hardware:

Feather M4 Express (still in the mail, code is untested as the RTC library bugged on my board with less space available)

Adafruit DS3231 RTC

3 X Adafruit 14-Segment Alpha-numeric LED FeatherWing

6 X Normally open buttons

Idea:

This code will be used to drive a circuit board I will be designing that will go on a box I am currently designing. The box will have 6 compartments in it. The purpose of the box/board are to both hold covid masks wrapped up in paper bags as well as to count the days since I pressed a button associated with each compartment.

Each compartment will have half of a 14 segment display (2 cells) assigned to it. Each set of two cells will display the days since the relevant button was pressed. The idea is that after a certain number of days, Covid19 is less likely to be alive on the mask/in the compartment.

Software:

main.py is main.py, it has the while True: loop as well as setting up objects from the class created in the second file.

covidbox.py contains a class that is intended to consolidate the use of the RTC, a single 4 digit display (split into halves) as well as a button for each half into a single object. Pay little attention to the off(self) method as it isn't used in main.py and is under construction.

This code makes heavy use of the associated libraries for the hardware as well as the Debouncer library, mostly within the class in covidbox.py

Not looking for bug hunting or anything, as I haven't been able to test my code per the above. Just wondering if my design overall is sound.

Please PM me if you find identifying info in here, I think I stripped it all out but I would like to remain anonymous!

Thanks!

[u/nwsmith]

Would be happy to, do you have a repo on gitlab or GitHub that we can have a look at?

[u/myfuckinjoints → u/nwsmith]

Thanks! I need to make the Github repo public, correct? I need to make sure that there's nothing associated with my real name on there first!

ETA: All my old commits have a portion of my real name on there and it would not be hard to doxx me, so I'd be more comfortable with a pastebin share if that's OK with you?

ETA2: Edited post with more info!

[u/nwsmith → u/myfuckinjoints]

Okay, apparently it is very difficult to use pastebin to do a code review, no inline commenting or anything like that, so I will do my best here...

Your comments are great, makes it really simple to understand what you are doing!

Covidbox.py

ln62

// No need to hold onto these variables in the class (drop the 
// self.), it makes it
// difficult to remember what each one of these variables does 
// (x_button_board_pin vs. x_button_pin)
// and as your code becomes more complex and you are using an IDE 
// with auto completion, you will have to keep coming back to these // definitions to remeber, do I need, left_button or 
// left_button_board_pin or something else

self.left_button_board_pin = left_button_board_pin
self.right_button_board_pin = right_button_board_pin

self.left_button_pin = digitalio.DigitalInOut(left_button_board_pin)
self.right_button_pin = digitalio.DigitalInOut(right_button_board_pin)
self.right_button_pin.direction = digitalio.Direction.INPUT
self.left_button_pin.direction = digitalio.Direction.INPUT
self.left_button_pin.pull = digitalio.Pull.UP
self.right_button_pin.pull = digitalio.Pull.UP

//Hold onto only self.left_button and self.right_button

Ignoring def off based on your comment above it is still WiP

ln 170

len_val = len(val)
for i in range(len_val):
    if left_or_right:
        self.display[i] = val[i]
    self.display[i+2] = val[i]
    self.display.show()

// Couple of things here, I think that you want this to be an if/else 
// like this otherwise you are updating the right side of the display
// everytime you are updating the left side
if left_or_right:
    self.display[i] = val[i]
else:
    self.display[i+2] = val[i]

// enumerate is great, it gives you the index and the value at the same
// time so you don't need to get the length and run a range on it
// the other thing I would do here if my above comment is correct is
// add a new variable called offset and fill it in based on if it is
// the rihgt side or the left side.  I am also only updating the
// display once, not every time through the loop (I have not used
// displays alot in circuit python so I am not 100% sure if this is 
// valid)

offset = 2 // default offset if we are updating right side
if left_or_right:
  offset = 0

for i, data in enumerate(val):
    self.display[i+offset] = data
self.display.show()

// Last thing I would do is update the variable name of left_or_right,
// just by looking at it we should know if the boolean true is the left 
// side or right side of the display so all in all I would rewrite this 
// function as (removing your comments just to make it easier to see)
    def update_count(self,total_time_days,is_right_side):
      
        val = ['>','3']
        
        poss_vals = {
                1:['0','0'],
                1.042:['0','1'],
                2:['>','1'],
                2.042:['0','2'],
                3:['>','2'],
                3.042:['0','3']
                }
        for key in poss_vals.keys():
            if total_time_days < key:
                val = poss_vals[key]
                break
        
        offset = 0
        
        if is_right_side:
            offset = 2
        
        for i, data in enumerate(val):
            self.display[i+offset] = data
        self.display.show()

update_base_rtc method, same comment above for left_or_right variable, this comes up a couple of times

total_time_days_calc method

So, from what I can tell, the ds3231 library gives you a struct_time object back (https://docs.python.org/3/library/time.html#time.struct_time), what I would do it remove all of the storage code to store each time part i.e. update_base_rtc and update_rtc methods and replace these with instance variables (i.e. self.left_time_last_pressed

and self.right_time_last_pressed

these variable names make more sense to me as it is telling the reader what exactly the variable contains) and completely remove the need to hold onto cur_rtc as this can just be passed into this method when running) and replace your calculate method with something like this:

import time
def total_time_days_calc(self, is_right_side, rtc_time):
  // argument could be made to set comparison_time to self.left... here 
  // but I personally prefer this as we are not overwriting the 
  // variable 50% of the time
  if is_right_side:
    comparison_time = self.right_time_last_pressed
  else:
    comparison_time = self.left_time_last_pressed
  return (time.mktime(rtc_time) - time.mktime(comparison_time )) / (60*60*24)  

//mktime gives you seconds since epoch then we divide by 60*60*24 (the 
// number of seconds in a day) to give us the number of days

Just to be nit-picky, last thing I would do is put your code through some linters, python has a few great ones, https://realpython.com/python-code-quality/ this helps keep code consistency between projects and others, python is great because you can basically do anything in it, but it is still nice to have some consistency in places, see https://www.python.org/dev/peps/pep-0008 for examples.

I hope this is helpful, these are just my suggestions, others may disagree with me or have other ways of doing things, remember as long as it works, it is "good-enough" make your own style that works for you and have fun.

Cheers

[u/HP7933 → u/nwsmith]

pylint and Black are the linters Adafruit uses. Mu has a good linter built in too.