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

AddNodeLink and AddNodeLinkClean seem to have the same functionality #4729

Closed
schomatis opened this issue Feb 22, 2018 · 3 comments
Closed
Labels
topic/technical debt Topic technical debt

Comments

@schomatis
Copy link
Contributor

While reviewing code for issue #4052 I came across the functions AddNodeLink and AddNodeLinkClean that would seem to have the same code except for an extra line in AddNodeLink where the name is set to the link that will be added, but the link name is not used in the AddRawLink function that receives the link (as the name is specified as a separate argument), and the link itself seems to be discarded after the call (its members Size and Cid are copied but not the link structure).

Both functions are equally used in the code base but I'm failing (from reading the code and the comments) to see their difference, am I missing something?

@Stebalien
Copy link
Member

No, that looks like duplicate code. It looks like we used to cache children nodes inside link objects but don't do that anymore. Feel free to deduplicate.

(also, if I don't get back to you in a reasonable amount of time (e.g., 2 days) on a question, please hunt me down on IRC and yell at me)

@Stebalien Stebalien added the topic/technical debt Topic technical debt label Apr 1, 2018
@schomatis
Copy link
Contributor Author

@Stebalien So the name of the remaining function could be AddNodeLink? From the comments it seems like AddNodeLinkClean was the one that didn't cache the child node, but I don't think that would carry much meaning anymore.

(also, if I don't get back to you in a reasonable amount of time (e.g., 2 days) on a question, please hunt me down on IRC and yell at me)

No problem, this was a low priority issue.

@Stebalien
Copy link
Member

Yes, I'd go with AddNodeLink.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/technical debt Topic technical debt
Projects
None yet
Development

No branches or pull requests

2 participants