1
\$\begingroup\$

I'm currently inserting chars into a string with the given position.

Unfortunately at this stage I was unable to create a regular expression that would allow me to achieve the following.
Nevertheless, while the code is working, I'm not particularly happy with it, any advice?

Thanks.

  • Input Text - A12123456789012

  • Output Text - A-12/1234/5678-901

    const insertAt = (str, sub, pos) => `${str.slice(0, pos)}${sub}${str.slice(pos)}`;
    
    function interceptor(value){
    
        if (!value) return '';
    
        let a = null;
        let b = null;
        let c = null;
        let d = null;
    
        if (value.length > 0){ a = insertAt(value, '-', 1);}
        if (value.length > 3){ b = insertAt(a, '/', 4);}
        if (value.length > 8){ c = insertAt(b, '/', 9);}
        if (value.length > 13){ d = insertAt(c, '-', 14);}
    
        if (d !== null) return d;
        if (c !== null) return c;
        if (b !== null) return b;
        if (a !== null) return a;
    }
    
\$\endgroup\$
1
  • \$\begingroup\$ Thank you everyone for your input, I have learnt a lot :) \$\endgroup\$ Commented Feb 17, 2022 at 22:59

4 Answers 4

4
\$\begingroup\$

Your variable names of a, b, c, and d aren't particularly descriptive. We can refactor your code to promote scalability (if you want to add more "insertions" in the future), and at the same time, do away with these variables entirely.

Instead, let's store your insertions in an array of pairs:

const insertions = [
  ['-', 1],
  ['/', 4],
  ['/', 9],
  ['-', 14],
];

Now, in your interceptor function, you can iterate over this collection of insertions, and apply each one to the value after you've checked the length against the position:

function interceptor(value){
    if (!value) return '';
    
    let transformed = value;
    
    for (const [char, pos] of insertions) {
        if (transformed.length <= pos) break;
        transformed = insertAt(transformed, char, pos);
    }

    return transformed;
}

I've created the transformed variable in favour of modifying the function's parameter because this is generally frowned upon.

This code makes the assumption that the insertions array is sorted by the position (ascending). This assumption lets us exit the loop early (if (transformed.length <= pos) break;) and skip unnecessary checks.

If you can't guarantee the sort order of insertions, you can either sort it before the loop, or reshuffle the loop's logic to not break:

function interceptor(value){
    if (!value) return '';
    
    let transformed = value;
    
    for (const [char, pos] of insertions) {
        if (transformed.length > pos) transformed = insertAt(transformed, char, pos);
    }

    return transformed;
}
\$\endgroup\$
2
  • \$\begingroup\$ Thanks Luke, at the time of posting the question, I did think about loop each char ect, however I like your idea of a keyvaluepair like array. \$\endgroup\$ Commented Feb 17, 2022 at 23:00
  • \$\begingroup\$ The order of insertions is important, because the index 4 for / takes into account that a - was inserted at 1. Without inserting at 1 first, then / should be inserted at 3 (which only becomes 4 after inserting '-' at 1). The second reshuffled loop will not work properly if the insertions are for example reverse sorted. \$\endgroup\$
    – zeluisping
    Commented Mar 2, 2022 at 20:11
2
\$\begingroup\$

The big thing that sticks out to me about your solution is that the code doesn't express intent of what you're going for. It works, but it doesn't explain what it's trying to achieve.

I liked your idea of using a regex, so I parsed the input with a regular expression and then used the matched groups while formatting the output. To separate those concepts, I moved them into separate functions (so the code shows that we parse and then format).

Then, I wanted a way to express the core idea of the code: we put a bunch of parts together with different joiners.

The second idea in the code is that if a part is empty, then the joiner before it should be omitted. Since that seemed like a less important part of the code, I hid that idea in the withPrefix function. That way, the main code expresses the overall format of the output.

function withPrefix(prefix, value) {
  if (!value) return;
  return prefix + value;
}

function skipEmptyValues(value) {
  return !!value;
}

function parseInput(value) {
  return value.match(
      /(?<first>.)(?<second>.{1,2}|)(?<third>.{1,4}|)(?<fourth>.{1,4}|)(?<fifth>.{1,4}|)/
    ).groups;
}

function formatInput({ first, second, third, fourth, fifth }) {
    return [
      first,
      withPrefix('-', second),
      withPrefix('/', third),
      withPrefix('/', fourth),
      withPrefix('-', fifth)
    ].filter(skipEmptyValues).join('');
}

function interceptor(value){
    const parts = parseInput(value);
    return formatInput(parts);
}

