principlejavascriptMinor
Function to compare phone numbers
Viewed 0 times
comparefunctionphonenumbers
Problem
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;
}
}
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;
}
}
Solution
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.
The user will have to call your functions like this:
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:
At a minimum, something like the following will work for API documentation:
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
*/Code Snippets
CompareNumbers.comparePhoneNumbers1('123', '123');//function using arrays for comparison
var _compNumbersUsingArray = function (input1, input2) {/**
*
* @param {Array} The first phone number
* @param {Array} The second phone number
* @return {String} A predefined message that indicates success or error
*/Context
StackExchange Code Review Q#101436, answer score: 5
Revisions (0)
No revisions yet.