5
\$\begingroup\$

The code below is equivalent. I can see pros and cons for both versions. Which is better: the short, clever way, or the long, ctrl+c way?

Short version:

character.on("key",function(key){ var action = ({ "a":{axis:"x",direction:-1}, "d":{axis:"x",direction:1}, "w":{axis:"y",direction:1}, "s":{axis:"y",direction:-1}})[key[1]], stop = key[0]=="-"; if (action) if (stop) this.walkdir[action.axis] = 0; else this.walkdir[action.axis] = this.lookdir[action.axis] = action.direction; });

Long version:

character.on("key",function(key){ switch (key){ case "+a": this.walkdir.x = -1; this.lookdir.x = -1; break; case "+d": this.walkdir.x = 1; this.lookdir.x = 1; break; case "+w": this.walkdir.y = 1; this.lookdir.y = 1; break; case "+s": this.walkdir.y = -1; this.lookdir.y = -1; break; case "-a": if (this.walkdir.x == -1) this.walkdir.x = 0; break; case "-d": if (this.walkdir.x == 1) this.walkdir.x = 0; break; case "-w": if (this.walkdir.y == 1) this.walkdir.y = 0; break; case "-s": if (this.walkdir.y == -1) this.walkdir.y = 0; break; case "space": this.setStance("jumping"); break; }; });
\$\endgroup\$
10
  • \$\begingroup\$ I don't see how the long version is better. \$\endgroup\$ Commented Apr 21, 2013 at 17:09
  • \$\begingroup\$ It's faster and probably much easier to understand. \$\endgroup\$ Commented Apr 21, 2013 at 17:26
  • \$\begingroup\$ @Dokkat In the first example you shouldn't be defining a static object when the function is called - that's definitely going to become costly. Try benchmarking that by removing the object definition out of the function and just accessing it. \$\endgroup\$ Commented Apr 21, 2013 at 17:52
  • 3
    \$\begingroup\$ Faster? Probably not. Even if it were, I doubt it'd make any difference in real code. Easier to read? I think that's where we diverge :) \$\endgroup\$ Commented Apr 21, 2013 at 17:53
  • 5
    \$\begingroup\$ @Dokkat These objections are pretty irrelevant: writing code for people who don’t understand the language is counter-productive since it prevents you from using its features properly. Never succumb to the temptation of doing that. In addition, this code only uses core language features, not obscure hacks. \$\endgroup\$ Commented Apr 21, 2013 at 18:15

1 Answer 1

Reset to default
13
\$\begingroup\$

The short code wins hands down. I understood it immediately, and more importantly, I can trivially verify that the code is reasonably error free. This is much harder with the longer code.

You say that the longer code is easier to understand but I claim that this is objectively wrong.

Case in point, the long code uses lots of magic numbers: 1, 0, -1, … what do these stand for? Ah, the short code tells us: they are directions.

The longer code also makes us scroll (depending on the screen size) to see the whole method. This significantly impacts ease of understanding. I believe there were even studies demonstrating this empirically (but I cannot cite them; Code Complete would probably be the relevant reference here).

The one thing I would change in the short code is the lookup itself: define the dictionary separately, maybe even outside the method, and perform the lookup as follows:

var action = movement_commands[key[1]];

And maybe think about tokenising key properly, i.e. assigning the parts to variables before using them. However, I think that the method is short enough to make this unnecessary.

You also said that the longer code is more efficient but I’d like to see a benchmark before I believe that. You probably think that the first code is slower because of the dictionary lookup. But consider that JavaScript is a dynamic language – every single variable access is potentially a dictionary lookup internally. So there is no difference in performance – indeed, the short code could be faster since there’s less variable lookup involved.

(Of course the two code snippets do different things: the long version handles jumping, and they behave differently when the character was previously walking in one direction and now you cancel walking into a different direction.)

\$\endgroup\$
4
  • \$\begingroup\$ I argue it's easier to understand, not read; mainly for a novice trying to grasp the code; but I'm not sure I should worry about that. About the speed, it's not because of the dictionary lookup, but because of it's creation, which makes it obviously much heavier. As you said, moving the object out of the function would solve the issue, but then, in order not to pollute the outer namespace, I'd have to introduce a closure, making it even worse for novices (and much less readable in general, IMO): jsfiddle.net/EXv7e . Note: I obviously prefer the smaller version. Great answer by the way. \$\endgroup\$ Commented Apr 21, 2013 at 18:24
  • \$\begingroup\$ @Dokkat Well you could declare actions outside of on.character entirely, as a global variable. I’m not a big fan of this, however (keep scope as small as possible). \$\endgroup\$ Commented Apr 21, 2013 at 18:38
  • \$\begingroup\$ @Dokkat A novice will only be a novice for a short time; once they encounter and learn how the short code works, they've learned something new that will help them from then on. \$\endgroup\$ Commented Apr 22, 2013 at 1:07
  • \$\begingroup\$ @Dokkat Also, the speed hit in creating a small, static dictionary like that is going to be completely unnoticeable given what the event is reacting to. (And probably unnoticeable in general compared to the function call itself; and it may just be JIT-compiled and only created once anyway... (I'm not entirely sure if it would, though)) \$\endgroup\$ Commented Apr 22, 2013 at 1:13

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.