Slot Machine Constructor

Question

This is a fun little program I made of a fruit machine gambler, aka "slot machine". What mistakes am I making? How can I make this better OOP programming, less spaghetti?

var spin = false;

var value1, value2, value3, value4;

var holdFirst, holdSecond, holdThird, holdForth = false;

var first, second, third, forth;

// ******************* constructor

function getValue() {
    var num = Math.random();

	  if(num < 0.2) {
	    this.card = '<i class="fa fa-anchor"></i>';
	  }
	  if(0.2 > num < 0.4 ) {
	    this.card = '<i class="fa fa-heart"></i>';
	  }
	  if(0.4 > num < 0.6){
	    this.card = '<i class="fa fa-lemon-o"></i>';
	  }
	  if(0.6 > num < 1){
	    this.card = '<i class="fa fa-car"></i>';
	  }
}

// ***************** initial

function getInitial() {
	spin = true;

	function startSpining() {
		if(spin){
			value1 = new getValue();
			document.getElementById('value1').innerHTML = value1.card;
			value2 = new getValue();
			document.getElementById('value2').innerHTML = value2.card;
			value3 = new getValue();
			document.getElementById('value3').innerHTML = value3.card;
			value4 = new getValue();
			document.getElementById('value4').innerHTML = value4.card;
		}
	}

	setTimeout(function(){ 
		spin = false;
	}, 1000);

	setInterval(function(){ 
		startSpining();
	}, 10);
}
getInitial();

// ************** hold

function hold(el) {

	$(el).toggleClass('active');

	switch (el.name) {
		case 'first':
			holdFirst = !holdFirst;
			break;
		case 'second':
			holdSecond = !holdSecond;
			break;
		case 'third':
			holdThird = !holdThird;
			break;
		case 'forth':
			holdForth = !holdForth;
			break;
	}

}

// **************** Get new

function getNew() {

	var spin = true;




	function startSpining(firstCard, secondCard, thirdCard, forthCard) {
		
		this.someMethod = function (x) {
			if(spin && !holdFirst){
				x = new getValue();
			}

    };

		if(spin && !holdFirst){
				value1 = new getValue();
				document.getElementById('value1').innerHTML = value1.card;
		}
	}

	function startSpiningFirst() {
		if(spin && !holdFirst){
				value1 = new getValue();
				document.getElementById('value1').innerHTML = value1.card;
		}
	}

	function startSpiningSecond() {
		if(spin && !holdSecond){
				value2 = new getValue();
				document.getElementById('value2').innerHTML = value2.card;
		}
	}

	function startSpiningThird() {
		if(spin && !holdThird){
				value3 = new getValue();
				document.getElementById('value3').innerHTML = value3.card;
		}
	}

	function startSpiningForth() {
		if(spin && !holdForth){
				value4 = new getValue();
				document.getElementById('value4').innerHTML = value4.card;
		}
	}

	setTimeout(function(){ 
		spin = false;
		console.log(value1.card, value2.card, value3.card, value4.card);
	}, 1000);

	setInterval(function(){ 
		startSpiningFirst();
		startSpiningSecond();
		startSpiningThird();
		startSpiningForth();
	}, 10);

}

ul>li {
  margin: 20px;
  width: 60px;
  h2 {
    font-size: 60px;
  }
}

.active {
  background: #ff5a5f !important;
}

<!DOCTYPE html>
<html>
<head>
<script src="https://code.jquery.com/jquery-2.1.1.min.js"></script>
<script src="https://code.jquery.com/jquery.min.js"></script>
      <link rel="stylesheet" href="//maxcdn.bootstrapcdn.com/font-awesome/4.3.0/css/font-awesome.min.css">
<link href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.4/css/bootstrap.min.css" rel="stylesheet" type="text/css" />
<script src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.4/js/bootstrap.min.js"></script>
  <meta charset="utf-8">
  <title>JS Bin</title>
</head>
<body>
      <div class="jumbotron">
          <h1 id="first"></h1>
          <ul class="list-inline">
            <li> <h2><i id="value1" class="fa"></i></h2> </li>
            <li> <h2><i id="value2" class="fa"></i></h2> </li>
            <li> <h2><i id="value3" class="fa"></i></h2> </li>
            <li> <h2><i id="value4" class="fa"></i></h2> </li>
          </ul>
             
      <button class="btn btn-default" onclick="getNew()">Spin</button>
          
          <div class="btn-group" role="group" aria-label="...">
            <button type="button" name="first" onclick="hold(this)" class="btn btn-default">Hold First</button>
            <button type="button" name="second" onclick="hold(this)" class="btn btn-default">Hold Second</button>
            <button type="button" name="third" onclick="hold(this)" class="btn btn-default">Hold Third</button>
            <button type="button" name="forth" onclick="hold(this)" class="btn btn-default">Hold Forth</button>
          </div>


          <h1 id="demo"></h1>
