Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

network: Documented, simplified and cleaner eachBin function in pot.go #1783

Merged
merged 3 commits into from
Sep 24, 2019

Conversation

kortatu
Copy link
Contributor

@kortatu kortatu commented Sep 19, 2019

I've tried to make EachBin function more clear to developers. It used to have this signature:

func (t *Pot) EachBin(val Val, pof Pof, po int, f func(int, int, func(func(val Val) bool) bool) bool) {

...and almost no documentation. It was difficult to understand what was the purpose of each of the three annonymous function parameters of EachBin.

I've created several abstractions to try to express better the behaviour of these iterators.

There is a new type ValConsumer with signature: type ValConsumer func(Val) bool that consumes a value Val and returns whether the caller wants to consume more values.
This consumer can be generalized so I created also BinConsumer with similar signature:
type BinConsumer func(bin *Bin) bool. Unsurprisingly, this function consumes a Bin and returns whether to continue consuming Bins or stop.

There is also a ValIterator function type:
type ValIterator func(ValConsumer) bool
that completes the abstraction of iterators. An iterator of type T takes a consumer of type T (ValConsumer in this case) and starts iterating T elements. The iterator returns a boolean signaling the last value returned by the last consume function. The ValIterator in our case is responsible of iterating Val elements inside a Bin.
Finally, Bin is just a struct with the information provided to a BinConsumer, its ProximityOrder, its Size and the ValIterator to traverse all its Val elements:

type Bin struct {
	ProximityOrder int
	Size           int
	ValIterator    ValIterator
}

With this new types, the signature of EachBin now is:

func (t *Pot) EachBin(pivotVal Val, pof Pof, minProximityOrder int, binConsumer BinConsumer) {

I also renamed the parameters to express their use.

…o. Crated several types to express the use of several complicated functions
@kortatu kortatu added ready for review pot cleanup code completion, add comments and more kademlia labels Sep 19, 2019
Copy link
Member

@zelig zelig left a comment

Choose a reason for hiding this comment

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

very nice refactor.
NOTE that pot will be rewritten very soon. Dont spend too much time deciphering this

pot/pot.go Outdated
type ValConsumer func(Val) bool

// ValIterator is a function that iterates values and executes for each of them a supplied ValConsumer.
// it returns the result of the last ValConsumer executed. Usually is the pin of a pot but if could be the last element
Copy link
Contributor

Choose a reason for hiding this comment

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

if -> it

pot/pot.go Outdated
type ValConsumer func(Val) bool

// ValIterator is a function that iterates values and executes for each of them a supplied ValConsumer.
// it returns the result of the last ValConsumer executed. Usually is the pin of a pot but if could be the last element
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually is the pin of a pot

I am not sure I understand what you mean here.

Copy link
Contributor Author

@kortatu kortatu Sep 20, 2019

Choose a reason for hiding this comment

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

Almost all uses of EachBin will use k.base as pivotVal, so it will be the node address and the "pin" of the pot.
In that situation eachBin is just traversing the bins from less po to more po and finally consuming the pin of the pot.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. But I am unsure about the wording here. "Usually the value passed to the ValConsumer is the pin"

If we want a tutorial-ish overview of the use cases in the comment, maybe they should be in bullet points and give a bit more context. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact I think I'm going to change it. The last val consumed will be always the pivot value.

pot/pot.go Outdated
n = t.bins[i]
size += n.size
if n.po < po {
var pot *Pot
Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider using a different variable name than the package name

pot/pot.go Show resolved Hide resolved
network/kademlia.go Show resolved Hide resolved
@kortatu
Copy link
Contributor Author

kortatu commented Sep 20, 2019

very nice refactor.
NOTE that pot will be rewritten very soon. Dont spend too much time deciphering this

Ok, let the rewrite of pot have a better guideline for rewrite EachBin function

@nolash
Copy link
Contributor

nolash commented Sep 20, 2019

rewritten very soon

My bet is it won't be THAT soon ;)

@kortatu kortatu merged commit 7006159 into ethersphere:master Sep 24, 2019
@kortatu kortatu deleted the clean-eachbin-function branch September 24, 2019 13:36
@skylenet skylenet added this to the 0.5.0 milestone Sep 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cleanup code completion, add comments and more kademlia pot ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants