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

Refactor keys() and values() internals #13

Closed
vweevers opened this issue Jan 16, 2022 · 1 comment
Closed

Refactor keys() and values() internals #13

vweevers opened this issue Jan 16, 2022 · 1 comment
Labels
semver-major Changes that break backward compatibility

Comments

@vweevers
Copy link
Member

Follow-up for #12, to be tackled in a next major version. As written there:

Adds a lot of new code, with unfortunately some duplicate code because I wanted to avoid mixins and other forms of complexity, which means key and value iterators use classes that are separate from preexisting iterators. For example, a method like _seek() must now be implemented on three classes: AbstractIterator, AbstractKeyIterator and AbstractValueIterator. This (small?) problem extends to implementations and their subclasses, if they choose to override key and value iterators to improve performance.

To come up with a more DRY approach, it may help to first reduce the differences between the 3 iterators. Mainly: change the callback signature of AbstractIterator#next() from (err, key, value) to (err, entry). Perhaps (dare I say) remove callbacks altogether.

If we can then merge the 3 classes into one, or at least have a shared and reusable base class, then unit tests can probably be simplified too, not having to repeat like so:

for (const mode of ['iterator', 'keys', 'values']) {
test(`for await...of ${mode}()`, async function (t) {
t.plan(1)
const it = db[mode]({ keyEncoding: 'utf8', valueEncoding: 'utf8' })
const output = []
for await (const item of it) {
output.push(item)
}
t.same(output, input.map(({ key, value }) => {
return mode === 'iterator' ? [key, value] : mode === 'keys' ? key : value
}))
})

Lastly (unrelated but I postponed it because of the next() callback signature and to avoid more breaking changes) perhaps rename iterator() to entries().

@vweevers vweevers added the semver-major Changes that break backward compatibility label Jan 16, 2022
@vweevers vweevers added this to Level Jan 16, 2022
@vweevers vweevers moved this to Backlog in Level Jan 16, 2022
@vweevers
Copy link
Member Author

Although #50 aligned the function signatures between the three types of iterators, I don't see a clean way to (further) merge them. The thing is, if we don't separate behavior by classes, then the separation must be made in individual methods, which would be worse. For example, to decode data, we currently do (simplified and in pseudocode):

class CommonIterator {
  async next () {
    const item = await this._next()
    return this.decodeItem(item)
  }
}

class AbstractIterator extends CommonIterator {
  decodeItem (entry) {
    entry[0] = this.keyEncoding.decode(entry[0])
    entry[1] = this.valueEncoding.decode(entry[1])
    return entry
  }
}

class AbstractKeyIterator extends CommonIterator {
  decodeItem (key) {
    return this.keyEncoding.decode(key)
  }
}

If we instead had a single base class, let's say AbstractIterator, then sure, implementations would not have to implement common methods like _seek() three times over:

class ExampleIterator extends AbstractIterator {
  _seek () {
    // Implement once here
  }

  async _next () {
    return ['key', 'value']
  }
}

class ExampleKeyIterator extends ExampleIterator {
  // Override ExampleIterator._next()
  async _next () {
    return 'key'
  }
}

But the downside is: we'd then need to add options to the AbstractIterator constructor telling it what type of iterator it is. And decode accordingly (etc - because decoding was just one example).

Long story short, the current code (after #50) is fine.

Repository owner moved this from Backlog to Done in Level Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Changes that break backward compatibility
Projects
Archived in project
Development

No branches or pull requests

1 participant