JavaScript prints the entire array instead of single element

Question

var input = document.querySelector("input");
var button = document.querySelector("button");
var inventory = ["jacket","pants","map"];
var actionsIKnow = ["north","east","west","south"];
var messages = ["A group of dangerous minions. Turn back, you do not have a sword",
  "You have gone too deep into the forest. Want to go further?",
  "A giant ruby crystal",
  "A silent lake; you see your reflection in the water",
  "Clear patch of land",
  "A sleeping dragon. Bettwe not wake it up",
  "A solitary cottage. Faint music can be heard from the inside",
  "Looks like this path leads to the cottage of the flute maker",
  "A lot of tombstones, looks like an old graveyard"
];

var userInput;
var startingPos = 4;

button.addEventListener("click",takeMeThere,false);

function takeMeThere(){
  userInput = input.value;
  userInput = userInput.toLowerCase();
  if(userInput!=null){
    validateInput();
  }
}
function validateInput(){
  for(var i=0;i<actionsIKnow.length;i++){
    if(userInput.indexOf(actionsIKnow[i]!=-1)){
      move(actionsIKnow[i]);
    }
  }
}
function move(where){
  console.log(where);
}

I am making a text based exploring game where the user can select where to go. Where the user wants to go depends on what was entered in the textfield. This data is then passed to move(where) where I console.log(where). Instead of printing north, east, etc it prints the whole of actionsIKnow array. Why ?

Not the problem, but note that the test if(userInput!=null){ will never be false. — nnnnnn
– nnnnnn, Commented Apr 6, 2013 at 6:42
The .value attribute returns a string, and the string method .toLowerCase() also returns a string. To test that the field is not empty compare to an empty string: if (userInput!=""){. You may also like to first trim any white space: userInput = userInput.replace(/\s/g,"");, in case the user enters nothing but spaces... — nnnnnn
– nnnnnn, Commented Apr 6, 2013 at 6:50

dfsq · Accepted Answer · 2013-04-06 07:42:24Z

4

Simple mistake. Change

if(userInput.indexOf(actionsIKnow[i]!=-1)){

to this:

if (userInput.indexOf(actionsIKnow[i]) !== -1 ) {

http://jsfiddle.net/bz8k5/

Edit. As per discussion in comments. To be able to validate input in different format like east, move east, but disallow easter you may also want to make use of more sophisticated validation rules. Maybe using regular expressions:

if (userInput.match(new RegExp("\\b" + actionsIKnow[i] + "\\b"))) {
    move(actionsIKnow[i]);
}

http://jsfiddle.net/bz8k5/2/

edited Apr 6, 2013 at 7:42

answered Apr 6, 2013 at 6:40

dfsq

193k26 gold badges244 silver badges261 bronze badges

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

13 Comments

nnnnnn Over a year ago

+1. You just beat me - guess I shouldn't have "wasted" time creating a fiddle: jsfiddle.net/HuyaT

dfsq Over a year ago

@nnnnnn ))) I created a fiddle after I answered.

Michael Geary Over a year ago

Enter "feasting" into the input box and click Go. It prints "east". Should it?

nnnnnn Over a year ago

Further to what Michael said, the user might enter "North East", and then it would print both "north" and "east" - is that right? (More a question for the OP than for dfsq...)

dfsq Over a year ago

@MichaelGeary Right, this is how OP searches the value in the array. Of course it still needs some work. For example I would use indexof method of the array. jsfiddle.net/bz8k5/1 But this is another question.

|

Michael Geary · Accepted Answer · 2013-04-06 09:44:26Z

3

Look closely at this line of code:

if(userInput.indexOf(actionsIKnow[i]!=-1)){

Let's add spaces to make it easier to see:

if( userInput.indexOf( actionsIKnow[i]!=-1 ) ) {

See it yet? Let's turn the spaces into newlines and indent:

if(
    userInput.indexOf(
        actionsIKnow[i] != -1
    )
) {

Now do you see the problem? :-)

Anyway, what you probably really meant here is simply:

if( userInput == actionsIKnow[i] ) {

Or, as @nnnnnn suggested, you can use .indexOf() instead of your own loop, like this:

function validateInput(){
    if( actionsIKnow.indexOf(userInput) >= 0  ) {
        move( userInput );
    }
}

But beware of one problem: .indexOf() on an array is not available in IE8 and earlier versions of IE. If you need to support IE8, this won't work.

So here is my favorite way to do it: Use an object lookup instead of an array.

Change actionsIKnow to:

var actionsIKnow = { north:1, east:1, west:1, south:1 };

and then change validateInput() to:

function validateInput(){
    if( userInput in actionsIKnow ) {
        move( userInput );
    }
}

This works in all browsers, and if there were a large number of items in actionsIKnow, this object lookup would be much faster than .indexOf(). For a small number of items the performance wouldn't matter, but I think the in operator makes for the nicest reading code.

Here's a jsperf that compares performance between .indexOf() and in for a large number of strings.

edited Apr 6, 2013 at 9:44

answered Apr 6, 2013 at 6:43

Michael Geary

29k9 gold badges67 silver badges77 bronze badges

6 Comments

nnnnnn Over a year ago

In your updated function that uses the in operator, what is i? Should be move(userInput)...

Michael Geary Over a year ago

Oops, sorry about the typo, just fixed it. The function doesn't use i at all any more. It doesn't need to loop.

nnnnnn Over a year ago

Cool. Even with an array the OP could skip the loop and just use the array .indexOf() method.

Michael Geary Over a year ago

Very true. Of course .indexOf() does loop, it just does it for you. If you were checking against a very large dictionary of words, putting them in an object and testing with the in operator should be faster than .indexOf(). But for a small array performance would be fine either way.

ErikE Over a year ago

Michael, how sure are you that the object is faster? Doesn't it have to loop just the same, under the covers?

|

Collectives™ on Stack Overflow

JavaScript prints the entire array instead of single element

2 Answers 2

13 Comments

6 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

13 Comments

6 Comments

Your Answer

Sign up or log in

Post as a guest

Related