Issue #110 - Probe Request Information Element Order change #116
Conversation
|
Thank you for your PR. I will take a look at it. |
|
Please let me know your feedback on this when you get a chance to review. Thanks |
|
I'm also very interested in this and would love to help if necessary. |
|
Sorry for my late response. Currently I don't have enough time for Pcap4J... |
kaitoy
left a comment
kaitoy
left a comment
There was a problem hiding this comment.
In addition to comments in line, getRawFields, buildString, calcHashCode, and equals must be modified with consideration of element order.
| private final Dot11InterworkingElement interworking; | ||
| private final Dot11MeshIdElement meshId; | ||
| private final List<Dot11VendorSpecificElement> vendorSpecificElements; | ||
| private Dot11SsidElement ssid = null; |
There was a problem hiding this comment.
All fields should be final as possible.
| private Dot11MeshIdElement meshId = null; | ||
| private List<Dot11VendorSpecificElement> vendorSpecificElements = new ArrayList<Dot11VendorSpecificElement>(); | ||
|
|
||
| private List<Dot11InformationElement> infoElementOrderedList = new LinkedList<Dot11InformationElement>(); |
There was a problem hiding this comment.
infoElementOrderedList holds the same objects as the other fields such as meshId, which is a bad practice.
| int elemLen = elem.length(); | ||
| offset += elemLen; | ||
| length -= elemLen; | ||
| short iterCount = 0; |
There was a problem hiding this comment.
I want the below code to be replaced with a packet factory like StaticIpV4OptionFactory for maintainability and enhanceability.
| * @return infoElementOrderedList | ||
| */ | ||
| public List<Dot11InformationElement> getInformationElementOrderedList() { | ||
| return infoElementOrderedList; |
There was a problem hiding this comment.
All fields must be immutable. So, here must make a defensive copy.
4 participants
Please review these initial version of changes. Since I have to iterate inside the header elements constructor I am forced to take out the "final" declarations on information elements. I didn't want to do that though. Please take a look at this and let me know your thoughts on it. Also I have not worked on factory implementation yet. I will be working on that soon. Thanks