Basic Algorithm Scripting: Truncate a String solution review

Hi. I want somebody to look at this. Is it a good way to solve it? Because others use splice or smth else

function truncateString(str, num) {
  // Clear out that junk in your trunk
  let trunk = str.split('');
  let trunkel = '';
    for(let i=0;num>i;i++){
      if (trunk[i] === undefined){
      }else{
		trunkel += trunk[i];
      }
    }  
  if (trunk.length>num){
 return trunkel + '...';
  }else {
    return trunkel;
  }
  }

truncateString("A-tisket a-tasket A green and yellow basket","A-tisket a-tasket A green and yellow basket".length + 2 );

1 Like

Before going into the details of the code, did you run your function through the tests? Because it fails two cases where the string is longer than num. You’re supposed to return a string of size num. If truncation is necessary you need to keep in mind that the last three characters will be dots so the number of characters extracted from the string will be less than num.

Anyway on to the code.

Solving this with a for loop is fine if a bit old fashioned. That’s how I used to code in C/C++ in the 90s. It’s good practice in my book, it makes you familiar with the concept (sometimes a for loop is the best solution to your problem). However it’s also prone to bugs (e.g. off by one bugs) so nowadays people tend to use predefined functions where possible.

I’ll go into details a bit because I see a few things that could be improved.

for(let i=0;num>i;i++){

The normal way to write this is with i to the left of the comparator. The for loop definition is all about i so we make each instruction start with i:

  • i=0
  • i<num
  • i++

It makes it a lot easier to read. Otherwise it’s a matter of style but I like it better with spaces, again because it’s easier to read. This I can read with no effort at all:

for (let i = 0; i < num; i++){

Moving on to the test:

      if (trunk[i] === undefined){
      }else{
		trunkel += trunk[i];
      }

You don’t write a test with an empty block. Again it’s hard to read. Invert the test:

      if (trunk[i] !== undefined){
		trunkel += trunk[i];
      }

And honestly I would never loop through a string-like array like that – probably because I come from a C background where, if you access an array item that doesn’t exist, it crashes your program. Anyway my opinion is that you should only loop through existing array items. Once you hit trunk[i] === undefined, the rest of the loop is just wasting time (In this case, I mean. It’s different with a sparse array). You could use break of course but it would be cleaner to write something like that:

for (let i = 0; i < trunk.length; i++){

or better, make sure it doesn’t get bigger than either trunk.length or num:

let max = Math.min(trunk.length, num);
for (let i = 0; i < max; i++){

In case it’s not clear I used a separate variable so that Math.min(trunk.length, num) is not evaluated every time we go through the loop.

You might think that it’s all a matter of style and in a way it’s true. You might think that, because of that, it doesn’t matter and that is not true. Readability is super important, especially when you start working with other people.

My only other comment is that you’re doing this in hard mode, so well done. Solving this problem with the suggested slice function is a lot easier. You just calculate how many characters you need to keep from the string, slice that many characters, add the three dots and you’re done. I’d suggest to try it too once you’re done with the loop. It will come handy in the future.

1 Like

Thanks for you review.
Yes, it passed beta.freecodecamp tests.
I tried to do this with my knowledge so I used loops. Is my code slower than code with Math or ternary operator?

1 Like

Oh I see, I didn’t realise you did the version on the beta website. They seem to have changed the tests.

Is your code slower than a version with Math.min or the ternary operator? The only way to tell is to measure it but I wouldn’t bother. The strings in this exercise are short and it wouldn’t make any difference. I like this quote by Donald Knuth: “Premature optimisation is the root of all evil”. It means you will do more harm than good if you optimise the parts of your program that don’t need to be optimised.

I was really talking about good habits for writing a for loop, but performance-wise it would only have an impact if num is very big (say, 1 billion) and str is much smaller (say 100). Because then you’d do 1 billion turns of the loop (minus 100) for nothing. It’s not likely to happen here.

1 Like