current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

Is this the simplest way to write this? This code evaluates if a textbox value exceeds 20 characters, and then displays the proper message (essentially truncating the string and then adding an ellipses to the end.)

I've tried using a tertiary operator but doesn't seem to work with appending data to an element as it overwrites what was previously appended.)

if (searchBoxValue.length > numCharToDisplay) {
    searchDropDown.append('<div class="noresults"><span>Searching "' +
    searchBoxValue.trim().substring(0, numCharToDisplay) +
    '..." Found No Results</span></div>');
} else {
    searchDropDown.append('<div class="noresults"><span>Searching "' +
    searchBoxValue.trim() + '" Found No Results</span></div>');
}
share|improve this question
    
Please stop editing your post to add more code and to ask for more reviews; you are invalidating old answers. Read the help center for every site SE site, please. –  SirPython yesterday

2 Answers 2

up vote 2 down vote

While I agree with Tiago Marinho, I believe that this could be taken a step further.

Look at both your appends: that are appending the exact same string. Therefore, we can just store this in a variable:

var text = "<div class='noresults'></span>Search '";

I disagree with Tiago Marinho's point about ternary operators, however. I don't believe that it will have any negative impact on your code.

With good formatting, this can actually make your code look rather nice, in my opinion:

var trimmedSearch = searchBoxValue.length > numCharToDisplay ?
                    searchBoxValue.trim().substring(0, numCharToDisplay)  + "...'":
                    searchBoxValue.trim();

All that's left is adding the final "Found No Results" string. Here is the entire code now.

var text = "<div class='noresults'></span>Search '";
var trimmedSearch = searchBoxValue.length > numCharToDisplay ?
                    searchBoxValue.trim().substring(0, numCharToDisplay) + "...'":
                    searchBoxValue.trim();

searchDropDown.append(text + trimmedSearch + "' Found no results</span></div";
share|improve this answer
    
Wow, you posted your answer at the same time I updated mine. hah –  Tiago Marinho yesterday
    
I'll roll back my edit, because you posted it first. (and also because I haven't noticed that "...'" difference between strings) –  Tiago Marinho yesterday
    
Thanks for your input. Could you check my edit above and provide your critique. –  Jeff yesterday
    
@Jrags87 While I would love to, alas, I cannot. As you see, Jamal has rolled back your edit because you have broken rules found in the help center. Please read there on what you may and may not do after receiving answers. –  SirPython yesterday
up vote 2 down vote

Instead of writing append two times, why don't append a single time with two different values depending on the length of searchBox?

Also, moving the trimmed search value into a variable simplifies the code a bit.

var text,
    trimmedSearch = searchBoxValue.trim();
if (searchBoxValue.length > numCharToDisplay) {
    text = '<div class="noresults"><span>Searching "' +
    trimmedSearch.substring(0, numCharToDisplay) +
    '..." Found No Results</span></div>';
} else {
    text = '<div class="noresults"><span>Searching "' +
    trimmedSearch + '" Found No Results</span></div>';
}
searchDropDown.append(text);

Using ternary operators, as you suggested, wouldn't be a good practice since it complicates things up, leaving maintenance really difficult.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.