</body>
</html>

Flambino · Accepted Answer · 2015-05-28 23:00:41Z

You have a bug/basic misunderstanding of JS syntax: You can't do chained comparisons in JavaScript, so none of the if(x > num < z) lines work the way you think they do.

Instead what happens is that it's being interpreted as (x > num) < z, which is the same as true < z or false < z. It still does something due to type coercion, however. The left-hand boolean value is converted to zero or one, and then compared. So you're always comparing either 1 < z or 0 < z.

But even if the syntax worked like you think, it's still wrong: 0.2 > num < 0.4 would mean that num should be smaller than both 0.2 and 0.4. Which is another way of saying num < 0.2. Except that because of the boolean-to-number coercion, you instead end up with the exact opposite: The equivalent of checking num > 0.2.

Point is, the code is not doing what you think it's doing.
Why card? This isn't a deck of cards. Something like symbol seems more appropriate.
new getValue(). There's a lot of weird going on here:
1. getValue doesn't sound like a constructor function; it sounds like it's supposed to return a value. So putting new in front of it seems odd.
2. If it is a constructor function, the JavaScript convention would be to name it with PascalCase, not camelCase.
3. Why doesn't it just return a value? It'd be a lot simpler.
Why is getNew called getNew? Why not just spin? Ok, you already have a variable called spin, but that should probably be spinning instead, since that's what describes.
Wat?
```
function startSpining(firstCard, secondCard, thirdCard, forthCard) {

    this.someMethod = function (x) {
        if(spin && !holdFirst){
            x = new getValue();
        }

  };

    if(spin && !holdFirst){
            value1 = new getValue();
            document.getElementById('value1').innerHTML = value1.card;
    }
}
```
First of all: "start spining". Spellcheck that (it's all over the place). Secondly: This whole chunk is never called as far as I can tell, and I have no idea what the declaration of this.someMethod is supposed to do, or what the function itself is supposed to do (it's supposedly called with an argument, x which you then set to something... and then it does nothing? But of course someMethod is never called anyway). And the indentation is all over the place.
Each time you spin, you create a new 10ms interval timer. Click 8 times, and you've got 8 timers running simultaneously, just waiting for spin == true. Probably not what you intend.
"Hold forth" should be "Hold fourth". Ironically, "hold forth" is an expression in English. It means "talking lengthily, assertively, or tediously about a subject" (like how I'm doing here!). But that has little to do with slot machines.
Why are you loading two versions of jQuery? And why load bootstrap's JS scripts? You don't need them for anything here, as far as I can tell.
Why, when you're loading jQuery, aren't you using it for more? As far as I can tell it's only used for one single line: $(el).toggleClass('active');. That's nuts. You're loading a huge library (twice even!) for something that could be handled with 2-3 lines of plain JS.
Meanwhile you're not using jQuery where it'd be useful. For instance, you're setting event handlers in the HTML when jQuery has much more feature-rich event handling. Using onclick attributes is an outmoded technique, and has been for many, many years now. Heck, you're not even using jQuery to find elements, which is perhaps the most basic use of jQuery.

I don't mean to be harsh, but you should probably read up on JavaScript in general. There's a lot here that seems to be just written-until-it-kinda-worked, with little understanding of why it suddenly worked. It seems cobbled together from disparate snippets of code.

And there's little reason to make this object oriented. Or, rather, it is object oriented since you're using the DOM, and the DOM is the Document Object Model. But there's little reason to invent your own constructors.

I'd do something like this:

// Get a random symbol class
function getRandomIconClass() {
  var rand = Math.random();
  
  if(rand < 0.25) return "fa fa-anchor";
  if(rand < 0.50) return "fa fa-heart";
  if(rand < 0.75) return "fa fa-car";
  return "fa fa-lemon-o";
}

// Listen for "hold"-button clicks
$(document).on("click", ".wheel button", function () {
  var button = $(this);
  button.toggleClass("active");
  button.parent().toggleClass("hold");
  button.blur(); // get rid of the focus
});

$(document).on("click", "#spin", function () {
  // get a plain array of symbol elements
  var symbols = $(".wheel").not(".hold").find("i").get(); 
  
  if(symbols.length === 0) {
    alert("All wheels are held; there's nothing to spin");
    return; // stop here
  }
  
  var button = $(this);
  
  // get rid of the focus, and disable the button
  button.prop("disabled", true).blur();
  
  // counter for the number of spins
  var spins = 0;
  
  // inner function to do the spinning
  function update() {
    for(var i = 0, l = symbols.length ; i < l ; i++) {
      symbols[i].className = getRandomIconClass();
    }
    
    if(++spins < 50) {
      // set a new, slightly longer interval for the next update. Makes it seem like the wheels are slowing down
      setTimeout(update, 10 + spins * 2);
    } else {
      // re-enable the button
      button.prop("disabled", false);
    }
  }
  
  // Start spinning
  setTimeout(update, 1);
});

// set the wheels to random symbols when the page loads
$(function () {
  $(".wheel i").each(function () {
    this.className = getRandomIconClass(); // not using jQuery for this, since we don't need to
  });
});

.wheel {
  width: 25%;
  float: left;
  text-align: center;
}

.wheel .fa {
  display: block;
  font-size: 4em;
  margin-bottom: 0.5em;
}

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<link rel="stylesheet" href="//maxcdn.bootstrapcdn.com/font-awesome/4.3.0/css/font-awesome.min.css">
<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.4/css/bootstrap.min.css">

<div id="wheels">

  <div class="wheel clearfix">
    <i></i>
    <button type="button" class="btn btn-default">Hold</button>
  </div>
  
  <div class="wheel clearfix">
    <i></i>
    <button type="button" class="btn btn-default">Hold</button>
  </div>
  
  <div class="wheel clearfix">
    <i></i>
    <button type="button" class="btn btn-default">Hold</button>
  </div>
  
  <div class="wheel clearfix">
    <i></i>
    <button type="button" class="btn btn-default">Hold</button>
  </div>

  <!-- add more wheels if you want; just remember to update the width in the CSS -->

</div>

<p class="text-center">
  <button id="spin" type="button" class="btn btn-default">Spin</button>
</p>

Addendum: I feel it's worth pointing out that the above doesn't behave like a slot machine (nor does the original implementation). It's more like rolling dice, since each "wheel" can assume any value on each update.

A more "realistic" implementation would be to cycle through the possible values, in order, but make the number of cycles random.

var SYMBOL_CLASSES = ["anchor", "heart", "car", "lemon-o"].map(function (klass) {
  return "fa fa-" + klass;
});

$(document).on("click", ".wheel button", function () {
  $(this).toggleClass("active").blur().parent().toggleClass("hold");
});

$(document).on("click", "#spin", function () {
  var wheels = $(".wheel").not(".hold").find("i"),
      wheelCount = wheels.length;
  
  if(wheelCount === 0) {
    alert("All wheels are held; there's nothing to spin");
    return;
  }
  
  var button = $(this).prop("disabled", true).blur();
  
  // keep track of running wheels
  var counter = wheelCount;
  function stopped() {
    if(--counter === 0) {
      button.prop("disabled", false);
    }
  }
  
  // create a bunch of wheel-spinning functions, each slightly offset
  var steps = wheels.each(function (i, element) {
    var wheel = $(this),
        count = 50 + (i * 3) + (Math.random() * 20 | 0),
        spins = $(this).data("current-symbol");
    
    function spinner() {
      element.className = SYMBOL_CLASSES[++spins % wheelCount];
      if(spins < count) {
        setTimeout(spinner, 1 + spins * 2);
      } else {
        wheel.data("current-symbol", spins % wheelCount);
        stopped();
      }
    };
    
    spinner();
  });
});

$(function () {
  $(".wheel i").each(function () {
    var rand = Math.random() * 4 | 0;
    $(this).data("current-symbol", rand).addClass(SYMBOL_CLASSES[rand]);
  });
});

.wheel {
  width: 25%;
  float: left;
  text-align: center;
}

.wheel .fa {
  display: block;
  font-size: 4em;
  margin-bottom: 0.5em;
}

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<link rel="stylesheet" href="//maxcdn.bootstrapcdn.com/font-awesome/4.3.0/css/font-awesome.min.css">
<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.4/css/bootstrap.min.css">

<div id="wheels">

  <div class="wheel clearfix">
    <i></i>
    <button type="button" class="btn btn-default">Hold</button>
  </div>
  
  <div class="wheel clearfix">
    <i></i>
    <button type="button" class="btn btn-default">Hold</button>
  </div>
  
  <div class="wheel clearfix">
    <i></i>
    <button type="button" class="btn btn-default">Hold</button>
  </div>
  
  <div class="wheel clearfix">
    <i></i>
    <button type="button" class="btn btn-default">Hold</button>
  </div>

  <!-- add more wheels if you want; just remember to update the width in the CSS -->

</div>

<p class="text-center">
  <button id="spin" type="button" class="btn btn-default">Spin</button>
</p>

SirPython · Accepted Answer · 2015-05-27 23:57:03Z

Your indentation is all out of whack; in some places, you have unnecessary indenation, and in others you don't have necessary indentation.

For example, in the getValue function, why are those if statements indented?

And, in the .someMethod method of the startSpinning function of the getNew , the closing bracket should have another indent.

Your constructor should have a name describing what object it is returning, not what it is doing.

Your getValue class really isn't/shouldn't be a class.

All it is doing is returning a value; why isn't it just a normal function?

To make this thing "more object oriented", I recommend that you put all of this into a class.

Right now, you have variables spread out, and you have functions with names that make them sound like they are methods for a class (get...), even though they aren't.

You should make a classes like SlotMachine and Spinner where SlotMachine has a few Spinners that have spin methods.

You should make your function names much, much more specific.

someMethod -- this tells me absolutely nothing.

getNew -- get new what? If you are getting a new object, this should be class, and then you should be getting a new whatever with the new keyword.

It should be spelled "fourth", not "forth".

I'm not sure if this is an issue on all browsers, but it has been an issue for me before on Google Chrome:

By using setInterval and setTimeout, you are creating multiple threads for code to run simultaneously.

However, when with more than one setInterval running, you are going to get some timing issues.

Issues for me in the past have been that some code is skipped, or the timing gets all messed up and nothing runs at the interval you want it to.

To fix this, I recommend that you try to keep everything on one thread; try finding a way to combine the functions being run in the setInterval in getNew with the functions being run in the setInterval in getInitial.

Again, this is only sometimes a problem on some browsers.

Now to talk about your HTML.

The tags inside the head tags should be indented. Some are, some aren't, and that is even worse than not having them indented at all.

You should try running your code through an HTML validator.

Community · Accepted Answer · 2017-05-23 11:33:42Z

7

Random Issues

This seems bad:

function getValue() {
    var num = Math.random();

      if(num < 0.2) {
        this.card = '<i class="fa fa-anchor"></i>';
      }
      if(0.2 > num < 0.4 ) {
        this.card = '<i class="fa fa-heart"></i>';
      }
      if(0.4 > num < 0.6){
        this.card = '<i class="fa fa-lemon-o"></i>';
      }
      if(0.6 > num < 1){
        this.card = '<i class="fa fa-car"></i>';
      }
}

When called repeatedly by this and similar:

value1 = new getValue();

I'm no Javascript expert, but I think you could improve performance by generating a large number of random slot machine sequences when the program starts, rather than calling Math.random() repeatedly as the program is running. Maybe the performance gain is not that important, but I think it is worth mentioning. Also, when you start thinking about generating a large number of sequences, you could think about ways to manipulate the results, potentially for gameplay reasons (such as a power-up increases the odds of a certain value).

This may not apply so much to Javascript, but definitely in other languages calling the equivalent of Random() can be costly.

I looked up Math.random() and it appears that there may be better options for better random generation. See this Stack Overflow answer about Javascript randomness.

edited May 23, 2017 at 11:33

CommunityBot

1

answered May 28, 2015 at 1:14

bazola

8,5792 gold badges32 silver badges72 bronze badges

\$\begingroup\$ Why are you reviewing a mix of my answer's code, and OP's code? \$\endgroup\$
– Flambino
May 28, 2015 at 1:33
\$\begingroup\$ Also, there's no Random object in JavaScript, just the Math.random() function, which is also what's used in the SO answer you linked to. \$\endgroup\$
– Flambino
May 28, 2015 at 1:37
\$\begingroup\$ Sorry, fixed the mistake with reviewing your code! \$\endgroup\$
– bazola
May 28, 2015 at 1:38
\$\begingroup\$ Roger that. I'd go over the review again if I were you, though. As far as I can tell, OP's code is not "good and clean and easy to understand". Don't mean to sound like a self-important ass, but I think you might have been looking more at my code for that as well. Also, I'm curious: What's "the new Javascript array syntax" you're referring to? \$\endgroup\$
– Flambino
May 28, 2015 at 1:41
\$\begingroup\$ You are partially correct. Thanks for pointing it out, its worthwhile to try to improve the answers on the site. \$\endgroup\$
– bazola
May 28, 2015 at 1:49

Add a comment |

Stack Exchange Network

Slot Machine Constructor

3 Answers 3

Your Answer

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

Hot Network Questions

Slot Machine Constructor

3 Answers 3

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

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