3
\$\begingroup\$

I have the following code below and I am trying to improve it. How can I know if there is any limitations/drawbacks writing this way and why? Any possible error conditions?

What can be improved on this code? Thoughts? Suggestions?

<!DOCTYPE html >
    <html>
        <head>
        <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
        <title>Untitled Document</title>

        <script src="http://ajax.googleapis.com/ajax/libs/jquery/1.10.2/jquery.min.js></script>

        <script language="javascript">
        $(document).ready(function() {
            $("#submit").click(function (event) { // wire up this as an onclick event to the submit button.                
                var searchTerm = $("#search").val(); // get the user-entered search term
                var URL = "http://api.flickr.com/services/feeds/photos_public.gne";
                var ID = "25053835@N03";
                var tags="&tags="+ searchTerm;
                var tagmode="&tagmode=any";
                var jsonFormat = "&format=json&jsoncallback=?";                 
                var ajaxURL= URL+"?id="+ID+tags+tagmode+jsonFormat;

                $.getJSON(ajaxURL,function(data){
                    $("h1").text(data.title);

                    $.each(data.items, function(i,photo) {

                        var photoHTML = "<h4>" +photo.tags + "</h4>";
                        photoHTML += '<a href="' + photo.link + '">';
                        photoHTML += '<img src="' + photo.media.m + '"></a>';

                    $('#photos').append(photoHTML).fadeIn(2000);
                    });
                });
            });         
       });


       //table cell selection
       $(document).ready(function(){

           var getval = function(html) {
               alert(html);
           }

          $('#tblMain td').on('click', function(){
              getval($(this).html());
              });
          });

    </script>
    </head>

    <body>
        <div align="center">
        <h2>flicker tag search</h2>
        <div>Enter Search Term</div>
        <input type="text" id=search />
        <input type="button" id=submit value="Search" /> 
        <div id="photos"></div>
    </body>
</html>
\$\endgroup\$

2 Answers 2

Reset to default
2
\$\begingroup\$

Adding in a timeout to the JSONP request is a good idea as suggested by Noval Agung Prayogo.

Other than that, there's not a lot to change...

  • $(document).ready(function () { ... }); can be written as $(function () { ... });.
  • Move everything into a single $(function () { ... });
  • Function expressions should end in a semicolon

so here:

var getval = function(html) {
    alert(html);
} // <-- there should be a semicolon here.

You may also want to consider having a function to create your ajaxUrl variable so that it can be reused elsewhere if you need to.

E.G.

// wrap in an immediately invoked function expressions (IIFE) passing in window
// (the global object in a browser).
(function (w) {
    "use strict";

    w.ns = {}; // ns will be used a namespace to avoid polluting global scope.
    w.ns.flickr = {};

    w.ns.flickr.generateUrl = (function () {
        // These wont change so keep them as constants accessed via closures
        // in the returned function below.
        var URL = "http://api.flickr.com/services/feeds/photos_public.gne",
            ID = "25053835@N03";

        return function (tags, tagMode, format, callbackName) {
            // Use some defaults to simplify calling in the simplest case.
            tagMode = tagMode || 'any';
            format = format || 'json';
            callbackName = callbackName || '?';

            return URL + '?id=' + ID + 
                '&tags=' + tags + 
                '&tagmode=' + tagMode + 
                '&format=' + format + 
                '&jsoncallback=' + callbackName;
        };
    }());
}(window));

Then in your code you can just call: 

ns.flickr.generateUrl($("#search").val());
\$\endgroup\$
4
  • \$\begingroup\$ JS doesn't need semicolons. they are optional. See: davidwalsh.name/javascript-semicolons and codecademy.com/blog/78-your-guide-to-semicolons-in-javascript . But +1 for good code example \$\endgroup\$
    – Pinoniq
    Commented Sep 23, 2013 at 9:36
  • \$\begingroup\$ @Pinoniq - Semicolons are optional in that the JS compiler is able to figure out where they should go most of the time. However, this can often lead to hard to trace bugs and unexpected behaviour. For safety's sake, most people agree that it's in your best interest to always include the semicolons. e.g. both jshint.com and jslint.com will complain about missing semicolons. \$\endgroup\$
    – RobH
    Commented Sep 23, 2013 at 10:02
  • \$\begingroup\$ You sum up my point :) semi colons are a best practice. I just wanted to point out that they are not required. \$\endgroup\$
    – Pinoniq
    Commented Sep 23, 2013 at 10:12
  • \$\begingroup\$ @RobH - Thanks looks lot cleaner. I have one question, I noticed you have something called (window) at teh end of the function. Could you explain on that and why you took that approach? \$\endgroup\$
    – user244394
    Commented Sep 28, 2013 at 13:26
1
\$\begingroup\$
  • Put your JavaScript into a separate *.js file. Avoid mixing HTML, CSS, and JavaScript.
  • Move your JavaScript out of and just before , so that JavaScript's blocking nature does not prevent the HTML from loading, if there's an exception
  • Use JSHint
  • Use functions with names instead of anonymous functions as event handlers. This improves readability, makes debugging easier and allows for testing and reuse of the event handler.

These are just a few improvements, that won't necessarily make your code run better, but they do improve maintainability.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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