# 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;
}
}

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

Another solution using most of your existing code, is to not create the additional array called trunk. Strings can be accessed via their indexes just like arrays can. So, in your code you could replace every instance of trunk with str. There is no reason to use additional memory when you do not have to.

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