Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernise IsStartNode() #1506

Merged
merged 32 commits into from
Apr 5, 2022
Merged

Modernise IsStartNode() #1506

merged 32 commits into from
Apr 5, 2022

Conversation

originalfoo
Copy link
Member

@originalfoo originalfoo commented Apr 2, 2022

Replaces obsolete ExtSegmentManager.IsStartNode() with NetSegment extensions - either IsStartnode() IsStartNode() (always returns a bool) or IsStartNode() GetRelationToNode() (may return null if node not attached to segment) as applicable.

Additionally, updates a few encountered for loops like for (int i = 0; i < 8; ++i) to more descriptive for (int segmentIndex = 0; segmentIndex < Constants.MAX_SEGMENTS_OF_NODE; ++segmentIndex).

Small number of other minor tweaks - eg. to replace some other obsolete stuff in proximity/relation to the main task.

Probably has a lot of overlap with stalled PR #1272.

@originalfoo originalfoo added the code cleanup Refactor code, remove old code, improve maintainability label Apr 2, 2022
@originalfoo originalfoo added this to the 11.6.5.2 milestone Apr 2, 2022
@originalfoo originalfoo self-assigned this Apr 2, 2022
@originalfoo originalfoo marked this pull request as draft April 2, 2022 23:34
@originalfoo originalfoo changed the title WIP: Modernise IsStartNode() Modernise IsStartNode() Apr 3, 2022
@originalfoo originalfoo marked this pull request as ready for review April 3, 2022 01:48
@kianzarrin
Copy link
Collaborator

we also have IsstartNode() that does returns a simple bool.

@originalfoo
Copy link
Member Author

we also have IsstartNode() that does returns a simple bool.

I assume you mean IsStartnode() (lowercase n) ? I'm already using that one where simple bool is required (it's slightly faster as it only does one comparison).

I couldn't find any IsstartNode() method with the lowercase s.

Copy link
Collaborator

@kvakvs kvakvs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then I kinda like this code change

- Added some xmldoc
- Improved consistency in file
- Suppressed some warnings
@originalfoo
Copy link
Member Author

bump :)

Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, didn't notice problems

@originalfoo originalfoo merged commit fd77aa3 into master Apr 5, 2022
@originalfoo originalfoo deleted the modernise-IsStartNode branch April 5, 2022 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Refactor code, remove old code, improve maintainability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants