Use canonical maps/arrays instead of new Function

[dcousens]

Resolves #3

Instead of using new Function, this pull request replace the two functions with an array lookup (generated at runtime) and a typical object lookup (static).

I additionally noticed that the previous optionStringToNumber actually returned a string if the string was found (e.g '11' instead of 11), otherwise it returned a number from the parseInt.
I assumed that was a bug, but happy to switch the values in OPTIONS_BY_NUMBER if not.

node -v

v17.3.0

Results

// number -> string
optionNumberToString:       2.444s
optionNumberToStringNew:    1.447s

// string -> number
optionStringToNumber:       5.268s
optionStringToNumberNew:    2.191s

Method

Benched with the following code.
You could use benchmark as an improvement.
The benchmark runs are shown sequentially, but if you want to check for less hot-path results, you can omit them or change the order.
The results appear to be consistent.

// copied verbatim
const numMap = {
  1: 'If-Match',
  3: 'Uri-Host',
  4: 'ETag',
  5: 'If-None-Match',
  6: 'Observe',
  7: 'Uri-Port',
  8: 'Location-Path',
  9: 'OSCORE',
  11: 'Uri-Path',
  12: 'Content-Format',
  14: 'Max-Age',
  15: 'Uri-Query',
  16: 'Hop-Limit',
  17: 'Accept',
  19: 'Q-Block1',
  20: 'Location-Query',
  23: 'Block2',
  27: 'Block1',
  28: 'Size2',
  31: 'Q-Block2',
  35: 'Proxy-Uri',
  39: 'Proxy-Scheme',
  60: 'Size1',
  258: 'No-Response',
  2049: 'OCF-Accept-Content-Format-Version',
  2053: 'OCF-Content-Format-Version'
}

// copied verbatim
const optionNumberToString = (function genOptionParser () {
  let code = Object.keys(numMap).reduce(function (acc, key) {
    acc += 'case ' + key + ':\n'
    acc += '  return \'' + numMap[key] + '\'\n'

    return acc
  }, 'switch(number) {\n')

  code += 'default:\n'
  code += 'return \'\' + number'
  code += '}\n'

  return new Function('number', code) /* eslint-disable-line no-new-func */
})()

// copied verbatim
const optionStringToNumber = (function genOptionParser () {
  let code = Object.keys(numMap).reduce(function (acc, key) {
    acc += 'case \'' + numMap[key] + '\':\n'
    acc += '  return \'' + key + '\'\n'

    return acc
  }, 'switch(string) {\n')

  code += 'default:\n'
  code += 'return parseInt(string)'
  code += '}\n'

  return new Function('string', code) /* eslint-disable-line no-new-func */
})()

///////////////////////////////////////////////////////////////////////////

function generate () {
  const keys = Object.keys(numMap)
  const rows = []
  for (let i = 0; i < 1e4; ++i) {
    const number = parseInt(keys[Math.floor(Math.random() * keys.length)])
    const string = numMap[number]
    rows.push({ number, string })
  }
  return rows
}
// WARNING: comment this out once
//  require('fs').writeFileSync('bench.json', JSON.stringify(generate()))

// new code from here down
const rows = require('./bench.json')
const OPTIONS_BY_NAME = {
  "If-Match": 1,
  "Uri-Host": 3,
  "ETag": 4,
  "If-None-Match": 5,
  "Observe": 6,
  "Uri-Port": 7,
  "Location-Path": 8,
  "OSCORE": 9,
  "Uri-Path": 11,
  "Content-Format": 12,
  "Max-Age": 14,
  "Uri-Query": 15,
  "Hop-Limit": 16,
  "Accept": 17,
  "Q-Block1": 19,
  "Location-Query": 20,
  "Block2": 23,
  "Block1": 27,
  "Size2": 28,
  "Q-Block2": 31,
  "Proxy-Uri": 35,
  "Proxy-Scheme": 39,
  "Size1": 60,
  "No-Response": 258,
  "OCF-Accept-Content-Format-Version": 2049,
  "OCF-Content-Format-Version": 2053
}

const OPTIONS_BY_NUMBER = new Array(2054)
for (const key in OPTIONS_BY_NAME) {
  const number = OPTIONS_BY_NAME[key]
  OPTIONS_BY_NUMBER[number] = key
}

function optionNumberToStringNew (number) {
  const string = OPTIONS_BY_NUMBER[number]
  if (string) return string
  return '' + number
}

function optionStringToNumberNew (string) {
  const number = OPTIONS_BY_NAME[string]
  if (number) return number
  return parseInt(string)
}

function bench (what, f) {
  console.time(what)
  for (let i = 0; i < 1e4; ++i) {
    for (const row of rows) {
      f(row)
    }
  }
  console.timeEnd(what)
}

bench('optionNumberToString', ({ number, string }) => {
  if (optionNumberToString(number) != string) throw new Error('bad string')
})

bench('optionNumberToStringNew', ({ number, string }) => {
  if (optionNumberToStringNew(number) != string) throw new Error('bad string')
})

bench('optionStringToNumber', ({ number, string }) => {
  if (optionStringToNumber(string) != number) throw new Error('bad number')
})

bench('optionStringToNumberNew', ({ number, string }) => {
  if (optionStringToNumberNew(string) != number) throw new Error('bad number')
})

Disclaimer: I don't actually think this function needs to be fast, but, new Function was a red flag to me.

[dcousens]

If 0 is intended to be supported [somehow], you could use string !== undefined.

Little to no difference to the benchmark.

optionNumberToStringNew: 1.537s

[JKRhb]

Thank you for your contribution, @dcousens! :) This is a great improvement over the old version. As to this particular check: As 0 is a reserved number, I think it is safe to keep it as is :)

[dcousens]

Sorry, this comment should have been on optionStringToNumber, but the implications of the comment remain the same. (number !== undefined)

Little to no difference to the benchmark.

optionStringToNumberNew: 2.250s

[JKRhb]

LGTM :) @mcollina: Could you release a new version with these changes?

[mcollina]

lgtm