2
\$\begingroup\$

I am retrieving data from a json object and even though code works I am not sure if I am following best practices.

In particular some of the objects seems too deep (like value.metadata.connections.comments.total) and not sure if there is a better way to avoid creating all my markup in JS.

(function getData() {

    $.ajax({
        url: 'data.json',
        type: 'GET',
        dataType: 'json',
        success: function(response) {
            console.log('success!');
            var fixtureHTML = '';

           for(var i = 0; i < 5; i++){
                var value = response.data[i];   

                fixtureHTML += '<div class="media"><div class="media-left">';
                //Author avatar + link
                if(value.user.pictures !== null) {
                    fixtureHTML += '<a href="' + value.user.link +'"><img class="media-object" src="' + value.user.pictures.sizes[2].link + '" alt="' + value.user.name + '"></a><p>'+ value.user.name + '</p>';
                } else {
                    fixtureHTML += '<a href="' + value.user.link +'"><img class="media-object no-thumb" src=""></a><p>'+ value.user.name + '</p>';
                }
                fixtureHTML += '</div><div class="media-body">';
                fixtureHTML += '<h4 class="media-heading"><a href="' + value.link +'">'+ value.name + '</a></h4>' + '<span class="more">' + textFormatter(value.description) + '</span>';
                console.log(value.user.name);

                //Number of plays + comments + likes inside meta-info
                fixtureHTML += '<div class="meta-info">';
                fixtureHTML +=  '<span class="comments">' + value.metadata.connections.comments.total + '</span>';
                fixtureHTML += '<span class="likes">' + abbreviateNumber(value.metadata.connections.likes.total) + '</span>';
                fixtureHTML += '<span class="plays">' + abbreviateNumber(value.stats.plays) + '</span>';
                fixtureHTML += '</div></div></div>';

            }

            // Append generated HTML to DOM
            $('.js-load').append(fixtureHTML);

            // Show more-less  content

            var showChar = 300;  // How many characters are shown by default
            var ellipsestext = "...";
            var moretext = "Show more +";
            var lesstext = "Show less -";


            $('.more').each(function() {
                var content = $(this).html();

                if(content.length > showChar) {

                    var c = content.substr(0, showChar);
                    var h = content.substr(showChar, content.length - showChar);
                    var html = c + '<span class="moreellipses">' + ellipsestext+ '&nbsp;</span><span class="morecontent"><span>' + h + '</span>&nbsp;&nbsp;<a href="" class="morelink">' + moretext + '</a></span>';
                    $(this).html(html);
                }

            });

            $(".morelink").click(function(){
                if($(this).hasClass("less")) {
                    $(this).removeClass("less");
                    $(this).html(moretext);
                } else {
                    $(this).addClass("less");
                    $(this).html(lesstext);
                }
                $(this).parent().prev().toggle();
                $(this).prev().toggle();
                return false;
            });



        }, // End of Success
        error: function() {
            console.log('errror');
        }
    });



    // Format string of text

    function textFormatter(str) {
      return str.replace(/\r?\n/g, '<br />')
        .replace(/((https?|ftp):\/\/[^\s/$.?#].[^\s\\\<]*)/g, "<a href='$1'>$1</a>");
    }

    // Format number to abbreviation
    function abbreviateNumber(value) {
        var newValue = value;
        if (value >= 1000) {
            var suffixes = ["", "k", "m", "b","t"];
            var suffixNum = Math.floor( (""+value).length/3 );
            var shortValue = '';
            for (var precision = 2; precision >= 1; precision--) {
                shortValue = parseFloat( (suffixNum != 0 ? (value / Math.pow(1000,suffixNum) ) : value).toPrecision(precision));
                var dotLessShortValue = (shortValue + '').replace(/[^a-zA-Z 0-9]+/g,'');
                if (dotLessShortValue.length <= 2) { break; }
            }
            if (shortValue % 1 != 0)  shortNum = shortValue.toFixed(1);
            newValue = shortValue+suffixes[suffixNum];
        }
        return newValue;
    }






    // Filter by user that have more than 10 likes

    var checkbox = $('.js-check');

    checkbox.on('click',function(){
        if($(this).is(':checked'))
            alert('ciao');
        else
            alert('unchecked'); // unchecked
    });


    // Filter search by text functionality

    $('.js-filter').keyup(function () {

        var value = $(this).val();

            $('.media-body').each(function() {
                if ($(this).text().search(value) > -1) {
                    $(this).parent().show();
                }
                else {
                    $(this).parent().hide();
                }
            });
    });


})(); //end of main function
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I hope you'll have good reviews of your code! \$\endgroup\$ – Marc-Andre Jun 30 '16 at 12:43
1
\$\begingroup\$

Be Consistent

  • Quotes used for strings. var fixtureHTML = ''; vs var moretext = "Show more +";.
  • Else-Statement Formatting. } else { vs putting the else { on its own line.
  • Spacing. function () { vs function (){. Sure, it's nit-picking but consistency adds significant amounts of readability. And being able to read code quickly is half the battle.
  • If-Statement Brackets. I would suggest always using them. It'll save you (or someone else) from a rookie mistake when maintaining the code. It adds consistency. etc.

Other Suggestions

  • Regex. I would suggest adding a bit of a descriptive comment above each one as to what they're doing. Regex's readability is near rock-bottom. Someone else or even you a few weeks from now might have to maintain this code and then you're going to have to spend some amount of time re-figuring out what these regex expressions do.

I know these suggestions aren't much, but every little bit helps right?

\$\endgroup\$
0
\$\begingroup\$

Minor comments:

  • CSS actually handles the display of text overflow with ellipsises https://developer.mozilla.org/en-US/docs/Web/CSS/text-overflow

  • I would use some debug function that takes and evaluates a severity before writing to console.log ( like in console.log('success!');)

  • When there is no-thumb you should still provide an alt text.

  • From a naming perspective, c and h are pretty terrible

  • Check out $.toggleClass() for the $(".morelink").click handler

  • function textFormatter could use a tad more commenting, I have no idea what it does

\$\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.