4
\$\begingroup\$

I wrote this code and I just can't stomach it. The idea is that there is a schema, and the goal is to find the entry with autoIncrement set to true. If it's not found, the result should be idProperty.

const idProperty = 'id'
const schema = {
  id: { type: 'number' },
  code: { type: 'number', autoIncrement: true },
  email: { type: 'string', trim: 512 }
}

// Find the auto increment field
let autoIncrementSchemaFieldName = null
for (const name in schema) {
  const entry = schema[name]
  if (entry.autoIncrement) {
    autoIncrementSchemaFieldName = name
    break
  }
}
autoIncrementSchemaFieldName = autoIncrementSchemaFieldName || idProperty

console.log('RESULT:', autoIncrementSchemaFieldName)

Is this really the best way to do it?

(To test whether the idProperty fallback works, just rename autoIncrement into autoIncrement2 and run it again)

\$\endgroup\$
1
\$\begingroup\$

When you want to find a particular matching value in a data structure, using Array.prototype.find is usually the right way to go. It's complicated by the fact that the condition involves the value of the object, but you want the key of the object to be found, so to iterate over the keys and values at once, use Object.entries. It'll give you an array of [key, value] subarrays, and you can return the first item in the resulting subarray to get the key, if anything is found:

const idProperty = 'id';
const schema = {
  id: { type: 'number' },
  code: { type: 'number', autoIncrement: true },
  email: { type: 'string', trim: 512 }
};

const autoIncrementSchemaFieldName = Object.entries(schema)
  .find(([, value]) => value.autoIncrement)
  ?.[0] // Extract the key from the found entry
  ?? idProperty; // Otherwise, if no entry was found, use idProperty

console.log('RESULT:', autoIncrementSchemaFieldName)

That's what I'd prefer, but there are a few compatibility caveats:

  • Object.entries was introduced in 2017. It's not new, but it's not old either
  • I'm using optional chaining and the nullish coalescing operator for conciseness, which is only supported in environments built in 2020 and later

As always, the usual professional solution to compatibility problems is to use Babel to transpile syntax down to ES5 or ES6, and use polyfills to add new built-in objects and methods.

Without ES2020, it'd look like:

const idProperty = 'id';
const schema = {
  id: { type: 'number' },
  code: { type: 'number', autoIncrement: true },
  email: { type: 'string', trim: 512 }
};

const entry = Object.entries(schema)
  .find(([, value]) => value.autoIncrement);
const autoIncrementSchemaFieldName = entry
  ? entry[0]
  : idProperty;

console.log('RESULT:', autoIncrementSchemaFieldName)

\$\endgroup\$
3
  • 1
    \$\begingroup\$ OMG is this even Javascript? :D (I am talking out of my own ignorance!) Nice pearls here, having a play -- and thank you \$\endgroup\$
    – Merc
    Oct 27 '20 at 1:18
  • \$\begingroup\$ What does this do? ?.[0] -- I get the question mark, marking the chaining optional, but then that .[0]? \$\endgroup\$
    – Merc
    Oct 27 '20 at 1:35
  • \$\begingroup\$ That's what optional chaining looks like with bracket notation. Dot notation: obj?.propName. Bracket notation: obj?.[propName] stackoverflow.com/q/59623674 The extra dot is needed to distinguish from the conditional operator when the token is first encountered, I think. \$\endgroup\$ Oct 27 '20 at 1:38
1
\$\begingroup\$

Functions

Always write functions. Even if its just an experiment or example code, writing the code as a function provides you with a better representation of the problem at hand.

As a function the code is easier to read and understand defining the code as a verb, can often provide a simpler solution, keeps the namespace (scope) clean, and is portable

for...of rather than for...in

Do not use for...in as it requires guard functionality to ensure you are not traversing outside the objects prototype and own properties.

The guard typically used with for...in is

 for (const name in object) {
     if (object.hasOwnProperty(name)) {

Rather use for...of. With objects you can access keys, properties or both with Object.keys(, Object.values( or Object.entries(

Noise

Noise is code that does nothing but interfere with our ability to read and understand the code. There are many forms of code noise, in your code there are three types of noise.

Redundant and superfluous noise

You have let autoIncrementSchemaFieldName = null When a variable is declared it is automatically set to undefined there is no need to set the value to null

Comments

Comments have there place but comments are almost always because the code is not clear.

You have the comment // Find the auto increment field a clear indication that the code should be a function. If one assumes we know what the code should do then the comment is only noise.

Naming noise

Variable names must be easy to identify. Making names too long does not make code more readable, the reverse is true in a variety of ways.

The following code has a nasty bug that in standard context will not throw an error

 let autoIncrementSchemaFieldName;
 // ... code may assign name a value ...
 autoIncrementSchemaFie1dName = name ?? idProperty;

Due to how we read names typos can be lost in the noise. There is a direct relationship between the length of a name, the chance of introducing an error, and our inability to spot the error.

General rules of thumb for variable name lengths.

  • No more than 20 characters
  • Use common abbreviation. Do not use ad-hoc or made up abbreviations.
  • Infer via context (scope). The smaller the scope the shorter a name can safely be.

Semicolons ;

Yes JavaScript does not require the source code to have semicolons, however to parse JavaScript they are required and are thus inserted automatically before parsing.

Automatic semicolon insertion can be complicated and unless you can, off the top of your head, name all situations that can result in a semicolon not being inserted at the end of a line you should always use semicolons when there is a possibility of a ambiguous end of line.

Strict mode

If your code is not a module you should ALWAYS start the code with the directive "use strict";

Reasons to use Strict mode.

  • Strict mode will throw errors that are ignored in sloppy mode. Sloppy mode bugs can remain dormant until well after release.

  • Strict mode code always executes quicker (The optimizer has more optimization options)

Bullet proof

Bullet proof code is code that never exhibits undefined behavior.

Your code is not bullet proof as it does not come with a list of expected behaviors for unexpected data

A list of reasons your code is not bullet proof.

  1. What if schema does not contain an autoIncrement nor a property id?
  2. What if autoIncrement is not a boolean?
  3. What if schema contains non objects?
  4. What if schema is not an object?

And maybe outside the functions responsibility, but one would expect it to be.

  1. What if the object with autoIncrement is not of type "number"?
  2. What if the default object is not type "number" and there is no autoIncrement object?

Rewrite A

The following is a rewrite that matches the behavior of your code.

Note that the code has been moved into a function

"use strict";

function findIncName(schema, defaultName) {   
    for (const [name, entry] of Object.entries(schema)) {
        if (entry.autoIncrement) { return name }
    }
    return defaultName;
}

console.log(findIncName({id: {}, code: {autoIncrement: true}}, "id"));
console.log(findIncName({id: {}, code: {autoIncrement: false}}, "id"));

Rewrite B

The following addresses some of the problems with the code.

Returns undefined when a name can not be defined according to the rules...

  • Name must be of an existing property of schema,
  • Be of type "number"
  • Have a boolean property autoIncrement or be the default name

"use strict";

function findIncName(schema, defaultName) {   
    if (schema && typeof schema === "object") {
        for (const [name, entry] of Object.entries(schema)) {
            if (entry?.autoIncrement === true && entry.type === "number") { return name }
        }
        return schema[defaultName]?.type === "number" ? defaultName : undefined;
    }
}

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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