-3

I was working on this practice problem, and solved it, but I want a more elegant way of writing this code:

// Usually when you buy something, you're asked whether your credit card number, phone number or answer to your most secret question is still correct. However, since someone could look over your shoulder, you don't want that shown on your screen. Instead, we mask it.

// Your task is to write a function maskify, which changes all but the last four characters into '#'.

const maskify = (cc) => {
    let ccArray = Array.from(cc);
    let length = cc.length;
    let lastFour = cc.slice(-4);
    let newArray = [];

    if (length <= 4) {
        return cc;

    } else if (length > 4) {
        let index = length - 4;
        ccArray.splice(index, 4);
        ccArray.forEach(n => {
            newArray.push('#');
            return newArray;
        });
        return newArray.concat(lastFour).join('');
    }
}

console.log(maskify('4556364607935616'));
// ############5616

console.log(maskify('1'));
// 1

console.log(maskify('11111'));
// #1111
Mulan
  • 129,518
  • 31
  • 228
  • 259
Mariam Qasim
  • 1
  • 1
  • to start with, you don't need the `else if`. if it doesn't meet the first condition then the length will be larger than 4 – andrewgi Sep 11 '18 at 17:45
  • I retracted the close vote since this is not exactly a duplicate but a question on whether you can use ternary or not. Yes you can. Check the answer here to see how: https://stackoverflow.com/questions/6259982/how-do-you-use-the-conditional-operator-in-javascript – AndrewL64 Sep 11 '18 at 17:47
  • One thing that IMO goes a long way to making a function more elegant is documentation around it. So document the parameters, what are the valid values of the parameter, what are the errors that can occur, and provide some test case examples in the comments that demonstrate what this function does – ControlAltDel Sep 11 '18 at 17:48
  • Yes, there is a more elegant way to do this that also involves a ternary operator, but really you should try to change your approach first - do something that does not use `splice` or `forEach`. – Bergi Sep 11 '18 at 17:57

4 Answers4

2

There are many different approaches:

function maskify(cc) {
    return "#".repeat(Math.max(0, cc.length-4)) + cc.slice(-4);
}


function maskify(cc) {
    return Array.from(cc, (char, index) =>
        index < cc.length - 4 ? "#" : char
    ).join('')
}


function maskify(cc) {
    return cc.replace(/.(?=.{4})/g, "#");
}
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
0

This might be among the easiest ways to achieve that goal:

const maskify = (cc) => {
   return ("################################"+cc.slice(-4)).slice(-cc.length);
}

console.log(maskify('4556364607935616'));
console.log(maskify('1'));
console.log(maskify('11111'));

Just make sure the "################################" is long enough to cover all your use-cases.

To make it dynamic and work for any length of a string, it gets only slightly more complicated:

const maskify = (cc) => {
   return ([...cc].map(x=>'#').join('')+cc.slice(-4)).slice(-cc.length);
}

console.log(maskify('4556364607935616'));
console.log(maskify('1'));
console.log(maskify('11111'));

You can also use a regular expression:

const maskify = (cc) => {
   return (cc.replace(/./g,'#')+cc.slice(-4)).slice(-cc.length);
}

console.log(maskify('4556364607935616'));
console.log(maskify('1'));
console.log(maskify('11111'));
connexo
  • 53,704
  • 14
  • 91
  • 128
-1

Generally speaking, I think your code is fine. Since "more elegant" is a little nebulous, I'll concern myself with 1) time complexity and 2) memory complexity

1) Time Complexity

Generally, you're fine here. No extra loops, and no intensive operations. There is a minor improvement to remove the final .join since that will have to loop over the final array.

2) Memory complexity

Some room for improvement here. Currently, the function creates a bunch of extra arrays. This likely won't be an issue for something as small as a credit card number, but could bite you if you applied a similar logic in a different situation.

Annotated original:

const maskify = (cc) => {
    let ccArray = Array.from(cc); // Array 1
    let length = cc.length;
    let lastFour = cc.slice(-4); // Array 2
    let newArray = []; // Array 3

    if (length <= 4) {
        return cc;

    } else if (length > 4) {
        let index = length - 4;
        ccArray.splice(index, 4);
        // Loop 1
        ccArray.forEach(n => {
            newArray.push('#');
            return newArray;
        });
        // Loop 2
        return newArray.concat(lastFour).join(''); // Array 4
    }
}

Improved version:

const maskify = (cc) => {
    let ccArray = Array.from(cc); // Array 1
    let length = cc.length;
    // Loop 1
    let resultString = ccArray.reduce((result, original, index) => {
        result += (index >= length - 4 ? original : '#');
    }, "");
    return resultString;
}

Array Reduce Documentation

As others have noted, there are many other ways to write this method. Since the question was specifically about how the existing code could be better, I tried to focus this answer on that.

Vlad274
  • 6,514
  • 2
  • 32
  • 44
  • "*you're fine here. No extra loops*" - I'm counting 5 looping constructs in the OPs code. That seems a bit many. – Bergi Sep 11 '18 at 18:08
  • @Bergi Are you including `slice`, `splice`, and `concat` in that number? I omitted those since they're a consistent number of executions regardless of the input size. – Vlad274 Sep 11 '18 at 18:18
  • The `slice` and `concat` operations have to copy a dynamic amount of data to the new value, so yes. The others are `Array.from`, `forEach` and `join`. Maybe the JS engine can optimise some of them as they are native data types, but I doubt it for anything but the string `slice`. – Bergi Sep 11 '18 at 18:26
-1

Here's a way you can do it using destructuring assignment and single ternary expression

const maskify = ([ a = '', b = '', c = '', d = '', ...rest ]) =>
  rest.length === 0
    ? a + b + c + d
    : '#' + maskify ([ b, c, d, ...rest ])

console.log(maskify('4556364607935616'));
// ############5616

console.log(maskify('1'));
// 1

console.log(maskify('11111'));
// #1111

Destructuring creates intermediate values which can be avoided by using a secondary parameter, i, which represents the index of the current computation

const maskify = (str, i = 0) =>
  4 + i >= str.length
    ? str.substr (i)
    : '#' + maskify (str, i + 1)

console.log(maskify('4556364607935616'));
// ############5616

console.log(maskify('1'));
// 1

console.log(maskify('11111'));
// #1111

Now it's easy to make 4 an argument to our function as well

const maskify = (str, n = 4, i = 0) =>
  n + i >= str.length
    ? str.substr (i)
    : '#' + maskify (str, n, i + 1)

console.log(maskify('4556364607935616'));
// ############5616

console.log(maskify('4556364607935616', 6));
// ##########935616

console.log(maskify('1'));
// 1

console.log(maskify('11111', 10));
// 11111
Mulan
  • 129,518
  • 31
  • 228
  • 259
  • This introduces the aspect **3) code complexity** ;) – connexo Sep 11 '18 at 18:03
  • Fancy! I don't like however that you cannot easily change this to replace all but the, say, 10 last characters – Bergi Sep 11 '18 at 18:06
  • @connexo, some see long strands of `map`, `join` and multiple `slice` as complex. Does we need to remark on the complexity of the regular expression engine too? – Mulan Sep 11 '18 at 18:32
  • @Bergi thanks! I added a length-configurable program as well :D – Mulan Sep 11 '18 at 18:33