Function to compare phone numbers

Question

I got rejected in tech screen yesterday. I was asked to write a JavaScript function and send it over email. I have also added hiring manager's response at the bottom.

Could you please write a JavaScript function to compare two strings that look like phone numbers and indicate whether the digits in both strings match? (To be clear, while the input looks like phone numbers, this question doesn't care how phones/telephony/dialing works.) Your code should compare only the digits of the input, ignore any letters or punctuation, and ignore the fact that country/area codes can be optional. As an example, "(123) 456-7890" and "123.456.7890" match because both have the digits "1234567890" in that order, while "1-123-456-7890" and "123-456-7890" do not match (even though they'll probably reach the same person if dialed in the US).

I wasn't sure if regex would be an acceptable solution so I implemented it two ways. Please, let me know what I could have done better.

"use strict"

var CompareNumbers = function () {

    //#region private variables
    var _phoneNumber1;
    var _phoneNumber2;
    var _minPhoneLength = 1;
    var _validateResult;
    var _notValidMessage = "Not a valid Phone Number";
    var _successMessage = "Match";
    var _failureMessage = "Not a Match";

    var _inputArray1;
    var _inputArray2;
    var _numArray1;
    var _numArray2;
    //#endregion

    //#region private functions
    //function using regex to remove spaces & braces etc
    var _compNumbersUsingRegex = function (input1, input2) {
        if (_validateInputs(input1, input2)) {
            _phoneNumber1 = input1.replace(/[^0-9]+/g, '');
            _phoneNumber2 = input2.replace(/[^0-9]+/g, '');
            if (_validateInputs(_phoneNumber1, _phoneNumber2)) {
                if (_phoneNumber1 == _phoneNumber2) {
                    return _successMessage;
                }
                else {
                    return _failureMessage;
                }
            }
            return _notValidMessage;
        }
        else {
            return _notValidMessage;
        }

    }

    //function using arrays for comparison
    var _compNumbersUsingArray = function (input1, input2) {

        if (_validateInputs(input1, input2)) {
            _inputArray1 = new Array();
            _inputArray2 = new Array();

            _inputArray1 = input1.split('');
            _numArray1 = new Array();
            for (var i = 0; i < _inputArray1.length; i++) {

                if (!isNaN(_inputArray1[i]) && _inputArray1[i] != ' ') {
                    _numArray1.push(_inputArray1[i]);
                }
            }

            _inputArray2 = input2.split('');
            _numArray2 = new Array();
            for (var i = 0; i < _inputArray2.length; i++) {
                if (!isNaN(_inputArray2[i]) && _inputArray2[i] != ' ') {
                    _numArray2.push(_inputArray2[i]);
                }
            }
            if (_validateInputs()) {
                return _numArray1.toString() == _numArray2.toString() ? _successMessage : _failureMessage;
            }
            else {
                return _notValidMessage;
            }
        }
        else {
            return _notValidMessage;
        }
    }

    //Helper funciton to validate if inputs exceed a minimum number declared in variable section
    function _validateInputs(validate1, validate2) {
        if (validate1.length < _minPhoneLength || validate2.length < _minPhoneLength) {
            return false;
        }
        else {
            return true;
        }
    }
    //#endregion

    //#region public members
    var publics = {
        comparePhoneNumbers1: _compNumbersUsingArray,
        comparePhoneNumbers2: _compNumbersUsingRegex
    };
    return publics;
    //#endregion 
}();

The response that I got back from the hiring manager was:

Thanks! I've looked over your code and think you captured the essence of the problem correctly. I like that you included both approaches - I prefer the _compNumbersUsingRegex implementation which is basically the one I like to see (regex+compare). That said, your resume shows you've been working with JavaScript in your current role, but some of the patterns I see in your code aren't quite in line with what I look for from senior developers on the team, so this role might not be the perfect match for you.

Answer 1

Regions

Regions seem to be a Microsoft IDE specific feature. Most people will configure their IDEs/text editors to fold/collapse without pointless comments that add noise.

Meeting the requirements

The instructions only ask you to compare two strings, but you added a second function that compares arrays. Not meeting the requirements is bad, but overshooting the requirements is also bad (twice as much stuff to hold in your brain, twice as many bugs to squish).

Terrible API/variable names

Variable names are meant to be self-documentating. Failing that, comments will do the job.

_compareNumbersUsingArray doesn't at all convey the idea that the parameters are meant to be arrays, considering that _compNumbersUsingRegex takes inputs as strings but uses regex as a comparison method. In other words, be consistent.

The user will have to call your functions like this:

CompareNumbers.comparePhoneNumbers1('123', '123');

How are they supposed to know what "comparePhoneNumbers1" does without consulting the (lack of) documentation?

Use descriptive comments

Your comments don't give any new information. For example:

//function using arrays for comparison
var _compNumbersUsingArray = function (input1, input2) {

At a minimum, something like the following will work for API documentation:

  /**
   * 
   * @param  {Array}          The first phone number
   * @param  {Array}          The second phone number
   * @return {String}         A predefined message that indicates success or error
   */

Answer 2

Interviewers don't like to read a lot of code, so they rarely ask questions that require you to write a lot of code, especially in the initial screening phase of the hiring process. Your interviewer probably had this solution in mind:

function arePhoneNumbersEquivalent(phone1, phone2) {
    return phone1.replace(/[^0-9]+/g, '') == phone2.replace(/[^0-9]+/g, '');
}

That's all. Maybe also add use strict, documentation, and unit tests.

Specifically, note…

• You wrote two solutions because you couldn't decide, which seems to indicate that you either lacked confidence in your own work, or didn't understand the question (in which case you should seek clarification).

  There may be cases where it is reasonable to provide more than one solution. For example, "Here is a short and simple recursive solution, but it's susceptible to stack overflow. This other solution uses iteration and scales better, but it involves a lot more code and is less readable."

  For this particular question, regular expressions are clearly the superior strategy, so submitting the array-based solution was probably a bad idea.

• Spec compliance. You were prompted to "write a JavaScript function". Not an IIFE that returns an object in a pseudo-OOP framework.
• Interface design. "Do the digits in both string match?" is a yes/no question. I'd expect a boolean return value.
• YAGNI. What's the point of _minPhoneLength? There was never any mention of a need to validate the length, so I would infer that you have a tendency to overcomplicate things. It might be reasonable to check that the input is non-empty after discarding the non-digits, but you could just check str != ''.
• DRY. All of the code is written in duplicate — once per phone number. I would infer that you tend to copy-and-paste code instead of finding ways to reduce code bloat.
• Underscores. I'm not a fan of your convention for naming "private" variables. Not only is it rather noisy, it also looks like you're trying to impose a Java-like straitjacket on JavaScript. JavaScript allows nesting scopes, and the convention seems to indicate a refusal to embrace that feature of the language.