5
\$\begingroup\$

I have built a random machine generator website. It fetches the quotes from forismatic and has a button to get a new quote. When it gets a new quote, the website color changes. It also features a button to tweet the quote and a button to copy to clipboard (using clipboard.js).

//Needs JQuery
function getNewQuote(){
  //Changes the text of elements with class .quote-text
  //and .quote-author to have the corresponding
  //values on the json
  var leftQuoteIcon = "<i class='fa fa-quote-left'></i> ";
  //callback function that actually updates the DOM
  function updateQuote(json){
    $(".quote-text").fadeOut(1000, function() {
      $(this).html(leftQuoteIcon+json.quoteText);
    }).fadeIn(1000);

    var author = json.quoteAuthor;
    if (author === ""){//check if author is empty
      author= "Unknown";
    }
    $(".quote-author").fadeOut(1000, function() {
      $(this).text("-- "+author);
    }).fadeIn(1000);
    colorChange();
  }

  //makes json request
  $.getJSON("https://crossorigin.me/http://api.forismatic.com/api/1.0/",
            {_: new Date().getTime(), //to prevent caching http://stackoverflow.com/a/31948654/1952996
            method: "getQuote",
            format: "json",
            lang : "en"},
            updateQuote);
}

//Open new window with tweet ready with current quote
function tweet(){
  var author = $(".quote-author").text();
  var quote = $(".quote-text").text();
  window.open("https://twitter.com/intent/tweet?text="+quote+" "+author+"&hashtags=FamousQuotes");
}

function colorChange(){
  var colors = ["#E74C3C", "#9B59B6", "#3498DB", "#1ABC9C", "#27AE60",
                "#F1C40F", "#D35400", "#34495E", "#797D7F"];
  var newColor = colors[Math.floor(Math.random()*(colors.length))];
  //Change color of body, buttons and text
  $("body").css("background-color", newColor);
  $(".button-colored").css({"background-color": newColor,
                           "border-color": newColor});
  $(".quote").css("color", newColor);

}



$(document).ready(function(){
  getNewQuote();//Get first quote for page load

  $("#newquote").on("click", getNewQuote);//Associate getNewQuote with button

  $("#twitter-share").on("click", tweet);

  //Copy to clipboard
  new Clipboard("#copy-clipboard");

  $("#copy-clipboard").attr("data-clipboard-target", ".quote");
});
\$\endgroup\$
  • \$\begingroup\$ I'm not sure what your question is? do you want to improve on something? or you are looking for alternative solution? \$\endgroup\$ – Siobhan Aug 2 '16 at 22:46
  • \$\begingroup\$ I am looking to know if I am following best practices or if I'm introducing any unexpected bugs. I assumed that's enough reason for the scope of this website. \$\endgroup\$ – zelite Aug 3 '16 at 3:29
  • \$\begingroup\$ @TolaniJaiye-Tikolo This is codereview, I think he wants a code review ;) \$\endgroup\$ – konijn Aug 3 '16 at 15:00
2
\$\begingroup\$

From spending some time looking at your code

  • Purely style, but I think grouping statements belonging together and commenting on them sometimes makes your code easier to read

    $(document).ready(function(){
      //Take care of the initial quote
      getNewQuote();
    
      //Take care of click handlers
      $("#newquote").on("click", getNewQuote);//Associate getNewQuote with button
      $("#twitter-share").on("click", tweet);
    
      //Take care of clipboard
      new Clipboard("#copy-clipboard");
      $("#copy-clipboard").attr("data-clipboard-target", ".quote");
    });
    
  • I would move the magic constants 1000 to a properly named constant

  • A logical OR could make this look much cleaner: ( Returns expr1 if it can be converted to true; otherwise, returns expr2. ) "" and undefined cannot be converted to true.

    var author = json.quoteAuthor || "Unknown";
    $(".quote-author").fadeOut(1000, function() {
      $(this).text("-- "+author);
    }).fadeIn(1000);
    
  • Inside getNewQuote() I would have placed the JSON call on top of the code
  • You can have a list of css classes inside a jQuery call:

    $("body").css("background-color", newColor);
    $(".button-colored").css({"background-color": newColor,
                              "border-color": newColor});
    

    could be

    $("body, .button-colored").css("background-color", newColor);
    $(".button-colored").css("border-color", newColor});
    

    Though I do wonder how beautiful changing every single element to 1 color is, I would probably have gone with matching background, foreground, and text colors.

Other than that, this code is readable and maintainable.

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