1
\$\begingroup\$

I'm working on JavaScript bookmarklet that lets users send a text and link to a site.

This is the bookmarklet:

javascript:(function(){new_script=document.createElement('SCRIPT');new_script.src='http://bookmarklet.example.com/js/bookmarklet.js?v=1';document.getElementsByTagName('head')[0].appendChild(new_script);new_script.type='text/javascript';})();

This is the JavaScript for it:

if(frames.length>0){
      F=' (Frames not supported)'
  } else {
      F=''
  }
  Q=document.getSelection();
  if(!Q){
 void(Q=prompt('Enter a text...'+F+'',''))      
  };
  if(Q){
      void (window.open('http://www.example.com/add?url='+encodeURIComponent(window.location)+'&text='+escape(Q)+'&title='+encodeURIComponent(document.title)))
  }

It's the first time I'm using JavaScript, so I would love to hear your feedback on the code. Is there anything that needs to be optimized?

Thanks. Patrick

\$\endgroup\$
  • \$\begingroup\$ Where's the void() function defined and why are you using it? \$\endgroup\$ – jfriend00 Apr 24 '13 at 14:19
1
\$\begingroup\$

The loader could be simplified to the following:

javascript:(function () {document.getElementsByTagName('head')[0].appendChild(document.createElement('script')).src = 'http://bookmarklet.example.com/js/bookmarklet.js?v=1'}());
  • No more variables. By the way, you forgot to use var. It causes the variable to be declared as a global.
  • Browsers, by default, parse script tags as JavaScript, thus the type can be omitted.

And in your script body, as far as I can understand, it's a script that gets a selected text or, if none selected, lets the user manually input the text and sends it to a server. Thus we have:

//remember that you are loading this script into the global scope. Protect your 
//variables by enclosing it in a closure
(function () {
      //a ternary if is a short 2-way evaluation (as far as reading is concerned)
  var F = (frames.length > 0) ? ' (Frames not supported)' : '',
      //the OR operator (`||`) could also be used to evaluate stuff
      //here, it returns either document.getSelection(), 
      //or if non existent, a prompted value
      Q = document.getSelection() || prompt('Enter a text...' + F + '', '');

  //then we execute your command
  window.open('http://www.example.com/add?url=' + encodeURIComponent(window.location) + '&text=' + escape(Q) + '&title=' + encodeURIComponent(document.title))

}());

As an additional tip, I suggest you load the url to a hidden iframe for a cleaner interface. You could add listeners (there are hacks for cross domain iframe messaging), to detect a success and show a confirmation UI afterwards.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Awesome, thanks for your feedback and suggestions! Quick question: I tried the bookmarklet in the Android browser (Android 4) but it's not working. The site loading bar starts but stops at ~20%. Is there a way to make it work for Android? javascript:(function(){new_script=document.createElement('SCRIPT');new_script.src='http://bookmarklet.example.com/js/bookmarklet.js?v=1';document.getElementsByTagName('head')[0].appendChild(new_script);new_script.type='text/javascript';})(); \$\endgroup\$ – Patrick Apr 25 '13 at 17:16
  • \$\begingroup\$ @Patrick Head over to StackOverflow if your code doesn't work on Android. \$\endgroup\$ – Joseph Apr 25 '13 at 18:08

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.