11
\$\begingroup\$

Is there anything that I could have done better in this code?

<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>Quotes</title>
<style>
body { color: #333; font: 20px georgia; }
em { color: #555; font-size: 90%; }
</style>
</head>
<body>
<article id="quotes"></article>
<script>
(function() {
  "use strict";

  var quotes = [
    ["Stay Hungry. Stay Foolish.", "Steve Jobs"],
    ["Good Artists Copy, Great Artists Steal.", "Pablo Picasso"],
    ["Argue with idiots, and you become an idiot.", "Paul Graham"],
    ["Be yourself; everyone else is already taken.", "Oscar Wilde"],
    ["Simplicity is the ultimate sophistication.", "Leonardo Da Vinci"]
  ].sort(function() {
    return 0.5 - Math.random();
  }),
    quotesHTML = "";

  for (var i = 0; i < quotes.length; i++) {
    quotesHTML += "<p>&ldquo;" + quotes[i][0] + "&rdquo; &mdash; <em>" + quotes[i][1] + "</em></p>";
  }
  document.getElementById("quotes").innerHTML = quotesHTML;

}());

</script>
</body>
</html>
\$\endgroup\$

3 Answers 3

Reset to default
8
\$\begingroup\$

I think your code looks pretty good already. It follows conventions and your indentation and variable names are precise. There's not much to review.

But if you're targeting modern browsers (IE9+) I would suggest a different approach using map and would separate the random logic into a function. I'd also declare the element up top so I know at first glance that the code is working with the DOM. It may seem like more code at first but it's good for code reuse, plus I think it reads better; the intent is more clear:

(function() {
  "use strict";

  var el = document.getElementById("quotes");

  var quotes = [
    ["Stay Hungry. Stay Foolish.", "Steve Jobs"],
    ["Good Artists Copy, Great Artists Steal.", "Pablo Picasso"],
    ["Argue with idiots, and you become an idiot.", "Paul Graham"],
    ["Be yourself; everyone else is already taken.", "Oscar Wilde"],
    ["Simplicity is the ultimate sophistication.", "Leonardo Da Vinci"]
  ];

  function rand(xs) {
    return xs.slice(0).sort(function(){
      return .5 - Math.random();
    });
  }

  function quote(q) {
    return "<p>&ldquo;"+ q[0] +"&rdquo; &mdash; <em>"+ q[1] +"</em></p>";
  }

  el.innerHTML = rand(quotes).map(quote).join('');

}());
\$\endgroup\$
8
\$\begingroup\$

Based upon Jiving's answer and his markup, I'd suggest moving the quote signs to the CSS. For this purpose I added p tags inside blockquote. I also changed the author class to source.

"<blockquote class="random-quotes"><p>"+ q.quote +"</p><footer class="source"> &mdash;"+ q.author +"</footer></blockquote>"

(/!\ Note: Since the p tags are hard coded into my example, this would only work with single paragraph quotes. I don't know if this is enough. If not, you'll need to pass the p tags with the quotes.)

CSS:

blockquote p:first-of-type:before {
    content: "\201C";
}

/* Selecting only the last paragraph for when there are multiple paragraphs */
blockquote p:last-of-type:after {
    content: "\201D";
}

blockquote p:last-of-type {
    margin-bottom: 0;
}

.source {
    display: block;
}
\$\endgroup\$
3
  • \$\begingroup\$ Why :last-of-type when there's only one p? \$\endgroup\$
    – Jivings
    Commented Feb 9, 2014 at 17:32
  • \$\begingroup\$ @Jivings That's for the edge case when there are multiple paragraphs. One usually wants the margin between the paragraphs, but not between paragraph and source. Also one only needs the quotation marks on the last paragraph. Ah, wait a sec... gonna need a :first-of-type for the open quote as well then. ;) \$\endgroup\$
    – kleinfreund
    Commented Feb 9, 2014 at 18:13
  • \$\begingroup\$ That makes more sense :) \$\endgroup\$
    – Jivings
    Commented Feb 9, 2014 at 18:40
7
\$\begingroup\$

The only thing I'd change is your data structure. I think it would work better as an array of objects, then you can access the quote and author without indexing an array.

var quotes = [
  { 
    quote: "Stay Hungry. Stay Foolish.", 
    author: "Steve Jobs"
  },
  [...]
];

[...]

"<blockquote>&ldquo;"+ q.quote +"&rdquo; <footer class="author"> &mdash;" 
   + q.author +"</footer></blockquote>"

Also, maybe use a <blockquote> element instead of a <p> for the correct semantics.

\$\endgroup\$
6
  • \$\begingroup\$ The source of a quotation should be wrapped with a footer element inside the blockquote. Also there should be no space between the em dash and the author. \$\endgroup\$
    – kleinfreund
    Commented Feb 9, 2014 at 14:50
  • \$\begingroup\$ @kleinfreund Done. \$\endgroup\$
    – Jivings
    Commented Feb 9, 2014 at 15:18
  • \$\begingroup\$ Plus what I was trying to say: author is inappropriate here. It's not an author, but the source of the quote. \$\endgroup\$
    – kleinfreund
    Commented Feb 9, 2014 at 15:27
  • \$\begingroup\$ @kleinfreund What's the difference? \$\endgroup\$
    – Jivings
    Commented Feb 9, 2014 at 15:28
  • \$\begingroup\$ Tried googling that, but apparantly there isn't even an author element.^^ \$\endgroup\$
    – kleinfreund
    Commented Feb 9, 2014 at 15:31

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.