Skip to content

Conversation

@ChefBingbong
Copy link

@ChefBingbong ChefBingbong commented Jun 14, 2025

this PR improves null cheks around the code base to improve typesafety of code and to avoid optional chaining when accessing items or functions inside entities or objects, hence improving readabilit, code cleanliness and reliability.

changes

  1. remove optional chaining and only access items inside objects if the entity is defined
  2. improve the logic in the discv5 start and stop methods by reducing duplicate code and makinging sure ip4 or ip6 sokets are defined before attempting to open or close their connections

@ChefBingbong ChefBingbong requested a review from a team as a code owner June 14, 2025 11:23
@CLAassistant
Copy link

CLAassistant commented Jun 14, 2025

CLA assistant check
All committers have signed the CLA.

@ChefBingbong ChefBingbong changed the title fix: better type safety and no null checks fix: better type safety and no null checks or optional chaining Jun 14, 2025
@ChefBingbong ChefBingbong changed the title fix: better type safety and no null checks or optional chaining fix: better type safety for null checks and optional chaining Jun 14, 2025
@wemeetagain
Copy link
Member

Hm, type-safety is a pretty objective measurement. I don't see any improvement from this PR. Reason being that optional chaining (a builtin language feature) has a quite clear meaning, and it is well modeled in TypeScript. Your change (1) is roughly (but not entirely!) equivalent, so it is mostly a lateral move wrt type-safety.

Wrt code cleanliness / readability / maintainability, this is somewhat subjective. And here I disagree with the changes you have. With 1, this is simply more verbose with no clear benefit. We think optional chaining is a useful syntax sugar and should be used in place of nullish checks in if or ternarys. We're firm on this. With 2, this is somewhat more subjective. But the intent in code here is to be explicit. We don't need to generically collect and iterate over sockets/configs, it doesn't even save us any lines of code! Because of the really limited/subjective benefit, I choose not to accept this change either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants