Truncate text to some number of words when clicked

Question

I am currently using the code below to truncate text:

 function truncate($elem, wordNum){

var words = $elem.find('p').text().trim().split(/\s+/).slice(0,wordNum);
var replacement = $("<p/>").text(words.join(" ") + "...");
$elem.find('.read-more').click(function(){
   if($(this).text() == 'Read More'){
    replacement.hide();
    $elem.find('p:eq(1)').show();    
   } else if($(this).text() == 'Read Less'){
       replacement.show();
    $elem.find('p:eq(1)').hide();
   }
    $(this).text(function(i,old){
    return old=='Read More' ?  'Read Less' : 'Read More';
});
return false
});

$elem.find('p:first').before(replacement);
$elem.find('p:eq(1)').hide();
}

truncate($("p"), 5);

Works, but it's not the most efficient solution. Is there a better way ?

Answer 1

A few thoughts:

• /\s/ is probably not the best thing to be searching for. I would consider using /\b/ to look for all word boundaries, not just spaces.
• It seems the $elem here would be a jQuery collection, however you are treating it is a single element without using things such a $.each() to be able to apply to multiple elements. Your function fails if more than one element is passed in $elem. This is a bad way to think about writing code against jQuery. You should be thinking about working against collections, not single elements.
• Since you are using jQuery, I would strongly suggest writing this as a jQuery plug-in such that you can better attach the behavior to a collection of elements (i.e. $('.some_class').truncatable({count: ...});.
• Is word count really meaningful as a way to determine how you are truncating? I would think better logic would be to look for first word boundary at a location less than X number of characters in string as your truncation point. That way you get more consistent size of truncated text, not subject to vary greatly based on overly long or overly short words in the content.
• In the second example, it seems odd to hard-code nested <p>, .read_more, etc. into the function. Why would $elem not already be whatever element you want to work with (the <p> in this case)? What if in the future you wanted to apply this to a <div> that does not have a <p> nested inside of it? Your code will break. You are limiting the usefulness of your code in doing this.
• You should get in the habit of using strict comparisons (===, !==) by default instead of loose comparisons (==, !=). This will make your code less fragile to unexpected truthy/falsey behavior, and this easier to maintain/debug. I would say the loose comparisons should only be used when you have a very specific use case for doing so and would ideally include a comment in such cases explaining why such loose comparison is appropriate for that case.
• I question the approach of storing the full/truncated texts in an alternate DOM elements. I would think that perhaps when this truncation behavior is applied to an element, that you might want to store the full text (and perhaps the truncated text as well, if you want to avoid recalculation) in a data attribute. If you take this approach, I would suggest using $.text() or native javascript textContent as means to make this change on click vs. $.html() or innerHTML for performance reasons. You definitely don't want to be inserting nodes in DOM using $.before() here.
• I know this is code review and we are reviewers should focus on the code presented, but I would suggest that you think about whether you are re-inventing the wheel here. There are several plug-ins for jQuery that do this already - https://plugins.jquery.com/tag/truncate/