Writing a class to hold Arduino Properties for an Arduino I2C Debugger

Question

I have this class which mostly holds a bunch of properties and their setters. I also have wrappers for some functions of another module. Here is the class:

class ARCArduino(object): """ Class to manage Arduino functionality """ def __init__(self, tree_item, port, identifier): self.__i2c = ArduinoI2C() self.__i2c.setPort(port) self.__i2c.connectPort() self.__id = None self.__i2c_version = None self.__switch_version = None self.__tree_item = tree_item self.__address_list = [] self.__port = port self.__identifier = identifier @property def id(self): if self.__id is None: try: self.__id = self.__i2c.getID() except IOError: self.__id = "NO CONNECTION" return self.__id @property def i2c_version(self): if self.__i2c_version is None: try: self.__i2c_version = self.__i2c.i2cVersion() except IOError: self.__i2c_version = "NO CONNECTION" return self.__i2c_version @property def switch_version(self): if self.__switch_version is None: try: self.__switch_version = self.__i2c.switchVersion() except IOError: self.__switch_version = "NO CONNECTION" return self.__switch_version @property def tree_item(self): return self.__tree_item @tree_item.setter def tree_item(self, item): if type(item) is not wx.TreeItemId: return self.__tree_item = item @property def address_list(self): return self.__address_list @address_list.setter def address_list(self, addr_list): if type(addr_list) is not list: return self.__address_list = addr_list @property def port(self): return self.__port @port.setter def port(self, port): if port[:3] != "COM": return self.__port = port @property def identifier(self): return self.__identifier @identifier.setter def identifier(self, letter): if type(letter) is not str: return if letter[-1] != ':': letter += ':' self.__identifier = letter def serialCommand(self, command): """ Wrapper for ArduinoI2C serialCommand function """ return self.__i2c.serialCommand(command) def close(self): self.__i2c.close() def connectPort(self): self.__i2c.connectPort() 

This implementation feels a little dumb to me. Is there a more elegant way to implement something like this?

Here is some further clarification on the functionality of the class:

self.__i2c is an instance of an Arduino I2C communication module that was internally developed at my company. It allows users to send commands to I2C devices over the Arduino's I2C bus. setPort and connectPort are setting and connect a COM port for the specific Arduino.

self.__id is a serial number generated for the Arduino from a company-MySQL database. It is set elsewhere in the program.

self.__switch_version and self.__i2c_version are two floats stored on the Arduino that are the version number of some software running on the Arduino.

self.__tree_item is a wx.TreeItemId (I believe it's stored as just an int) that is the id of the wx.TreeItem stored in a wx.TreeCtrl elsewhere in the program.

self.__address_list is a list of ints which are the I2C addresses of devices discovered on the Arduino's I2C bus.

self.__port is a string which holds the COM port the Arduino is inserted in.

self.__identifier is a letter which identifies the Arduino elsewhere in the program. The program expects the user has multiple Arduinos concurrently being used, so a letter is a quick shorthand to easily identify them.

Answer 1

Here is some feedback I have. I haven't had a chance to work with Arduino much, but there are some things that stand out to me:

 # Nothing to glaring in your constructor, but some tidbits: # I might add some defaults. # I also noticed you have a lot of items in your constructor. # Maybe **kwargs is warranted. def __init__(self, tree_item, port=8080, identifier): or def __init__(self, **kwargs) # the number of arguments is borderline imho. I usually find if I # have too large a number (subjective), there is opportunity to # break out the code into more classes. self.__i2c = ArduinoI2C() self.__i2c.setPort(port) self.__i2c.connectPort() self.__id = None self.__i2c_version = None self.__switch_version = None self.__tree_item = tree_item self.__address_list = [] self.__port = port self.__identifier = identifier # Typically methods indicate some sort of action. i.e. get_id, show_id. # specifying a noun is usually grounds for a class/instance variable. # I would consider renaming your method to "def get_id(self):" @property def id(self): if self.__id is None: try: self.__id = self.__i2c.getID() except IOError: # It may be good to add some logging. # python logging module is great. self.__id = "NO CONNECTION" return self.__id # ditto method name @property def i2c_version(self): if self.__i2c_version is None: try: self.__i2c_version = self.__i2c.i2cVersion() except IOError: # ditton on the logging. self.__i2c_version = "NO CONNECTION" return self.__i2c_version # ditto method name @property def switch_version(self): if self.__switch_version is None: try: self.__switch_version = self.__i2c.switchVersion() except IOError: self.__switch_version = "NO CONNECTION" return self.__switch_version # ditto method name @property def tree_item(self): return self.__tree_item # ditto method name @tree_item.setter def tree_item(self, item): if type(item) is not wx.TreeItemId: return self.__tree_item = item # ditto method name @property def address_list(self): return self.__address_list # ditto method name @address_list.setter def address_list(self, addr_list): if type(addr_list) is not list: return self.__address_list = addr_list # ditto method name @property def port(self): return self.__port # ditto method name @port.setter def port(self, port): if port[:3] != "COM": return self.__port = port # ditto method name @property def identifier(self): return self.__identifier # ditto method name @identifier.setter def identifier(self, letter): if type(letter) is not str: return if letter[-1] != ':': letter += ':' self.__identifier = letter # ditto, and its not idiomatic to use camelCase in a method name. # def run_serial_command(self, command) def serialCommand(self, command): """ Wrapper for ArduinoI2C serialCommand function """ return self.__i2c.serialCommand(command) # this method name is ok def close(self): self.__i2c.close() # this one two.. ditch the camelCase for snake_case connect_port def connectPort(self): self.__i2c.connectPort() 

It looks like there might be some opportunity for grouping your commands into separate classes, however not sure if you have enough per group yet.

i.e.

class ArduinoVersions(ARCArduino): # this might be to ambiguous, but just trying to give the general idea. class ArduinoProperties(ArcaArduino): 

I have a feeling your command set will grow, so breaking this out may provide better organization in the future.