2
\$\begingroup\$

I'd appreciate any help just refining this working code. It has a bit of a weird context, but basically it's for a gallery style site that has hundreds of pages, but needs new navigation buttons. Unfortunately there's no way to add these buttons without adding them for over 300+ pages manually, or using a javascript like this.

Let's say we have a URL like the following (this coincides with the site's URL structure):

http:/test.com/10-funny-pictures/

The script links next button that links to http:/test.com/10-funny-pictures/2/, and hides the last button.

If the last page in the gallery is http:/test.com/10-funny-pictures/10/, then it sets the last button to link to http:/test.com/10-funny-pictures/9/. But on the last page of a gallery, there is a javascript inserted on that last page that holds the URL of a new gallery to send the user to when the click next, that URL is passed to the main JavaScript and then the next button links to that rather than http:/test.com/10-funny-pictures/11/ (which doesn't exist).

It's messy, but the JSFiddle is right here, any help making the code more efficient would be appreciated. I just kind of made it but I'm not good at doing things the best way when I code.

Last
<div id="rightcontainer"><a href="#" id="nextlink"><div id="nextbutton">Next</div></a></div>

<script type='text/javascript' onload="linkSet()">
function linkSet() {
var currentURL = location.href;
var lastURL = "";
var nextURL = "";
var lastChar = currentURL.charAt(currentURL.length-2);
var secondLastChar = currentURL.charAt(currentURL.length-3);
var oneRoot = currentURL.slice(0, -2);
var twoRoot = currentURL.slice(0, -3);
if(isNaN(lastChar) == true) {
    var leftContain = document.getElementById('leftcontainer');
    var rightContain = document.getElementById('rightcontainer');
    function styleSet() {
        leftContain.style.display = "none";
        rightContain.style.width = "100%";
        rightContain.style.float = "none";
    };
    styleSet();
    lastURL = currentURL;
    nextURL = currentURL + "2/";
} else if(isNaN(secondLastChar) == false) {
    var twoslash = currentURL.substr(currentURL.length-3);
    var twodigit = twoslash.substr(0,2)*1;
    if(twodigit == 10) {
        lastURL = twoRoot + "9/";
        nextURL = twoRoot + "11/";
    } else {
        lastURL = twoRoot + (twodigit - 1) + "/";
        nextURL = twoRoot + (twodigit + 1) + "/";
    }
} else {
    var oneslash = currentURL.substr(currentURL.length-2);
    onedigit = oneslash.substr(0,1)*1;
     if(onedigit == 2) {
        lastURL = oneRoot;
        nextURL = oneRoot + "3/";
    } else {
        lastURL = oneRoot + (onedigit - 1) + "/";
        nextURL = oneRoot + (onedigit + 1) + "/";
    }
}
if (typeof urlData !== 'undefined') {
nextURL = urlData;    
    }
document.getElementById("lastlink").href = lastURL;
document.getElementById("nextlink").href = nextURL;
}
window.onload = linkSet;
</script>

<!---
EXAMPLE OF THE FINAL GALLERY PAGE SCRIPT THAT PASSES URL VALUE ON A GALLERY'S LAST PAGE!

<script type='text/javascript'>
var urlData = "http://www.test.com/10-crazy-things/";
</script>
-->

    #lastbutton {
display: inline-block;
width: 80px;
height: 20px;
background-color: #0a72c2;
color: #ffffff;
text-decoration: none;
text-align: center;
padding: 8px;
font-size: 16px;
}

#nextbutton {
display: inline-block;
width: 80px;
height: 20px;
background-color: #0a72c2;
color: #ffffff;
text-decoration: none;
text-align: center;
padding: 8px;
font-size: 16px;
}

#leftcontainer {
margin-top: 30px;
float: left;
text-align: center;
width: 50%;
}

#rightcontainer {
margin-top: 30px;
text-align: center;
float: right;
width: 50%;
}
\$\endgroup\$
0
2
\$\begingroup\$

