4
\$\begingroup\$

Any suggestions on how to shorten this would be greatly appreciated. This code takes three parameters through the URL and displays a page with targeted ads. I especially need help on how to cut down the three sections where I replace %20 with spaces. Also, here is the URL with sample parameters - http://timtechsoftware.com/ad.html?file_url=URL?file_name=FILE?keyword=KEY

This is the code of ad.html:

<!DOCTYPE html>
<html>
<head>
<title>Download File</title>

<script>
  (function(i,s,o,g,r,a,m){i['GoogleAnalyticsObject']=r;i[r]=i[r]||function(){
  (i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o),
  m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;m.parentNode.insertBefore(a,m)
  })(window,document,'script','//www.google-analytics.com/analytics.js','ga');
  ga('create', 'UA-41582851-1', 'timtechsoftware.com');
  ga('send', 'pageview');
</script>

</head>
<body><div align="center">

<script type="text/javascript"><!--
google_ad_client = "ca-pub-1582371570610820";
/* Page Link Medium */
google_ad_slot = "3981698399";
google_ad_width = 468;
google_ad_height = 15;
//-->
</script>
<script type="text/javascript"
src="http://pagead2.googlesyndication.com/pagead/show_ads.js">
</script>

  <script>
    function getParam(paramName){                //Look for parameters
  var prmstr = window.location.search.substr(1);
  var prmarr = prmstr.split ("?");
  var params = {};

  for ( var i = 0; i < prmarr.length; i++) {
     var tmparr = prmarr[i].split("=");
     params[tmparr[0]] = tmparr[1];
  }
  return params[paramName];                      //Return the requested parameter
}

var keyword = getParam('keyword');
var intIndexOfMatch = keyword.indexOf( "%20" );
while (intIndexOfMatch != -1){               // Loop over the string value replacing out each matching substring.
keyword = keyword.replace( "%20", " " )      // Relace out the current instance.
intIndexOfMatch = keyword.indexOf( "%20" );} // Get the index of any next matching substring.

var file_name = getParam('file_name');
var intIndexOfMatch = file_name.indexOf( "%20" );
while (intIndexOfMatch != -1){                 // Loop over the string value replacing out each matching substring.
file_name = file_name.replace( "%20", " " )    // Relace out the current instance.
intIndexOfMatch = file_name.indexOf( "%20" );} // Get the index of any next matching substring.

var file_url = getParam('file_url');
var intIndexOfMatch = file_url.indexOf( "%20" );
while (intIndexOfMatch != -1){                // Loop over the string value replacing out each matching substring.
file_url = file_url.replace( "%20", " " )     // Relace out the current instance.
intIndexOfMatch = file_url.indexOf( "%20" );} // Get the index of any next matching substring.

    document.write('<p style="text-align: center;"><a href="' + file_url + '">Download ' + file_name + '</a>' + 
'<br/><br/>Tagged: ' + keyword + '</p>'); //Write the page
  </script>
  <script type="text/javascript"><!--
google_ad_client = "ca-pub-1582371570610820";
/* Rectangle Large */
google_ad_slot = "9662168393";
google_ad_width = 336;
google_ad_height = 280;
//-->
</script>
<script type="text/javascript"
src="http://pagead2.googlesyndication.com/pagead/show_ads.js">
</script></div>
</body>  
</html>
Improve this question
\$\endgroup\$
0

1 Answer 1

Reset to default
2
\$\begingroup\$

Your getParam code has a bug and cannot possibly work..

function getParam(paramName){                //Look for parameters
  var prmstr = window.location.search.substr(1);
  var prmarr = prmstr.split ("?"); //This has to be ampersand!!
  var params = {};

  for ( var i = 0; i < prmarr.length; i++) {
     var tmparr = prmarr[i].split("=");
     params[tmparr[0]] = tmparr[1];
  }
  return params[paramName];                      //Return the requested parameter
}

This function could be shorter if you skipped a few intermediary and some unnecessary steps:

  • There is no need to assign anything to an object, and hence to create that object
  • You only split out prmstr, no need to put that in a var
  • You could return early out of the for loop

That would give something more like

//Look for parameters
function getParam( s )
{                
  var a = window.location.search.substr(1).split("&"), i = a.length, pair;
  while(i--)
  {
    pair = a[i].split("=");
    if( pair[0] == s )
      return pair[1];
  }
}

or, more Golfic

function getParam( s )
{                
  return location.search.substr(1).split(s+"=")[1].split("&")[0]
}

Then, someone clearly copy pasted 3 times the same piece of code and made minor changes. You should have 1 generic function, called 3 time instead. Or, realize that this code is a bad replacement of unescape and just call that. The reason the replacement is bad is that there are lot more characters out there than just %20.

var keyword = unescape( getParam( 'keyword' ) ),
    file_name = unescape( getParam( 'file_name' ) ), 
    file_url = unescape( getParam( 'file_url' ) );

Furthermore, you assign google_ad_client = "ca-pub-1582371570610820"; and google_ad_slot = "9662168393"; twice.

Be aware , document.write, I have been told by reliable sources, should only be used when standing inside a protective summoning circle, lest greater demons devour you. Instead you should have the link tag in your HTML and then find the link in your js and fill in the anchor and caption.

Finally, this is not very good code, I hope the server side code that downloads the file is of higher quality and will not allow path manipulation or path traversal.

Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ I don't get what you mean by this code being dangerous, but everything else is great. Especially unescape! \$\endgroup\$
    – Timtech
    Dec 24, 2013 at 13:15
  • \$\begingroup\$ You are writing out URL parameters, think about what happens if a malicious person puts a closing link tag and then javascript, that javascript could navigate around your site and do who knows what ( For more info, see owasp.org/index.php/DOM_Based_XSS ) \$\endgroup\$
    – konijn
    Dec 24, 2013 at 13:47

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.