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

Base isn't needed in main branch of algorithm #259

Closed
chaals opened this issue Jan 4, 2023 · 2 comments · Fixed by #260
Closed

Base isn't needed in main branch of algorithm #259

chaals opened this issue Jan 4, 2023 · 2 comments · Fixed by #260

Comments

@chaals
Copy link
Contributor

chaals commented Jan 4, 2023

There's no need for calculating the base unless you're going to use it. Moving it will simplify the algorithm, especially for "naive" implementations that just follow the spec instructions.

@marcoscaceres
Copy link
Member

Actually, I had to revert #260. The |base| is actually used by validate share data, so it's declared in the right place.

@chaals
Copy link
Contributor Author

chaals commented Jan 23, 2023

It's not completely obvious that the variables created in the course of executing a particular algorithm are expected to be generally available (if this were real code, you'd have a way of declaring a "global" variable differently from one that is locally scoped, but since it isn't real code the assumptions underlying it need pretty clear documentation IMHO).

So it makes more sense (to me) to add the same line (again, if needed because there is indeed a URL) to validate share data algorithm too. An alternative approach would be is to explicitly point to where those are declared in the other algorithm, and another would be to explicitly state that variables are global in scope. That seems ugly to my particular sense of programming aesthetics. Which doesn't stop me doing it in practice...

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 a pull request may close this issue.

2 participants