From a once over:

  • oneRoot and twoRoot are only needed in their respective if block, you should move the var statements to those if blocks.
  • This:

    if(twodigit == 10) {
        lastURL = twoRoot + "9/";
        nextURL = twoRoot + "11/";
    } else {
        lastURL = twoRoot + (twodigit - 1) + "/";
        nextURL = twoRoot + (twodigit + 1) + "/";
    }
    

    could simply be

        lastURL = twoRoot + (twodigit - 1) + "/";
        nextURL = twoRoot + (twodigit + 1) + "/";
    

    I am not sure why you needed to call out the (twodigit == 10 case

  • urlData <- Not sure what this does at all
  • var oneslash = currentURL.substr(currentURL.length - 2); <- You are not using this
  • onedigit = oneslash.substr(0,1)*1; <- You already captured this in lastChar, you could simply do onedigit = +lastChar;
  • You have 0 comments..
  • You did not declare onedigit with var
  • You have some indentation trouble towards the end of your code.

I have something like this: ( also on http://jsfiddle.net/konijn_gmail_com/unQQ7/ )

function linkSet() {
    var currentURL = location.href;
    var lastURL = "";
    var nextURL = "";
    var lastChar = currentURL.charAt(currentURL.length - 2);
    var secondLastChar = currentURL.charAt(currentURL.length - 3);

    if(isNaN(lastChar) === true) {
        //Hide left button, hard code right button to go to ../2
        var leftContain = document.getElementById('leftcontainer');
        var rightContain = document.getElementById('rightcontainer');
        leftContain.style.display = "none";
        rightContain.style.width = "100%";
        rightContain.style.float = "none";
        lastURL = currentURL;
        nextURL = currentURL + "2/";
    } else if(isNaN(secondLastChar) === false) {
        //The page number is a double digit, simply derive last/next
        var twoRoot = currentURL.slice(0, -3);
        var twoSlash = currentURL.substr(currentURL.length - 3);
        var twoDigit = twoSlash.substr(0, 2) * 1;
        lastURL = twoRoot + (twoDigit - 1) + "/";
        nextURL = twoRoot + (twoDigit + 1) + "/";
   } else {
       //The page number is single digit, with a special case for page 2
        var oneRoot = currentURL.slice(0, -2);
        var oneDigit = +lastChar;
        if(onedigit == 2) {
            lastURL = oneRoot;
            nextURL = oneRoot + "3/";
        } else {
            lastURL = oneRoot + (oneDigit - 1) + "/";
            nextURL = oneRoot + (oneDigit + 1) + "/";
        }
    }
    document.getElementById("lastlink").href = lastURL;
    document.getElementById("nextlink").href = nextURL;
}

If I had to write the code for a personal project, I would not distinguish between 1 char and 2 chars ( what if there are over a thousands pages ? ), I would just get the number from the URL and build the buttons from that. The code in that case would be easier to follow as well (to me at least). ( http://jsfiddle.net/konijn_gmail_com/xNQ7m/ )

function hideLeftButton() {
    var leftContain = document.getElementById('leftcontainer');
    var rightContain = document.getElementById('rightcontainer');
    leftContain.style.display = 'none';
    rightContain.style.width = '100%';
    rightContain.style.float = 'none';
}

function linkSet() {
    var currentURL = location.href,
        parts = currentURL.split('/'),
        lastURL, nextURL;

    //Get rid of "" ( because URL ends with / )
    parts.pop();          
    //Next part of the URL is the number ( or not )
    number = parts.pop();    
    //Build up the root to append numbers to
    var root = parts.join('/') + '/';

    if (isNaN(number) === true) { 
        hideLeftButton()
        lastURL = currentURL;
        nextURL = currentURL + '2/';
    } else if (number == 2) {
        lastURL = root;
        nextURL = root + '3/';
    } else {
        lastURL = root + (+number - 1) + '/';
        nextURL = root + (+number + 1) + '/';
    }
    document.getElementById('lastlink').href = lastURL;
    document.getElementById('nextlink').href = nextURL;
}
\$\endgroup\$
3
  • \$\begingroup\$ Thank you very much! This was made for gallery sets that do not ever exceed 30 pages, but from a code standpoint it does make sense to make it more universal. The urlData variable is set in a different JavaScript. On the last page of every gallery urlData is set in a JavaScript before this one and it links to another gallery. This was if you're at the end of the gallery you don't get linked to funny-pics-16 when there's only 15 pages in that gallery. The (twodigit == 10) part is not necessary I realize now, originally it was until I made the code better. \$\endgroup\$ – influential Jul 15 '14 at 4:57
  • \$\begingroup\$ I actually had to make a version that creates the buttons entirely using document.write(), of course that is not a good practice but sometimes ads on the page would take too long to load, and the script would fail since it would only load because it was using body onload to execute. I will make an updated fiddle soon and post it here again. \$\endgroup\$ – influential Jul 15 '14 at 4:59
  • \$\begingroup\$ If the ads are in their own frame, then document.write() can be acceptable. #perfmatters and all that. \$\endgroup\$ – konijn Jul 15 '14 at 13:07

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.