Added guard clause for NetworkMap.TryGetValue for graceful error handling#40
Conversation
There was a problem hiding this comment.
Pull request overview
Improves NetworkMap<T>.TryGetValue error handling so lookups don’t throw when address comparisons fail due to differing BinaryNumber lengths (commonly seen with IPv4 vs IPv6).
Changes:
- Wraps
IpEntrycreation andGetFloorEntry/GetCeilingEntrycalls in atry/catchand returnsfalseon failure. - Refactors local variable declarations to support the new guarded flow.
- Adds inline comments explaining the failure scenario.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for the PR. The NetworkMap is supposed to be used only with one address family. I think this can be handled by adding a constructor param which specifies the address family to use and then check for that in the public methods. |
|
Hi @ShreyasZare, In the unit tests PR, I wrote a test case for multiple family of addresses and I saw it failed. The address family restriction is implicit currently. Without changing the interface, I wanted to make the behavior more explicit, also pass the said test. You can see that I used a try catch block initially but it was implicit as well, even with comments. So, I thought, a proper guard clause would fit the design more. As you've mentioned, this project is used by many of your internal projects too, so I do not add anything changing neither the interface nor the behavior. |
Ya, you are right since it will break things if constructor was updated. One DNS server app is using it for both ipv4 and ipv6 so there will be issue in there too. So your solution is optimal here. |
8fbc5f8 to
d4870ba
Compare
|
Done |
afe53e3 to
2d9748d
Compare
Solves #42:
Added guard clauses to ensure a
NetworkMapis only of type IPv4 or IPv6. It throws exceptions when needed.