CODEWITHSUNDEEP
javascripthtml
ewjedi
ewjedi

Reputation: 3

Trying to make an on/off/auto button

I am new to javascript and am trying to make a button that cycles through on/off/auto each click. Each state also has to run code in the if statement and code in the switch as well. I currently cannot seam to get it to run right. Later Once I get this working I want the "if statement" function to be in its own .js file so I can reference it for other on/off/auto buttons. While the switch will be apart of the main code. What am I doing wrong?

function cycle()
{
    var onoffB = (); 
    if (document.getElementById("button1").value="On")
    {
        onoffB=1;
        document.getElementById("button1").value="Off";
    }  

    else if (document.getElementById("button1").value="Off")
    {
        onoffB=2;
        document.getElementById("button1").value="Auto"
    }
    else
    {
        onoffB=0;
        document.getElementById("button1").value="On"
    }

    switch(onoffB)
    {
        case 0:
        //running code;
        break;
        case 1:
        //running code;
        break;
        case 2:
        //running code;
        break;
    }
}

</script>
</head>
<body>

<input type="button" id="button1" value="On"  onclick="cycle()">

</body>
</html>

Upvotes: 0

Views: 311

Answers (4)

StackSlave
StackSlave

Reputation: 10627

Try something like this:

var doc = document, bod = doc.body;
function E(e){
  return doc.getElementById(e);
}
var state = 'On';
function onOffAuto(element){
  var e = element;
  switch(state){
    case 'On':
      e.value = state = 'Off';
      return;
    case 'Off':
      e.value = state = 'Auto';
      return;
    case 'Auto':
      e.value = state = 'On';
      return; 
   }
}
E('button1').onclick = function(){
  onOffAuto(this);
}

Upvotes: 0

Ethan Brown
Ethan Brown

Reputation: 27282

Well, there are two big problems I see right off the bat. First of all, the following syntax is invalid JavaScript:

var onoffB = ();

I think what you mean there is to have an undefined state for onoffB, which your'e better off doing with:

var onoffB = null;

Secondly, you're using assignment (=) instead of comparison (== or ===). (As a rule of thumb, you should always prefer === over == unless you have a very good reason and know what you're doing.) Consider the following:

var x = 3;                        // *sets* the value of x to 3

var x == 3;                       // invalid syntax
var x === 3;                      // invalid syntax

if( x = 2 ) { print 'x is 2'; }   // will *always* print 'x is 2', no matter what
                                  // the value of x is; x now has the value 2
                                  // PROBABLY NOT WHAT YOU WANT

if( x = 0 ) { print 'x is 0'; }   // will *never* print 'x is 0', no matter what
                                  // the value of x is; x now has the value 0
                                  // PROBABLY NOT WHAT YOU WANT

if( x === 5 ) { print 'x is 5'; } // will only print 'x is 5' if the value of x
                                  // is 5; value of x 

if( x === 0 ) { print 'x is 0'; } // will only print 'x is 0' if the value of x
                                  // is 0; value of x unchanged

There are a lot of stylistic things I would change about this too. Your code's really difficult to read because of your formatting choices. Furthermore, there's a lot of unnecessarily repeated code. If I were writing this, I would shorten it to the following:

switch( document.getElementById("button1").value ) {
  case 'On':
      // do stuff
      break;
  case 'Off':
      // do stuff
      break;
  case 'Auto':
      // do stuff
      break;
  default:
      // probably an error condition; show message, maybe? 
      break;
}

If you really need the value of the on/off/auto button, you can set it inside the case bodies.

Upvotes: 3

MrByte
MrByte

Reputation: 1083

Here's the complete working and running code:

<html>
<head>
<script type="text/javascript">
function cycle()
{
    var onoffB = null; 
    if (document.getElementById("button1").value == "On")
    {
        onoffB = 1;
        document.getElementById("button1").value = "Off";
    }  

    else if (document.getElementById("button1").value == "Off")
    {
        onoffB = 2;
        document.getElementById("button1").value = "Auto";
    }

    else if (document.getElementById("button1").value == "Auto")
    {
        onoffB = 0;
        document.getElementById("button1").value = "On";
    }

    switch(onoffB)
        {
            case 0:
            //running code;
            break;
            case 1:
            //running code;
            break;
            case 2:
            //running code;
            break;
    }
}

</script>
</head>
<body>

<input type="button" id="button1" value="On" onclick="cycle();">

</body>
</html>

As you can see, the main problems were:

  1. You set var onoffB = (); while it should have been null.
  2. You should have used the "==" instead of "=".
  3. Another 'else if' was missing for when the value is 'Auto'.

I hope I've helped :-)

Upvotes: 0

icktoofay
icktoofay

Reputation: 129011

  1. On this line:

    var onoffB = ();

    You need to either remove the initializer or replace the parentheses with something else.

  2. In each of your if statements:

    if (document.getElementById("button1").value="On")

    You're setting the value, making it always true. You probably want to compare using == rather than assigning with = here.

  3. While it's not really a problem but rather a possible improvement, I'd recommend putting the result of document.getElementById in a variable so you don't have to keep calling it. It should be rather quick on its own, but doing it once will probably be faster.

Upvotes: 0

Related Questions