0

I have 5 circles that appear inside a div which change depending on the accuracy of a certain thing.

For example <20 accuracy is only 1 filled (red) circle, with 4 unfilled circles.

Visual: This is 60-80 accuracy http://puu.sh/hnJsR/20af976827.png

The below code is inside:

$(document).ready(function(){

The code:

var accuracy1 = <?php echo 100*(1-$s1/1.33333); ?>;
var accuracy2 = <?php echo 100*(1-$s2/1.33333); ?>;

if (accuracy1 < 20) {
        $('<div class="accuracycircle accuracycircle1"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircleunfilled"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircleunfilled"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircleunfilled"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircleunfilled"></div>').appendTo('#circlesdiv1');
}
elseif(accuracy1 >= 20 && accuracy1 < 40) {
        $('<div class="accuracycircle accuracycircle2"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircle2"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircleunfilled"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircleunfilled"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircleunfilled"></div>').appendTo('#circlesdiv1');
}
elseif(accuracy1 >= 40 && accuracy1 < 60) {
        $('<div class="accuracycircle accuracycircle3"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircle3"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircle3"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircleunfilled"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircleunfilled"></div>').appendTo('#circlesdiv1');
}
elseif(accuracy1 >= 60 && accuracy1 < 80) {
        $('<div class="accuracycircle accuracycircle4"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircle4"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircle4"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircle4"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircleunfilled"></div>').appendTo('#circlesdiv1');
}
else {
        $('<div class="accuracycircle accuracycircle5"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircle5"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircle5"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircle5"></div>').appendTo('#circlesdiv1');
        $('<div class="accuracycircle accuracycircle5"></div>').appendTo('#circlesdiv1');
}

$s1 and $s2 are numbers declared earlier in a PHP tag that I transform to make it an accuracy between 0 and 100.

It doesn't work and is obviously very messy. What am I doing wrong and how could I clean it up too?

11
  • try put quote var accuracy1 = '<?php echo 100*(1-$s1/1.33333); ?>;' Commented Apr 23, 2015 at 5:50
  • did you know about switch? Commented Apr 23, 2015 at 5:51
  • Why on earth would he make a number used as a number a string? elseif is not JavaScript syntax by the way. try else if Commented Apr 23, 2015 at 5:52
  • put the accuracy in a hidden input use js's 'switch` to change the div, and why do you repeat yourself? Commented Apr 23, 2015 at 5:54
  • I'd like to see the code if there were 100 circles to display :) Commented Apr 23, 2015 at 5:57

2 Answers 2

3

Here is how you can clean your code.

  1. Use repeater to repeat the string, instead of repeating the code
  2. Use :lt selector to select all the elements having index less than the provided

Demo

// Repeat a string num times
String.prototype.repeat = function(num) {
  return new Array(num + 1).join(this);
};

var accuracy1 = 70; // Will be dynamic

var totalCircles = 5,
  noOfFilledCircles = Math.ceil(accuracy1 / 20);
noOfFilledCircles = noOfFilledCircles > 5 ? 5 : noOfFilledCircles;

var circleHtml = '<div class="accuracycircle"></div>';

$('#circlesdiv1').append(circleHtml.repeat(totalCircles)).find('div:lt(' + noOfFilledCircles + ')').addClass('accuracycircle1');
div#circlesdiv1 {
  height: 30px;
  width: 200px;
  background-color: #3C3E46;
}
#circlesdiv1 .accuracycircle {
  background-color: #8A8A8A;
  display: inline-block;
  width: 15px;
  height: 15px;
  border-radius: 50%;
  margin: 5px;
}
div.accuracycircle1 {
  background-color: #DBFF94 !important;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.0/jquery.min.js"></script>
<div id="circlesdiv1"></div>

Sign up to request clarification or add additional context in comments.

1 Comment

I'd also recommend using "accuracycircle " css class for unfilled and "accuracycircle accuracycircle-filled" css classes for filled circles. That way, it's easier to toggle filled/unfilled circle using jQuery and it's easier for a human to understand
0

You really really want to stop repeating your code like this. Use loops and conditions to generate stuff. Imagine, you have 100 circles to display, will you repeat 100 lines of codes 100 times? Will you really copy-paste 10.000 lines of code? Besides, each ('#circlesdiv1') is a call to the DOM, this needs to be done once and cached.

Here's a quick go at your problem, it's not perfect, but feel free to adapt it.

It's configurable, you can generate any number of circles you want as a scale, and it's just like 10 lines of code.

@Future downvoters, at least PLEASE SAY WHY you downvote.

/*------ CONFIGURATION -----*/
var accuracy1 = 20
var maxCircles = 5
/* -------------------------*/


var $circlesdiv1 = $('#circlesdiv1')
var filledCircles = parseInt(accuracy1/(100/maxCircles))

// Building empty circles
for(i=0; i<maxCircles; i++)
	$circlesdiv1.append('<div class="accuracyCircle"></div>')

// Filling requested number of circles, adding classes
for(i=0; i<=filledCircles; i++)
	$circlesdiv1
		.find(".accuracyCircle:nth-child("+(i+1)+")")
		.addClass("filled accuracycircle"+(filledCircles+1))
div#circlesdiv1 {
  height: 20px;
  width: 300px;
}
div.accuracyCircle {
  display: inline-block;
  width: 15px;
  height: 15px;
  border-radius: 50%;
  border: #008000 solid 1px;
}
div.accuracyCircle.filled {
  background: #008000;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div id="circlesdiv1"></div>

3 Comments

You are using two for loops, not required at all
True, it can also be done with one. As I said, it's a quick go and it's not perfect. Two loops are maybe easier to understand.
easier to understand(?) and slower when there are 100s/1000s of circles

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.