5
\$\begingroup\$

Challenge:

Check if a string (first argument, str) ends with the given target string (second argument, target).

This challenge can be solved with the .endsWith() method. But for the purpose of this challenge, do not use it.

I would like to especially have my solution critiqued.

function confirmEnding(str, target){
  let lengthOfString = str.length;
  let lengthOfTarget = target.length;
  let subtractLengths = lengthOfString - lengthOfTarget;
  let lastIndexOfString = str.lastIndexOf(target);

  if(str.includes(target) === true){
    if(lastIndexOfString === subtractLengths){
      return true;
    }
    else{
      return false;
    }
  }
  else{
    return false;
  }
}

Test Cases:

confirmEnding("Bastian", "n") should return true.
Passed
confirmEnding("Congratulation", "on") should return true.
Passed
confirmEnding("Connor", "n") should return false.
Passed
confirmEnding("Walking on water and developing software from a specification are easy if both are frozen", "specification") should return false.
Passed
confirmEnding("He has to give me a new name", "name") should return true.
Passed
confirmEnding("Open sesame", "same") should return true.
Passed
confirmEnding("Open sesame", "pen") should return false.
Passed
confirmEnding("Open sesame", "game") should return false.
Passed
confirmEnding("If you want to save our world, you must hurry. We dont know how much longer we can withstand the nothing", "mountain") should return false.
Passed
confirmEnding("Abstraction", "action") should return true.
\$\endgroup\$
  • 2
    \$\begingroup\$ really common anti-pattern: if b { return true } else { return false } just return b. \$\endgroup\$ – Alexander Sep 12 at 16:46
15
\$\begingroup\$

Your solution is far too complicated. It's also inefficient, because it uses functions .lastIndexOf() and .includes(), both of which analyze the entire str looking for target, whereas an optimal solution should look only starting at a known position at the end of str.

Here are two simple solutions:

function confirmEnding(str, target) {
  return str.startsWith(target, str.length - target.length);
}

function confirmEnding(str, target) {
  return target == str.substring(str.length - target.length);
}

console.log(
  false == confirmEnding("s", "try long target strings") &&
  false == confirmEnding("", "try an empty str") &&
  true  == confirmEnding("Bastian", "n") &&
  true  == confirmEnding("Congratulation", "on") &&
  false == confirmEnding("Connor", "n") &&
  false == confirmEnding("Walking on water and developing software from a specification are easy if both are frozen", "specification") &&
  true  == confirmEnding("He has to give me a new name", "name") &&
  true  == confirmEnding("Open sesame", "same") &&
  false == confirmEnding("Open sesame", "pen") &&
  false == confirmEnding("Open sesame", "game") &&
  false == confirmEnding("If you want to save our world, you must hurry. We dont know how much longer we can withstand the nothing", "mountain") &&
  true  == confirmEnding("Abstraction", "action")
)

The first solution, using .startsWith(), should be efficient, but it might be considered "cheating" to use .startsWith() even though only .endsWith() is prohibited.

The second solution is slightly simpler, but it involves creating a substring, so it would be less efficient.

\$\endgroup\$
8
\$\begingroup\$

Review

You have a bit too many variables to my taste, but that's not much of a problem.

let lengthOfString = str.length; // is a variable that useful here?

I would use const instead of let because the variables are only set once.

The if-statements could be written with much less overhead.

First,

str.includes(target) === true

could be written as

str.includes(target)

But even better is to leave it out entirely, since the index would be -1 when not included anyway, which would never match str.length - target.length. Matti Virkkunen found an edge case that escaped my attention. I would still leave this check out, but add a guard that the last index should be > -1.

Further, the false branches are completely redundant. You should return the result of the condition rather than creating unnecessary branches yielding true or false based on the condition.


Simplified Solution

The function could be shortened to:

function confirmEnding(str, target) {
  const subtractLengths = str.length - target.length;
  const lastIndexOfString = str.lastIndexOf(target);
  return lastIndexOfString === subtractLengths && lastIndexOfString > -1;
}

Since the challenge description and test cases don't mention null and undefined values, I didn't include any checks for these edge cases.

\$\endgroup\$
  • 1
    \$\begingroup\$ The includes check did actually do something and now e.g. confirmEnding("1234", "asdfg") returns true due to how lastIndexOf can return -1 - a parameter length check would be a better fix than includes though. \$\endgroup\$ – Matti Virkkunen Sep 12 at 7:10
  • \$\begingroup\$ @MattiVirkkunen well spotted, in this case we should include it again or add a check that the index must be >= 0. Anyway, more compact alternatives are provided in the other answer. I've edited the -1 bound check for better performance over the includes. \$\endgroup\$ – dfhwze Sep 12 at 7:13
  • \$\begingroup\$ -1 because this solution is an O(str) anti-pattern that does not obey to the performance characteristics of .endsWith. Checking the ending of a string should only require looking at the end of the string, aka O(target). This solution takes a long time for cases like confirmEnding('1'.repeat(1e9), '5') while '1'.repeat(1e9).endsWith('5') takes a negligible amount of time. \$\endgroup\$ – Voile Sep 12 at 9:37
  • \$\begingroup\$ @Voile I did not provide an alternative solution, I only reviewed the current solution and suggested how to clean the code up. For better performing alternatives, I have to redirect you to the other answer. \$\endgroup\$ – dfhwze Sep 12 at 11:22
