Confirm the ending of a string

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.

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.

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.

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.

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.