5
\$\begingroup\$

I have been developing a small application for a client which combines two arrays together and then creates a URL and then loads the images on the same page from the given URL.

I'd like you to look at my code and give feedback on how to make it more maintainable and robust.

function displayContent() {
    document.getElementById("findCar").onsubmit = function () {
        var registration = document.getElementById("regPlate").value,
            reference = document.getElementById("stockRef").value;
        var regArray = registration.split(''),
            refArray = reference.split(''),
            referenceNinth = refArray[10];
        reverseReg = regArray.reverse();
        var obfuscated = new Array();
        var obf_index = 0;
        for (i = 0; i <= 6; i++) {
            obfuscated[obf_index] = refArray[i];
            obf_index++;
            obfuscated[obf_index] = reverseReg[i];
            obf_index++;
        }
        obfuscated.push(referenceNinth);
        obfuscatedString = obfuscated.join("");
        var camera = new Array();
        var cameraSize = "350";
        var cam_index = 0;
        for (i = 0; i <= 1; i++) {
            if (i > 0) cameraSize = "800";
            camera[cam_index++] = cameraSize + "/f";
            camera[cam_index++] = cameraSize + "/i";
            camera[cam_index++] = cameraSize + "/6";
            camera[cam_index++] = cameraSize + "/5";
            camera[cam_index++] = cameraSize + "/4";
            camera[cam_index++] = cameraSize + "/r";
        }
        for (i = 0; i <= 11; i++) {
        var img = new Image();
        img.src = 'http://imagecache.arnoldclark.com/imageserver/" + obfuscatedString + "/" + camera[i] + "'
        document.body.appendChild(img);
    }
    return false;
    };

}
\$\endgroup\$
  • \$\begingroup\$ Could you explain better what your code does? Your first paragraph doesn't make any sense at all. "Combining two arrays" could mean anything. And what do two arrays (of what?) have to do with the creation of an URL? And what's "the same page"? The "page" of the URL? The page in which your script is running? \$\endgroup\$ – RoToRa Apr 1 '14 at 23:09
1
\$\begingroup\$

From a once over:

  • obfuscating urls from the javascript side will not stop any dedicated individual
  • displayContent is an unfortunate name for the function, it does not give away that all it does it setting a listener, consider setFindCarListener
  • var obfuscated = [] is considered better than var obfuscated = new Array();
  • It is considered better form to have a single var statement
  • reverseReg is an unfortunate name, it is unclear to me what it is supposed to contain
  • Don't assign onsubmit, use addEventListener instead
  • A loop that will only run twice should get unrolled ( for (i = 0; i <= 1; i++) { ), I would put the code in the loop block inside a helper function
  • You have some indentation trouble, consider using jsbeautifier
  • obf_index++ increases obf_index after evaluating it, so you can do

    for (i = 0; i <= 6; i++) {
        obfuscated[obf_index++] = refArray[i];
        obfuscated[obf_index++] = reverseReg[i];
    }
    
  • Since you start from 0, you are really simulating push, so you could also do

    for (i = 0; i <= 6; i++) {
        obfuscated.push( refArray[i] );
        obfuscated.push( reverseReg[i] );
    }
    
  • lowerCamelCase is preferred in JavaScript so obf_index -> obfIndex -> obfuscatedIndex
  • You use both single quotes and double quotes for strings, stick to single quotes
  • Use JsHint.com, you are not declaring every variable with var
  • refArray is like reverse polish notation, postfixing the type of the variable, regChars would be a better name, it tells me that this contains the characters of the reg(istration)

All in all ( this is untested ), I would go with something like this:

function addCameraEntries( array , cameraSize ) {
    array.push( cameraSize + "/f" );
    array.push( cameraSize + "/i" );
    array.push( cameraSize + "/6" );
    array.push( cameraSize + "/5" );
    array.push( cameraSize + "/4" );
    array.push( cameraSize + "/r" );
}

function setFindCarListener() {

    document.getElementById("findCar").addEvenListener( "submit", function onSubmit() {
        var registration = document.getElementById("regPlate").value,
            reference = document.getElementById("stockRef").value,
            referenceChars = reference.split(''),
            ninthReference = referenceChars[10];
            registrationChars = registration.split('').reverse(),
            obfuscated = [],
            camera = [];
        for (i = 0; i <= 6; i++) {
            obfuscated.push( referenceChars[i] );
            obfuscated.push( registrationChars[i] );
        }
        obfuscated.push(ninthReference);
        obfuscatedString = obfuscated.join("");
        addCameraEntries( camera , '350' );
        addCameraEntries( camera , '800' );
        for (i = 0; i <= 11; i++) {
            var img = new Image();
            img.src = 'http://imagecache.arnoldclark.com/imageserver/" + obfuscatedString + "/" + camera[i] + "'
            document.body.appendChild(img);
        }
        return false;
    };

}
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.