I've got this little module that returns basic user data required for a web app that's extracted from a large number of columns per row. I was asked to replace the small, column specific select statement I'd prepared with a stored procedure that returns the entire row. This moves column pruning into the server.

I've modified my original function to work as expected with the code below, but it feels like I could be doing better with Array.find((e) => {})

sqlRequest.on('row', function(columns) {
  response = {};
  const keys = ['id', 'fn', 'ln', 'ctr'];

  columns.forEach(function (c) {
    let key = String(c.metadata.colName).toLowerCase();
    if (keys.find((e) => { return key === e})) {
      let value = String(c.value).charAt(0).toUpperCase() + String(c.value).slice(1).toLowerCase();
      response[key] = value;

1 Answer 1

  • response is an implicit global, which is a no-no. Now, I imagine it is declared in a wider scope already, but it's still disconcerting to see a function have such crass side-effects. It just overwrites the entire response variable (but if other functions have their own references to the previous value, they won't know about the overwriting).
    I'd use a callback or a promise to "return" a fresh object, and let some other piece of code deal with whether to redefine response.

  • Your find call can be reduced to keys.find(e => key === e) - you don't need braces or return for one-line statements, or parentheses around a single parameter. But the more conventional way to do things is using indexOf. I.e.

    if(keys.indexOf(e) !== -1)
  • \$\begingroup\$ To address the last point first, this wasn't moved to JS, it was built from nothing in JS. I'm off the project soon and am working to transition and refactor with the new maintainer who will integrate it into a much larger code base. They asked that I use stored procedures, I agreed to it since I'm not going to be maintaining it. My original code was much less complex. \$\endgroup\$ Dec 28, 2016 at 20:53
  • 1
    \$\begingroup\$ On response, you're exactly right that it's scoped higher, and you have very valid points. I'm already in a callback (row event), and must be able to access the response in another callback triggered by the doneProc event my DB driver emits. That bit is on my radar to be replaced with something safer. \$\endgroup\$ Dec 28, 2016 at 21:00
  • 1
    \$\begingroup\$ @gelliott181 So you're using some existing stored procedures? In that case it makes more sense. I was reading it like you'd created some stored procedures that returned way too much - in which case I'd say change the procedures rather than handle it in JS. \$\endgroup\$
    – Flambino
    Dec 28, 2016 at 21:00
  • \$\begingroup\$ @gelliott181 No prob (removed the question from my answer btw) \$\endgroup\$
    – Flambino
    Dec 28, 2016 at 21:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.