3
\$\begingroup\$

Just curious if there is a more elegant way to perform the same result. I have a calculated rating which I want to convert to visible stars using fontawesome icons. Max rating is 5. I am rounding the rating to the nearest .5. Done in js given a rating called data (sum of all votes / number of votes).

//example, can be anything from 0-5 after being computed
data = 3.65;

//get rating in nearest .5 value (this is how many stars need to be yellow)
rating = yellow = (Math.round(data*2)/2).toFixed(1);

//how many stars need to be white
white = 5 - rating;

output = '<div title="'+rating+'">';

//output full yellow stars
while (yellow >= 1) {
    output += '<i class="fa fa-star text-yellow"></i>';
    yellow--;
}
//what is left can either be .5 or 0... if .5 output half yellow star
if (yellow == .5) {
    output += '<i class="fa fa-star-half-o text-yellow"></i>';
}
//output remaining full white stars
while (white >= 1) {
    output += '<i class="fa fa-star-o text-yellow"></i>';
    white--;
}

return output + '</div>';

The above does what I want it to, but I have to believe there is a better approach to get the same result.

\$\endgroup\$
2
\$\begingroup\$

I recently implemented this same thing, here's what I used. Main difference: Let the for loop do the math for you. By the end of the first loop, if there is half a star it will be in the i variable. Use an array to buffer your string (or create DOM objects instead, don't use += to build a string).

document.getElementById("stars").innerHTML = getStars(3.6);

function getStars(rating) {

  // Round to nearest half
  rating = Math.round(rating * 2) / 2;
  let output = [];

  // Append all the filled whole stars
  for (var i = rating; i >= 1; i--)
    output.push('<i class="fa fa-star" aria-hidden="true" style="color: gold;"></i>&nbsp;');

  // If there is a half a star, append it
  if (i == .5) output.push('<i class="fa fa-star-half-o" aria-hidden="true" style="color: gold;"></i>&nbsp;');

  // Fill the empty stars
  for (let i = (5 - rating); i >= 1; i--)
    output.push('<i class="fa fa-star-o" aria-hidden="true" style="color: gold;"></i>&nbsp;');

  return output.join('');

}
<link href="https://maxcdn.bootstrapcdn.com/font-awesome/4.7.0/css/font-awesome.min.css" rel="stylesheet" />

<b>Rating: <span id=stars></span></b>

\$\endgroup\$
3
\$\begingroup\$

The algorithm looks good to me. What I would change:

Use for loops when iterating over a previously known range or sequence

for ( ; yellow >= 1; yellow--)
  output += '<i class="fa fa-star text-yellow"></i>';
// ...
for ( ; white >= 1; white--)
  output += '<i class="fa fa-star-o text-yellow"></i>';

for-loops are better at communicating what is being iterated over and there's a common recommendation to not alter the iteration counter outside of the loop header. Both of these individually and even more so together make it much easier to read and understand loops.

Of course some algorithms are more elegant and easier to understand if you disregard these recommendations. That's why they're recommendations for the majority of simple cases and not hard rules.

Use String.prototype.repeat() to repeat strings

Instead of the loops above you can more elegantly (and probably more efficiently) use:

output += '<i class="fa fa-star text-yellow"></i>'.repeat(yellow);
// ...
output += '<i class="fa fa-star-o text-yellow"></i>'.repeat(white);

Manipulate DOM objects instead of generating HTML code in JavaScript

This may be a personal preference but I wouldn't even bother with HTML here.

var output = document.createElement("div");
output.title = rating;

for ( ; yellow >= 1; yellow--)
{
  var star = document.createElement("i");
  star.className = "fa fa-star text-yellow";
  output.appendChild(star);
}
if (yellow == .5) {
  var star = document.createElement("i");
  star.className = "fa fa-star-half-o text-yellow";
  output.appendChild(star);
}
for ( ; white >= 1; white--)
{
  var star = document.createElement("i");
  star.className = "fa fa-star-o text-yellow";
  output.appendChild(star);
}

return output;

This will directly construct the DOM (sub-)tree that you're trying to describe in HTML.

Respect the intent behind HTML tags

<i> is intended for text set in italic. However the use of “physical” formats in HTML is discouraged in favour of cascading styles (CSS). Use “logical” formats like <em> or <strong> or the other suggested tags on the previously linked MDN page to designate your intent and format them with CSS (or trust the client web browser to use a sensible default if you care about intent but not the actual resulting typesetting). If none of the pre-defined intents match your case, use <span> and annotate it with classes, then format it with CSS.

\$\endgroup\$
  • 1
    \$\begingroup\$ While <span> would indeed be more semantically correct than <i>, the "official" guide uses <i> somewhy. To be safe, I'd use whatever the official docs use, because the css selector may [in future] target i.fa instead of fa (which I hope never happens). I wonder what were they thinking about <i> in FontAwesome when decide to show the samples they show. fontawesome.io/icon/star-o \$\endgroup\$ – Igor Soloydenko Oct 15 '17 at 23:08
  • \$\begingroup\$ Thanks - this definitely gave me another outlook on it. Yes, the fontawesome icons use <i> by default. .repeat would not work in this case as it is possible to have half values... so while I could output the initial full yellow stars this way the value of yellow would not be decremented either so I wouldn't know if I needed to show a .5 star before moving onto the white ones. I do like the for loop usage and makes it a little cleaner. \$\endgroup\$ – user756659 Oct 16 '17 at 3:26
  • \$\begingroup\$ I guess I should also add this is part of a much larger project (datatables.net) so this is basically part of an output formatter - which is why the html is included. \$\endgroup\$ – user756659 Oct 16 '17 at 3:29
  • \$\begingroup\$ @user756659 - wait... Allan? Is that you? \$\endgroup\$ – iwrestledabearonce Oct 17 '17 at 18:52
  • \$\begingroup\$ I am not Allan. \$\endgroup\$ – user756659 Oct 17 '17 at 19:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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