Everything wrong with: Wikidot.modules.PageRateWidgetModule.callbacks.rate

Hello, I’m Monkatraz and I am a frontend-focused developer working on Wikijump. As you may know, Wikijump is a fork of Wikidot. This isn’t really by choice – Wikijump’s existence was forced by the aging and effectively unmaintained state of Wikidot. It’s in the name, we’re “jumping” to Wikijump to escape Wikidot. Truly, we did not know the nature of what we’re now trying to escape, or we’d have done this sooner.

In the process of learning how Wikidot works, we have found terrible things. Wikidot wasn’t made well. In this article, I want to talk about just one function that can be found in the JavaScript for Wikidot. This function reads like a car accident. For what it does, it’s shockingly long, and invokes so many bad practices that I can’t even list them all. Here is the full function — don’t bother trying to actually understand it completely.

Wikidot.modules.PageRateWidgetModule.callbacks = {
  rate: function(r){

  if(r.status == 'already_voted'){
    var w = new OZONE.dialogs.Dialog();
    w.content = r.body;
    w.show();
    return;
  }

  if(!Wikidot.utils.handleError(r)) {return;}

  OZONE.dialog.cleanAll();

  var el = $("prw54353");
  if(r.type != "S") {
    if (el) {
    var nu = el.innerHTML;
    nu = nu.replace(/\+/, '');
    nu = nu * 1 + 1 * r.points;
    if (nu > 0) {
      nu = '+' + nu;
    }
    }
  }
  else {
    var nu = parseFloat(el.innerHTML);
    var pts = parseFloat(r.points);
    var votes = parseFloat(r.votes);
    if(pts < 0) { // Changed vote downward or removed entirely, we can't see which from here.
    nu = (((votes + 1) * nu) + pts) / (votes);
    }
    else {
    nu = ((votes * nu) + pts) / (votes + 1);
    }
  }

  var eff = new fx.Opacity(el, {duration: 200});
  eff.setOpacity(0);
  el.innerHTML = nu;
  eff.custom(0,1);

  var el = $("prw54354");
  if(r.type != "S") {
    if (el) {
    var nu = el.innerHTML;
    nu = nu.replace(/\+/, '');
    nu = nu * 1 + 1 * r.points;
    if (nu > 0) {
      nu = '+' + nu;
    }
    }
  }
  else {
    if(el) {
    var nu = el.innerHTML;
    var pts = r.points;
    var votes = r.votes;
    nu = ((votes * nu) + pts) / (votes + 1);
    }
  }

  var eff = new fx.Opacity(el, {duration: 200});
  eff.setOpacity(0);
  el.innerHTML = nu;
  eff.custom(0,1);

  var el = $("prw54355");
  if(r.type != "S") {
    if (el) {
    var nu = el.innerHTML;
    nu = nu.replace(/\+/, '');
    nu = nu * 1 + 1 * r.points;
    if (nu > 0) {
      nu = '+' + nu;
    }
    }
  }
  else {
    if(el) {
    var nu = el.innerHTML;
    var pts = r.points;
    nu = nu.replace(/\+/, '');
    nu = reduce((nu, pts) => (nu + pts)) / 2;
    }
  }

  var eff = new fx.Opacity(el, {duration: 200});
  eff.setOpacity(0);
  el.innerHTML = nu;
  eff.custom(0,1);

  }
}

Right off the bat we have:

• Absurd levels of namespacing using a global God object
• Terrible variable names, with the “response” argument being named r
• Confusing and magic-number-like JQuery usage, what the hell does $("prw54355") select?
• Bizarre and just plainly horrible expressions, like nu = nu * 1 + 1 * r.points; (what???)

To be clear: I am not omitting anything important. There isn’t any deleted comments, this hasn’t been transpiled or minified. There isn’t any JSDoc type descriptions, TS interfaces, nothing. The shape of the response data isn’t described anywhere.

Let’s dissect this a bit and see if we can figure out what this code is actually doing:

Wikidot.modules.PageRateWidgetModule.callbacks = {

Even our first line has issues. Wikidot decided to use a really strange form of encapsulation, where everything is wrapped into this Wikidot God object that contains pretty much everything. I don’t know why they did this, there is better alternatives, even back in 2008. I won’t go into detail what they could’ve done instead, but this is absolutely not the way to do it.

Why is this so terrible? Just look at this:

Wikidot.modules.PageRateWidgetModule.vars.points = points;
OZONE.ajax.requestModule(null, p, Wikidot.modules.PageRateWidgetModule.callbacks.rate);

Yes, if you need to mutate a variable, you must write out Wikidot.modules.PageRateWidgetModule.vars first. Do you find this annoying? Apparently the Wikidot developers hated it, and made something even worse!

rate: function(r){

Don’t do this. Don’t give a variable (r, here) a one letter name unless it’s blatantly obvious what it means. r is standing in for “response”, and if they wanted to give it a short name they could’ve just used res.

if(r.status == 'already_voted'){
  var w = new OZONE.dialogs.Dialog();
  w.content = r.body;
  w.show();
  return;
}

Okay, so we know that status is a property of the response object. But, we can tell that rather than using status codes that are obvious and predictable, like HTTP status codes or booleans, they decided to use basically magic API malarkey. At least the intent of the code is somewhat obvious: it pops up a dialog error box. Unfortunately, if you were to take a peek at OZONE.dialogs.Dialog(), you would find some equally horrifying code, but we won’t bother with that today.

if(!Wikidot.utils.handleError(r)) {return;}

Apparently 'already_voted' wasn’t an error? Maybe it’s a special case? Who knows!

Now the true horror begins:

var el = $("prw54353");

That $ isn’t a special JS syntax. It’s the name of a function/object, the JQuery library. JQuery used to be very popular, but nowadays it isn’t needed anymore. This bit of code is for finding an element somewhere within the DOM, presumably an element with a class name of prw54353.

Why is it named that? I have no clue.

if(r.type != "S") {
  if (el) {
    var nu = el.innerHTML;
    nu = nu.replace(/\+/, '');
    nu = nu * 1 + 1 * r.points;
    if (nu > 0) {
      nu = '+' + nu;
    }
  }
}

Okay, if reponse.type != "S"… what? What is “S” as a type? Knowing personally how Wikidot works, it probably means “star”, but this is awful. Don’t do this — if you have to, please for the love of God leave comments.

Continuing on, we make a var named nu and… this naming thing is a pattern. What is nu? I’m inclined to believe that it stands for new, because new is a reserved name in JavaScript and you can’t name a variable that. They could’ve done _new or just, y’know, picked a better name, but they didn’t.

So we fetch the contents of $("prw54353"), and then remove a + sign from it. Since there is absolutely no comments explaining why any of this is being done, I’ll help out by explaining what $("prw54353") actually selects: it selects the +<n> element inside of the rating widget. So they’re removing the leading + sign.

What’s really, REALLY absurd about this is what they do next:

nu = nu * 1 + 1 * r.points;

The hell? Let me split this up so we can see the order of operations:

nu = (nu * 1) + (1 * r.points);

Still confused? Well, you need to know some archaic JS to figure it out. They’re casting a string to a number by multiplying a presumably valid (string) number by 1.

// sorta equivalent
const n: number = "5" * 2 // number: 10

That’s why they’re able to use a comparison operator afterwards:

if (nu > 0) {

Finally, they reconvert their number back into a string:

nu = '+' + nu;

The worst part about this is that they didn’t even need to do the string * 1 shenanigans — parseInt(el.innerHTML) would’ve accepted the leading plus sign and returned a number. And for r.points, they could’ve just called parseInt(r.points).

Here, they prove it to you:

else {
  var nu = parseFloat(el.innerHTML);
  var pts = parseFloat(r.points);
  var votes = parseFloat(r.votes);
  if(pts < 0) { // Changed vote downward or removed entirely, we can't see which from here.
    nu = (((votes + 1) * nu) + pts) / (votes);
  }
  else {
    nu = ((votes * nu) + pts) / (votes + 1);
  }
}

Literally in the next few lines, they use parseFloat. Wrong function, but I’ll take it.

Actually, screw that, I won’t take it. They mess it up near instantly — in the first branch of the if, they check if the element exists, but in the else branch, they don’t. They just assume it exists.

I’m not actually sure why this code doesn’t immmediately error. I think it’s because JQuery doesn’t return undefined or null for a missing element, or something to that effect. Which means that parseFloat would cast that innerHTML, whatever it is, into a number, probably 0.

I’m not going to go into as much detail about what they’re doing with those expressions at the bottom – to be honest, it’s not that entertaining or interesting. What is hilarious is how many parentheses they’ve used. Why did they wrap a bare votes? Why use parentheses now and not before? The mind wonders.

var eff = new fx.Opacity(el, {duration: 200});
eff.setOpacity(0);
el.innerHTML = nu;
eff.custom(0,1);

We’re outside the conditional now. As a recap, we:

• Don’t know if el exists.
• May not have defined the nu variable.

But screw that, we’ll use both! Although, if you write modern JS, you may be confused on how we’re using nu and it not be a syntax error. nu is only defined in nested scopes — scopes we have already left, how are we possibly using it?

Welcome to non-'use strict' JS. Using an undefined variable is fine. Additionally, even in “strict” mode, var doesn’t follow the usual scoping rules. Instead, var is scoped to the function. It doesn’t matter where you use it, the resulting variable will be accessible globally across the function. Both let and const don’t do this. Don’t use var. There isn’t any reason to use it anymore.

var el = $("prw54354");

Oh God no, not again. Wait, are they just… redeclaring a new variable called el? THEY ARE. I hate old JS. This is why people pick on JS so hard, because you used to see old code like this everywhere.

if(r.type != "S") {
  if (el) {
    var nu = el.innerHTML;
    nu = nu.replace(/\+/, '');
    nu = nu * 1 + 1 * r.points;
    if (nu > 0) {
      nu = '+' + nu;
    }
  }
}

This is the same thing. Oh, goddamnit the code is duplicated.

Spoiler alert: What follows is a repeat of what I’ve explained before. I don’t mean in concept, I mean that the code is straight-up duplicated. A total of three elements are “processed” like this, each one done in a slightly different way. The differences can be found in the respective else branches, and they are totally minor and uninteresting.

---

As you can probably tell, this function is horrifyingly bad. Unfortunately, It’s not at all alone. The entire codebase is like this, both the JS frontend and the PHP backend. We’re not even sure which of the two is the worst. With the JS at least, we have deemed it so spectacularly bad that we’re just replacing the entirety of the frontend with a new, clean, and modern one.

Realize that when you use Wikidot right now, you are depending on this nonsensical code to work. This code is in use right now, more than a decade after it was created. It’s actually astounding that Wikidot works at all, and that people can still use it. It urgently needs replacement, and we’re here to do that, however traumatizing that may be.

Anyways, that’s all. The moral of the story is never, ever write code like this. I hope you enjoyed this article as much as I didn’t enjoy writing it.