OLog Userscript - Logging messages, planets and researches

For the online text-based browser game OGame I am working on an application with as aim to assist the users where possible, for this I have a server-side part and a client-side part, the respective repositories can be found here for the server-side part and here for the client-side part.

The client-side part is implemented via an userscript, it has been tested to work with GreaseMonkey in Firefox.

The script currently offers the following functionality:

• Setting the server URL via a settings page.

• Retrieving special report keys from the players' inbox, combat, espionage, missile, and recycle messages have their own report keys.

• Retrieving the planets of the user

• Retrieving the research levels of the user

I'd like the review to focus in particular on maintainability since I am new to user scripts. Also, please note that the structure of the content on the web page cannot be changed, as example some div' classes will be named weirdly.

The userscript:

```
// ==UserScript==
// @name OLog
// @namespace http://www.olog.com/
// @description OLog Userscript
// @downloadURL https://github.com/skiwi2/OLog-Userscript/raw/master/olog.user.js
// @updateURL https://github.com/skiwi2/OLog-Userscript/raw/master/olog.user.js
// @version 0.3pre
// @include http://s-.ogame.gameforge.com/game/*
// @grant GM_xmlhttpRequest
// @grant GM_getValue
// @grant GM_setValue
// ==/UserScript==

"use strict";

var oLogInstanceUrl = getSetting("settings.ologinstanceurl", "http://localhost:8080/");

var menuTable = document.getElementById("menuTable");

menuTable.insertAdjacentHTML("beforeend", '' +
'\n' +
' \n' +
' OLog Settings\n' +
' \n' +
'\n');

var oLogMenuLi = document.getElementById("ologMenuLi");
oLogMenuLi.addEventListener("focus", function(c) {
if (b(c.target).closest(".dropdown").length == 0) {
b(".currentlySe

Your code overall is a bit hard to follow (due to it's size), but it is really clean.

I really like the idea of your project, and may be helpful to many.

But lets review the code!

• You have mixed quotes everywhere. But you are consistent about that.

You use single-quotes for multi-line strings and double-quotes for everything else.

-
Your multi-line strings have this structure:

''+
'    \n'+
'        some cool content\n'+
'    \n'+
''

There's a few things you can improve:

• Get rid of those empty strings

• Place the HTML intentation outside the string.

This will allow code minimifiers to reduce the code even further, and removes needless whitespace from your strings.

• Get rid of those \n. It's useless.

It won't improve the HTML readability, since you can see it pretty well formatted on an element inspector.

• On your getSettings and setSettings, you have a repeated line of code.

You can easily move it to a new function getFullKey().

-
Instead of this:

function getWindowVariable(name) {
    return window.eval(name);
}

You could do this:

function getWindowVariable(name) {
    return Function("'use strict'; return this." + name + ";")();
}

This will prevent lots of security issues, but won't prevent others.

One of the issues is that your functions will be harmful if someone tries to run getWindowVariable('while(1);').

With your version, it will block the browser, but, with this new version, it will throw a ReferenceError, if there's no while method on window.

Or, you can use this slower method:

function getWindowVariable(name) {
    var window = Function("return this;")();
    var props = name.split(".");
    var current = window[props.shift()];
    while(props.length) {
        if(!(props[0] in current))return;
        current = current[props.shift()];
    }
    return current;
}

This is completelly safe and may prevent some XSS vulnerabilities that you may find. While my first version would still crash if you run getWindowVariable('a,+function(){while(1);}();'), this one is totally safe.

It simply splits the variable name by . and tries to see if each "piece" is a property on the "current" object. It tries to return undefined as soon as possible, reducing the number of checks and speeding up the code.

-
On your showOLogSettings(), you have this line:

for (var i = 0; i < menuTableLiList.length; i++) {

You should always store the length of an array as a variable. Like this:

for (var i = 0, length = menuTableLiList.length; i < length; i++) {

This will speed up your code by quite a bit. Reading properties from an object is a lot slower than reading a local variable.

Everytime you have a for loop with the length of an array, you should always store the length in a local variable. And you have a few of those around.

It seems that the O.P. doesn't believe in my claim that storing the length in a local variable doesn't improve performance.

I've built a very basic test bench, where you can order to run a few performance tests.

It isn't perfect, but it is enough to give an idea that I'm not entirelly wrong about this. Depending on the size of the array, it is faster to store the length in a variable. At least on IE and Google Chrome 47.0.2526.106, on my hardware.

Here's the test script:



window.testBench = {
tests: {
'check length on loop': function(array){
for(var i = 0; i 0)
{
testBench.startTests(turns);
}
}
}
html, body {
font-family: sans-serif;
}




For an accurate time, open the console (F12)



-
Reading through your code, you have the following function:

function postData(object) {
    addPlayerData(object.data);
    console.log(JSON.stringify(object.data));
    GM_xmlhttpRequest({
        method: "POST",
        url: oLogInstanceUrl + "api/userscript/" + object.endpoint,
        data: JSON.stringify(object.data),
        headers: {
            "Content-Type": "application/json"
        },
        onload: function(response) {
            console.log("load");
        },
        onerror: function(response) {
            console.log("error");
        }
    });
}

Which looks good, but here is what you can improve in it:

-
You log into the console the result of console.log(JSON.stringify(object.data));.

Reading through the documentation of GM_xmlhttpRequest(), you will see that it has an aditional context property, that will have all the properties you've sent.

This means: Remove that line, it isn't doing much there.

-
Your console usage is pretty weak. You aren't using it to it's potential.

You simply log load or error, which is useless for debugging.
Try this:

```
function postData(object) {
addPlayerData(object.data);

var url = oLogInstanceUrl + "api/userscript/" + object.endpoint;

GM_xmlhttpRequest({
method: "POST",
url: url,
data: JSON.stringify(object.data),
headers: {

StackExchange Code Review Q#115067, answer score: 7