7
\$\begingroup\$

Review

This review may sound a bit harsh, but bad habits are hard to break so I point them out.

  • Use constants for variables that do not change.
  • Don't create single use variables if they do not improve readability.
  • Don't test for equality / inequality when the statement expression evaluates to a boolean. eg if(str.includes(target) === true){ is the same as if (str.includes(target)) {
  • Idiomatic JS has space after if and else, else on the same line as closing } space between ){
  • when if statements have all the branches return the literal value of the evaluated expression, return the result of the expression not literals. eg You have if (lastIndexOfString === subtractLengths) { return true } else { return false } can be just return lastIndexOfString === subtractLengths;

  • If a statement block returns then it should not be followed by an else eg if(foo) { return bar ) else { return foo } does not need the second block if (foo) { return bar } return foo;

  • Don't repeat logic. You find the last index with str.lastIndexOf(target); then you test if target is in str. with str.includes(target) yet the index you have from the previous line holds that information already if (lastIndexOfString > -1) { You are just doubling up on the same complex test.
  • Avoid preempting calculations that may not be needed. The value subtractLengths is only needed if str contains target, if not you have calculated a value that is never used.

Names

Your naming is very bad. Too verbose, inconsistent, and just a mess.

When naming variables remember the context in which the names are created. They need only have meaning within this context and need nothing outside this boundary.

We all can read, but most computer languages have a problem with spaces, soWeEndUpUsingSomeCrazy capitalization to get past this. This makes these names very prone to being miss read and miss typed. As JS is not compiled such errors can hide in code for a very long time. When naming less is more.

Some naming tips.

  • Keep names short 1 2 words if possible.
  • Use common abbreviations. eg str, for string, idx for index, len for length. Don't make up abbreviations
  • Don't use conjunctions and avoid prepositions in variable names.
  • Meaning is inferred, and often in good code what the variable name actually is, is only relevant as an identifier, the semantic meaning being self evident from its location, scope, declaration, assignment, and use.

Rewrite

Using the above points rewriting your function with the same logic we get a smaller cleaner faster and arguably better function

function confirmEnding(str, endStr) {
    const idx = str.lastIndexOf(endStr);
    if (idx > -1) { return idx === (str.length - endStr.length) }

    return false;
}

I would change some things to cover some edge cases you may not have thought of.

function confirmEnding21(str = "" , end = "") {
    const idx = str.lastIndexOf(end);
    return idx > -1 && end.length > 0 && idx === (str.length - end.length);
}

Testing

Your tests are weak.

Altogether you only have 4 different tests.

confirmEnding("Bastian", "n"); 
confirmEnding("Congratulation", "on");
confirmEnding("Connor", "n");
confirmEnding("Open sesame", "pen");

All the other tests are just repeating one of the above. However there are many tests you have not presented. (NOTE I am guessing the expected values so only show my expectation and functions return)

confirmEnding("","");        // expect false. Actual true
confirmEnding("abcdef", ""); // expect false. Actual true
confirmEnding("abcdef", "abcdef"); // expect true. Actual true
confirmEnding("abcde", "abcdef");  // expect false. Actual false
confirmEnding("", "abcdef");       // expect false. Actual false

valid edge cases

confirmEnding("abcdef"); // expect false. Throws Cannot read property 'length' of undefined
confirmEnding();         // expect false. Throws Cannot read property 'length' of undefined     

totally unexpected values

confirmEnding(1000, 0);   // expect true. Throws str.lastIndexOf is not a function
confirmEnding(1000, null);// expect false. Throws Cannot read property 'length' of null

A function throwing an exception can be normal behavior and pass the unit tests if the exception is the correct type. This is part of the reason unit tests must be extensive. Subtle changes to the code layout and style can change the behavior.

For example moving the line let lastIndexOfString = str.lastIndexOf(target); to the top of the function will change the type of errors thrown for some input. Unit test finds these inconsistencies and prevents a harmless source change become an sleeping disaster.

Unit tests must cover all behaviors or they are a waste of your time.

\$\endgroup\$
  • \$\begingroup\$ Where end is an empty string, I personally would expect a false result, but the existing endsWith method returns true. Given that the challenge states that it "can be solved with the .endsWith() method", I think we should use tests that replicate the bahaviour of endsWith. \$\endgroup\$ – moopet Sep 12 at 13:14
  • \$\begingroup\$ @moopet Personally JS handling of empty strings "" is a little out there. take this little non sequitur a = "A", b = "", a[a.indexOf(b)] !== b is true!!! Anyways the correctness of OP's code is not really in question IMHO \$\endgroup\$ – Blindman67 Sep 12 at 14:36
  • \$\begingroup\$ Don't worry about seeming harsh or anything, I'm going to take notes of your advice and use it for later solutions \$\endgroup\$ – DreamVision2017 Sep 17 at 0:03
2
\$\begingroup\$

The solution makes sense, but like @dfhwze said, there are too many variables.

As a future maintainer, I skim your solution and wonder what subtractLengths is, because it's ambiguous unless you move your eyes up to read more code to get the context. It doesn't read very smoothly.

I would re-arrange it a bit to read more like this:

str.lastIndexOf(target) + target.length === str.length

because that's easily parsable in English - the last mention of what you're looking for, plus how long it is, should be the end of the string. Only make variables (or in this case consts) if they are going to be re-used or if they help clarify the purpose of an expression.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.