I am trying to learn WHEN NOT to use:
- classes
- member variables
HERE IS THE CODE
access_point_detection_classes.py
from scapy.all import *
class Handler :
def __init__(self) :
self.wap_list = []
self.TempWAP = None
def process_packet_capture(self, packet_capture) :
for packet in packet_capture :
if packet.haslayer(Dot11) :
if packet.subtype == 8 :
bssid = packet.addr3
if packet.haslayer(Dot11Elt) :
p = packet[Dot11Elt]
while isinstance(p, Dot11Elt) :
if p.ID == 0 :
essid = p.info
if p.ID == 3 :
channel = ord(p.info)
p = p.payload
self.TempWAP = WirelessAccessPoint(bssid, essid, channel)
if self.check_for_duplicates() == False:
self.wap_list.append(self.TempWAP)
def check_for_duplicates(self) :
for w in self.wap_list :
if self.TempWAP.bssid == w.bssid :
return True
return False
class WirelessAccessPoint :
def __init__(self, bssid, essid, channel) :
self.bssid = bssid
self.essid = essid
self.channel = channel
access_point_detection.py
from scapy.all import *
from access_point_detection_classes_module import *
def main() :
H = Handler()
packet_capture = sniff(count = 50)
H.process_packet_capture(packet_capture)
for w in H.wap_list :
print w.bssid
print w.essid
print w.channel
print '++++++++++++++++++++++++'
# enable_managed_mode('wlan0')
if __name__ == "__main__" :
main()
HERE ARE MY THOUGHTS
I am mostly sure I know WHEN to use a class and member variables. Look at my second class WirelessAccessPoint
. The perfect need for a class in my opinion.
- I need multiple objects of this type
- Easier management of the data that comes along with this type
That's great, but look at my first class Handler
. To be honest, I mostly created it so I could practice OOP. It does come in handy when I need to access wap_list
. However, I initialize it without passing parameters for the constructor. Is that defeating the purpose? It seems that I could create all the methods of this class with only the self
parameter to them. For instance ( haha ) I could make packet_capture
a member variable of Handler
and have the method process_packet_capture()
take the parameter self
.
At this point I feel like if I do not set some ground rules for myself I will try to make everything a class and everything a member variable and I will never pass a parameter besides self
ever again!
-
When the costs outweigh the benefits.Robert Harvey– Robert Harvey2016年07月26日 05:21:58 +00:00Commented Jul 26, 2016 at 5:21
-
@Robert What are the 'costs' of using a class / member variable?ma77c– ma77c2016年07月26日 05:25:03 +00:00Commented Jul 26, 2016 at 5:25
-
Mutability, overall increase in complexity, increased storage requirements, increased need for testing, increased need for documentation, higher maintenance costs.Robert Harvey– Robert Harvey2016年07月26日 05:26:17 +00:00Commented Jul 26, 2016 at 5:26
3 Answers 3
However, I initialize it without passing parameters for the constructor. Is that defeating the purpose?
No, absolutely not. Handler, maybe misnamed, but in a sense it represents a collection object, which is perfectly reasonable without constructor parameters. A collection object maintains some state for it's duration, and, many collections are created with an initially empty set.
However, what I don't necessarily like is the use of a member variable for the short-lived state of TempWAP. I think a local variable and some parameter passing would be better here, no?
Maybe the example doesn't make it clear that TempWAP is otherwise used in your more complete implementation, or, as another alternative, you might actually have two classes here that are being conflated (one shorter lived and one longer lived), but based on what I can see in the question, I'd go for local variable instead of member variable here.
The point is that the lifetime of the two member variables in Handler seems inconsistent. So, these member should be differentiated, either one using a local variable, or in another class (overkill, though perhaps). To reiterate, when all your member variables in the same class have the same useful lifetime, you have a better class than otherwise.
-
TempWAP
is used throughout the for loop. However, it is completely replaced and assigned with each iteration of the loop. Also, no other method of the class uses it. Whilewap_list
is used in several methods as well as being appended to ( not reassigned ). Is this the reasoning?ma77c– ma77c2016年07月26日 04:04:16 +00:00Commented Jul 26, 2016 at 4:04 -
Yes, I think that is a good analysis.Erik Eidt– Erik Eidt2016年07月26日 05:04:54 +00:00Commented Jul 26, 2016 at 5:04
Don't use classes and or member variables when they are not needed. As soon as you need them, well, use them.
I'm serious.
A variable should always have the smallest scope and the shortest lifetime possible. Therefore a class member is preferable to a global variable, and a local variable or parameter is preferable to a class member.
In your example, TempWAP
is only used inside the execution of process_packet_capture()
and could therefore be changed to a local and passed as a parameter to check_for_duplicates()
. As a member variable, it is available after the execution, but does not have any useful value. wap_list
on the other hand is accessed after the execution, so this has to be a member.
But depending on how you use Handler
you could decide to rewrite process_packet_capture()
to a stand-alone function which returns wap_list
. This would make it simpler to use in my opinion, but this honestly comes down to preference. Some like to follow an OO paradigm throughout, while I personally prefer side-effect free stand-alone functions when possible.
The packet_capture
variable is only used as input to process_packet_capture()
, so given the above principles I see no reason to turn it into a member.
I do see a slight risk with having wap_list
as a member rather than a return value. It means you might mistakenly access wap_list
before process_packet_capture()
is called, and if you call process_packet_capture()
multiple times, wap_list
will contain the accumulated results of all calls. This might be what you intend, but if not, then having it as a return value rather than member avoids this risk of misuse. Alternatively, you could have a packet_capture
as a argument to the constructor and call process_packet_capture(packet_capture)
directly in the constructor. This way you still have wap_list
as a member but avoid the risk of misuse.
Explore related questions
See similar questions with these tags.