// The rest of this is for testing
const examples = [
  [ 'A12123456789012', 'A-12/1234/5678-9012' ],
  [ 'A12123456789', 'A-12/1234/5678-9' ],
  [ 'A1212345678', 'A-12/1234/5678' ],
  [ 'A1212345', 'A-12/1234/5' ],
  [ 'A121234', 'A-12/1234' ],
  [ 'A121', 'A-12/1' ],
  [ 'A12', 'A-12' ],
  [ 'A1', 'A-1' ],
  [ 'A', 'A' ],
]
const failures = [];
examples.forEach(example => {
  const [input, expectedOutput] = example;
  const actualOutput = interceptor(input);
  const doesMatch = expectedOutput === actualOutput
  if (!doesMatch) {
    console.log(`Expected interceptor('${input}') to be ${expectedOutput} but was ${actualOutput}`);
    failures.push(example);
  }
});
if (failures.length) {
  console.log(`${failures.length} failed examples`);
} else {
  console.log(`All ${examples.length} examples pass`);
}

If you wanted, you could take the "expressive code" idea one step further and build a "fluent interface" for defining the format. This might be worth the overhead if you're going to be defining many different formats, or if you expect this format to change often. If this is something that changes rarely, then I wouldn't bother with the following. Nonetheless, for the sake of completeness:

class FormattingStringBuilder {
  constructor(value, nextJoiner) {
    this.value = value;
    this.nextJoiner = nextJoiner;
  }
  
  join(joiner) {
    return new FormattingStringBuilder(this.value, joiner);
  }
  
  to(value) {
    if (!value) {
      return new FormattingStringBuilder(this.value, null);
    }
    return new FormattingStringBuilder(this.value + this.nextJoiner + value, null);
  }
  
  toString() {
    return this.value;
  }
}

function parseInput(value) {
  return value.match(
      /(?<first>.)(?<second>.{1,2}|)(?<third>.{1,4}|)(?<fourth>.{1,4}|)(?<fifth>.{1,4}|)/
    ).groups;
}

function formatInput({ first, second, third, fourth, fifth }) {
  return new FormattingStringBuilder(first)
    .join('-').to(second)
    .join('/').to(third)
    .join('/').to(fourth)
    .join('-').to(fifth)
    .toString();
}

function interceptor(value){
    const parts = parseInput(value);
    return formatInput(parts);
}

// The rest of this is for testing
const examples = [
  [ 'A12123456789012', 'A-12/1234/5678-9012' ],
  [ 'A12123456789', 'A-12/1234/5678-9' ],
  [ 'A1212345678', 'A-12/1234/5678' ],
  [ 'A1212345', 'A-12/1234/5' ],
  [ 'A121234', 'A-12/1234' ],
  [ 'A121', 'A-12/1' ],
  [ 'A12', 'A-12' ],
  [ 'A1', 'A-1' ],
  [ 'A', 'A' ],
]
const failures = [];
examples.forEach(example => {
  const [input, expectedOutput] = example;
  const actualOutput = interceptor(input);
  const doesMatch = expectedOutput === actualOutput
  if (!doesMatch) {
    console.log(`Expected interceptor('${input}') to be ${expectedOutput} but was ${actualOutput}`);
    failures.push(example);
  }
});
if (failures.length) {
  console.log(`${failures.length} failed examples`);
} else {
  console.log(`All ${examples.length} examples pass`);
}

For me, the tradeoff here is about how often this format will change, how important it is to the business, or how many different formats you will be defining. If it's changed frequently, business critical, or applied in many places, then the additional cost of the complicated fluent interface is offset by the clarity of the important/repeated code. But if it's rarely changed, not important to the business, and a one-of-a-kind piece of code, then there's no payoff for the complicated formatter code.

Testing

In any case, I do suggest you write automated tests for the different string length breakpoints, since both the solutions that I've written behave differently than yours when there is a single character past the breakpoint. If the behaviour you currently have is what you want, then I could change these examples to account for that. Your code makes A121 into A-121 but I'm guessing you intended for it to be A-12/1

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

The title of the question infers that you are interested in the function insert at. However the body of your question implies you are focused on the transformation of a id string. I will review both.

Insert

Your insertAt function does not follow JS conventions in regard to string manipulation.

There is no default insert position, for string if a position parameter is missing it is generally assumed to be the end of the string.

If you change the function to

const insertAt = (str, insert, pos = str.length) => `${str.slice(0, pos)}${sub}${str.slice(pos)}`;

You can also shorten (reducing code noise) to...

const insertAt = (str, insert, pos = str.length) => str.slice(0, pos) + sub + str.slice(pos);

However you don't need the function insertAt

Format string

You are not happy with the code, and I agree it is rather complex and hard to modify / maintain.

You can use a simple interleaver to do the task. The formatting is via an array of characters at positions to be inserted after.

You can then just map the string with cosponsoring format characters and join the resulting map to get the result.

const format = ["-", "", "/", "", "", "", "/", "", "", "", "-"];
const formStr = (str, form) => [...str].map((c, i) => c + (form[i] ?? "")).join("");

This is very flexible and thus reusable not requiring new logic if the format changes.

Example

