Skip to main content
Code Review

Return to Revisions

2 of 5
added 902 characters in body
Blindman67
  • 22.8k
  • 2
  • 16
  • 40

##A Code Review

Your code is a mess,

  • Inconsistent indenting.
  • Poor use of space between tokens, and operators.
  • Inappropriate use of variable declaration type let, var, const.
  • Contains irrelevant / unused code. eg substr

##Fails to meet requirements.

You list the requirement

"no trailing spaces in the end."

Yet your code fails to do this in two ways

When string is shorter than required length

 crop("trailing spaces ", 100); // returns "trailing spaces "

When string contains 2 or more spaces near required length.

 crop("Trailing spaces strings with extra spaces", 17); // returns "Trailing spaces "

Note: There are various white space characters not just the space. There are also special unicode characters the are visually 1 character (depending on device OS) yet take up 2 or more characters. eg "πŸ‘¨β€πŸš€".length === 5 is true. All JavaScript strings are Unicode.

##Rewrite

Using the same logic (build return string from array of split words) the following example attempts to correct the style and adherence to the requirements.

I prefer 4 space indentation (using spaces not tabs as tabs always seem to stuff up when copying between systems) however 2 spaces is acceptable (only by popularity)

I assume that the message was converted from ASCII and spaces are the only white spaces of concern.

function crop(message, maxLength) { // use meaningful names
 var result = message.trimEnd(); // Use var for function scoped variable
 if (result.length > maxLength) { // space between if ( > and ) {
 const words = result.split(" "); // use const for variables that do not change
 do {
 words.pop();
 result = words.join(" ").trimEnd(); // ensure no trailing spaces
 if (result.length <= maxLength) { // not repeating same join operation
 break;
 }
 } while (words.length);
 }
 return result;
}

Note: Check runtime has String.trimEnd or use a polyfill or transpiler.

##Update \$O(1)\$ solution

I forgot to put in a better solution.

Rebuilding the string is slow, or passing the string through a regExp requires iteration over the whole string.

By looking at the character at the desired length you can workout if you need to move down to find the next space and then return the end trimmed sub string result, or just return the end Trimmed sub string.

The result has a complexity of \$O(1)\$ or in terms of \$n = K\$ (maxLength) \$O(n)\$

function crop(message, maxLength) {
 if (message.length <= maxLength) { return message.trimEnd() }
 if (message[maxLength] === " ") { return message.substring(0, maxLength).trimEnd() }
 while (--maxLength && message[maxLength] !== " ");
 return message.substring(0, maxLength).trimEnd();
}

It is significantly faster than any other solutions in this question.

Blindman67
  • 22.8k
  • 2
  • 16
  • 40
default

AltStyle γ«γ‚ˆγ£γ¦ε€‰ζ›γ•γ‚ŒγŸγƒšγƒΌγ‚Έ (->γ‚ͺγƒͺγ‚ΈγƒŠγƒ«) /