Discussion on: Removing Comments

[Mihail Malo]

Would you say this comment is justified?

  buffer(length: number) {
    // has to come first so we don't allocate on error
    const start = this.fwd(length)
    const newBuf = Buffer.allocUnsafe(length)
    this.buf.copy(newBuf, 0, start, this.pointer)
    return newBuf
  }

It's there to prevent someone "optimizing" this method to the following:

  buffer(length: number) {
    const newBuf = Buffer.allocUnsafe(length)
    this.buf.copy(newBuf, 0, this.fwd(length), this.pointer)
    return newBuf
  }

Which offers no performance improvement, but if an error causes the bombardment of this method with yuuuuuge numbers, orders of magnitude larger than the underlying Buffer, it will cause lag, instead of just throwing the errors nicely.

For reference, this is what the .fwd method does:

  /**
   * Sets the buffer pointer, the address at which next parse will begin.
   * @throws {RangeError} if this will put pointer outside of `0..=len` range.
   * @param {number} addr What index to go to
   * @returns {number} the previous pointer, before setting
   */
  private goTo(addr: number): number {
    const { pointer, len } = this
    const newPointer = addr
    if (newPointer > len) throw new RangeError("Index past Buffer end")
    if (newPointer < 0) throw new RangeError("Index before Buffer beginning")
    this.pointer = newPointer
    return pointer
  }
  /**
   * Increments the buffer pointer, the address at which next parse will begin.
   * @throws {RangeError} if this will put pointer outside of `0..=len` range.
   * @param {number} dist How far to wind the buffer pointer. Can be negative to rewind and overwrite.
   * @returns {number} the previous pointer, before winding
   */
  fwd(dist: number): number {
    return this.goTo(this.pointer + dist)
  }

The JSDoc might seem excessive, but it is useful because tooling displays it in tooltips wherever it is referenced.

[Jason McCreary]

I don't consider "doc blocks" to be comments. So I wouldn't consider those "excessive".

As far as the inline comment regarding the optimization, these are definitely the gray area where a comment does relay non-obvious implementation details. This could potentially be mitigated with a name such as optimizedBuffer or by pushing the buffer optimizations lower to a createFastBuffer.

[Mihail Malo]

I didn't mean the JSDoc as part of the question, it was just a literal copy paste to show what fwd does.
I don't see how it could be extracted ergonomically, since the invariant is that this.fwd(length) returning as the return value and this.pointer the two arguments to copy runs before allocate

In most cases inlining them is perfectly acceptable. These are the cases around buffer:

  doublele() {
    return this.buf.readDoubleLE(this.fwd(8))
  }
  slice(length: number) {
    return this.buf.slice(this.fwd(length), this.pointer)
  }
  buffer(/**/){/**/}
  string(length: number, encoding: Encoding) {
    return this.buf.toString(encoding, this.fwd(length), this.pointer)
  }

But yeah, hopefully this class will be single-responsibility enough that no one will come edit it at all. And the comment is definitely staying.

One more good thing about deleting comments is that the few that do remain are seen as that much more important.