I have an object class with an update method: this method simply makes a few checks and updates the object's properties, nothing is returned a Bool is returned to determine whether it should be deleted.
Some of these checks outcomes preclude others execution: EDIT, Old Code Block:
def updateObject(self):
self.checkOne()
if not self.checkTwo():
self.checkThree():
def checkTwo(self):
if self.x == condition:
self.y = different value
return True
return False
New Code Block:
class MyObject():
def __init__(self, firstValueHash: int):
self.identifier # str (unused in this example)
self.isMarked # bool
# maxObserved is always greater than maxTrackedTicks
self.maxObservedTicks # int
self.maxTrackedTicks # int
self.ticksSinceUpdate # int
self.ticksSinceMarked # int
self.trackData = [None]*self.maxObservedTicks-1 # list of ints
self.trackData.append(firstValueHash)
# called externally once every 5 mins, returns True if object needs deleting, otherwise returns false
def performUpdate(self):
self.ticksSinceUpdate += 1
self.trackData = self.trackData[1:(self.maxObservedTicks-1)]
self.trackData.append(None) # some other function updates these None values
if self.ticksSinceUpdate > self.maxObservedTicks:
return True
if not self.__updateIfMarked():
self.__updateIfExpired():
return False
def __updateIfMarked(self):
if self.isMarked == True:
self.ticksSinceMarked += 1
if self.ticksSinceMarked < maxObservedTicks
return True
self.isMarked = False
self.ticksSinceMarked = 0
return False
def __updateIfExpired(self):
if self.ticksSinceUpdate >= maxTrackedTicks:
self.isMarked = None
# Other functions will update of isMarked outside of the 5 min update, by labelling it as True or False
As you can see, the outcome of checkTwo updateIfMarked() determines whether checkThree __updateIfExpired() occurs by returning a Bool.
This felt really uncomfortable to write, but it's the most clear and concise way I can think of to get the behavior I want.
Is this bad practice? I feel like it breaks the "one tool; one job" rule. I also think that it might make the behavior unclear due to excessive logical inversions...
I've worked out why writing this felt wrong:
If we look closely at if not self.__updateIfMarked() statement's overall behavior, it essentially says "execute __updateIfExpired() whenever the bool self.isMarked is False, but only after executing __updateIfTicked.
Crucially, self.isMarked is determined by the outcome of __updateIfTicked, so there must always be two checks of whether isTicked is True or False. Returning True or False to the if operator from __updateIfTicked is synonymous with:
def performUpdate(self):
self.ticksSinceUpdate += 1
self.trackData = self.trackData[1:(self.maxObservedTicks-1)]
self.trackData.append(None) # some other function updates these None values
if self.ticksSinceUpdate > self.maxObservedTicks:
return True
self.__updateIfMarked():
self.__updateIfExpired():
return False
def __updateIfMarked(self):
if self.isMarked == True:
self.ticksSinceMarked += 1
if self.ticksSinceMarked >= maxObservedTicks
self.isMarked = False
self.ticksSinceMarked = 0
def __updateIfExpired(self):
if self.ticksSinceUpdate >= maxTrackedTicks and self.isMarked = False:
self.isMarked = None
The order of operation is crucial to getting the intended behavior: a sliding buffer which preserved 'Marked' entities beyond the Tracking window.
Whether you should use None as a third state is maybe questionable. Perhaps an enum would be more appropriate?