# Confirm the ending of a string

Posted on

Problem

### 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.
``````

Solution

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

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 `const`s) if they are going to be re-used or if they help clarify the purpose of an expression.