With a slight change using undefined array inserts to make the format array more readable.

//             A   12   1234   5678   901
const format = ["-",,"/",,,,"/",,,,"-"];
const formStr = (str, form) => [...str].map((c, i) => c + (form[i] ?? "")).join("");


console.log(formStr("A1212345678901", format));
console.log(formStr("A121234567890", format));
console.log(formStr("A12123456789", format));
console.log(formStr("A1212345678", format));
console.log(formStr("A121234567", format));
console.log(formStr("A12123456", format));
console.log(formStr("A1212345", format));
console.log(formStr("A121234", format));
console.log(formStr("A12123", format));
console.log(formStr("A1212", format));
console.log(formStr("A121", format));
console.log(formStr("A12", format));
console.log(formStr("A1", format));
console.log(formStr("A", format));
console.log(formStr("", format));

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

Code should be correct, maintainable, robust, reasonably efficient, and, most important, readable.


In real-world code reviews, tests are an essential part of ensuring correctness, both now and in the future. Without adequate testing, your code will be rejected. You didn't post any tests.

I automated a test of your interceptor function using a simple and direct interceptorWant function. The test identified these anomalies:

length 1: want "A"; got "A-"
length 8: want "A-12/1234/5"; got "A-12/12345"
length 12: want "A-12/1234/5678-9"; got "A-12/1234/56789"
length 13: want "A-12/1234/5678-90"; got "A-12/1234/567890"

'use strict';

function interceptorWant(value) {
    if (!value) { value = ''; }
    let v = value.substring(0, 1);
    if (value.length <= 1) { return v; }
    v += '-' + value.substring(1, 3);
    if (value.length <= 3) { return v; }
    v += '/' + value.substring(3, 7);
    if (value.length <= 7) { return v; }
    v += '/' + value.substring(7, 11);
    if (value.length <= 11) { return v; }
    v += '-' + value.substring(11);
    return v;
}

const insertAt = (str, sub, pos) => `${str.slice(0, pos)}${sub}${str.slice(pos)}`;

function interceptor(value){
    if (!value) return '';
    let a = null;
    let b = null;
    let c = null;
    let d = null;
    if (value.length > 0){ a = insertAt(value, '-', 1);}
    if (value.length > 3){ b = insertAt(a, '/', 4);}
    if (value.length > 8){ c = insertAt(b, '/', 9);}
    if (value.length > 13){ d = insertAt(c, '-', 14);}
    if (d !== null) return d;
    if (c !== null) return c;
    if (b !== null) return b;
    if (a !== null) return a;
}

// format 'A12123456789012' to 'A-12/1234/5678-9012'
const value = 'A12123456789012';
for (let i = 0; i <= value.length; i++) {
    const got = interceptor(value.substring(0, i)); 
    const want = interceptorWant(value.substring(0, i)); 
    if (want !== got) {
        const err = `length ${i}: want "${want}"; got "${got}"`;
        console.log(err);
    }
}

If code needs to scale, then performance benchmarks are often part of a real-world code review. Using jsbench.me, I benchmarked your interceptor function and compared it to my interceptorWant function. For a string primitive argument, 'A12123456789012', interceptor was around 90% slower than interceptorWant. For a String Object argument, new String('A12123456789012'), interceptor was around 30% slower than interceptorWant.

Note: As I went to post this answer, I saw that @LukeCarr and @Blindman67 have posted solutions. For a string primitive argument, both solutions are around 90% slower than interceptorWant.

// https://jsbench.me/

// Setup:

'use strict';

// format 'A12123456789012' to 'A-12/1234/5678-9012'

const value = 'A12123456789012'; // primitive
//const value = new String('A12123456789012'); // object

// Test Case 1:

function interceptorWant(value) {
    if (!value) { value = ''; }
    let v = value.substring(0, 1);
    if (value.length <= 1) { return v; }
    v += '-' + value.substring(1, 3);
    if (value.length <= 3) { return v; }
    v += '/' + value.substring(3, 7);
    if (value.length <= 7) { return v; }
    v += '/' + value.substring(7, 11);
    if (value.length <= 11) { return v; }
    v += '-' + value.substring(11);
    return v;
}

interceptorWant(value);

// Test Case 2:

const insertAt = (str, sub, pos) => `${str.slice(0, pos)}${sub}${str.slice(pos)}`;

function interceptor(value){
    if (!value) return '';
    let a = null;
    let b = null;
    let c = null;
    let d = null;
    if (value.length > 0){ a = insertAt(value, '-', 1);}
    if (value.length > 3){ b = insertAt(a, '/', 4);}
    if (value.length > 8){ c = insertAt(b, '/', 9);}
    if (value.length > 13){ d = insertAt(c, '-', 14);}
    if (d !== null) return d;
    if (c !== null) return c;
    if (b !== null) return b;
    if (a !== null) return a;
}

interceptor(value);
\$\endgroup\$
0

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.