HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Recursive linear search - branch style and layout

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
linearsearchlayoutrecursivestyleandbranch

Problem

Is my layout of the if/else statement reasonable?

It feels clumsy to me to spread the termination condition over the first three lines of the function. Can I squeeze it into one or two lines? Would it help if I used the ternary operator? How can I make the JavaScript more idiomatic (while still using recursion)? Any other improvements?

var containsA = function(sentence) {
if (sentence.length == 0) {
return false;
}
else {
return sentence[0] === 'A' || containsA(sentence.substr(1));
};
};




var containsA = function(sentence) {
if (sentence.length == 0) {
return false;
}
else {
return sentence[0] === 'A' || containsA(sentence.substr(1));
};
};

// Unit tests
var assert = function(code) {
if (eval(code)) {
document.write(code + " test passed.
");
}
else {
document.write(code + " test FAILED.
");
};
};

assert("!containsA('')");
assert("containsA('A')");
assert("!containsA('a')");
assert("containsA('HAH!')");
assert("!containsA('carrots')");




I have simplified details that are not relevant to my question. This simpler version attempts to reimplement String.prototype.contains(). If I convert my string to an array I could use Array.prototype.find() but my hunch is that would be less readable.

Solution

How can I make the JavaScript more idiomatic?

By using indexOf:

var containsA = function(sentence) {
  return sentence.indexOf('A') != -1;
};


As your update specified that you want to use recursion, let's see what we can do.

Cleaning up, we can remove the else (and the extraneous semi-colon).

var containsA = function(sentence) {
  if (sentence.length === 0) {
    return false;
  }
  return sentence[0] === 'A' || containsA(sentence.substr(1));
};


Or you can write it more succinctly, but I would say at the cost of readability

var containsA = function(sentence) {
  return sentence.length > 0 &&
    (sentence[0] === 'A' || containsA(sentence.substr(1)));
};


Now we could end up creating a lot of substrings. To avoid this, we can pass an index into the string

var containsA = function(sentence, i) {
  i = i || 0;
  if (i >= sentence.length) {
    return false;
  }
  return sentence[i] === 'A' || containsA(sentence, i + 1);
};


Or

var containsA = function(sentence, i) {
  i = i || 0;
  return i < sentence.length &&
    (sentence[i] === 'A' || containsA(sentence, i + 1));
}


In case this is not homework or a thought exercise...

Don't do this. By using recursion (and substr) to solve this, you're simultaneously making your code less efficient, in terms of both time and memory, and less readable. As @Guffa pointed out, it may even blow the stack.

The function containsA has no reason to exist. Just use indexOf where appropriate.

Code Snippets

var containsA = function(sentence) {
  return sentence.indexOf('A') != -1;
};
var containsA = function(sentence) {
  if (sentence.length === 0) {
    return false;
  }
  return sentence[0] === 'A' || containsA(sentence.substr(1));
};
var containsA = function(sentence) {
  return sentence.length > 0 &&
    (sentence[0] === 'A' || containsA(sentence.substr(1)));
};
var containsA = function(sentence, i) {
  i = i || 0;
  if (i >= sentence.length) {
    return false;
  }
  return sentence[i] === 'A' || containsA(sentence, i + 1);
};
var containsA = function(sentence, i) {
  i = i || 0;
  return i < sentence.length &&
    (sentence[i] === 'A' || containsA(sentence, i + 1));
}

Context

StackExchange Code Review Q#62610, answer score: 8

Revisions (0)

No revisions yet.