3
\$\begingroup\$

This is one of the projects on freecodecamp. I would like a review on my code. Thanks in advance.

Javascript:

var answers;

function formatSearchString() {

  var searchString = document.getElementById("searchBar").value;
  var words = searchString.split(" ");
  searchString = words.join("_");
  return searchString;

}

function getQueryData() {

  var stringToSearch = formatSearchString();
  var wikiUrl = "http://en.wikipedia.org/w/api.php?action=opensearch&search=" + stringToSearch + "&format=json&callback=wikiCallbackFunction";

  $.ajax(wikiUrl, {

    dataType: "jsonp",
    success: function(wikiResponse) {
      //alert(wikiResponse);
      //alert(wikiResponse.length); Always 4.
      //alert(wikiResponse[0]); Search String
      //alert(wikiResponse[1]); Titles
      //alert(wikiResponse[2]); Explanations
      //alert(wikiResponse[3]); Links
      answers = wikiResponse;

    }

  });

  setTimeout(formatResults, 1500);

}

function getRandomArticle() {

  var wikiUrl = "http://en.wikipedia.org/wiki/Special:Random";
  window.location = wikiUrl;

}

function formatResults() {

  $("#results").empty();

  var i = 0;
  for (; i < answers[1].length; i++) {

    var newDiv = document.createElement("div");
    newDiv.className = "searchResults";

    var titleLink = document.createElement("a");
    titleLink.setAttribute("target", "_blank");
    titleLink.setAttribute("href", answers[3][i]);
    titleLink.innerHTML = answers[1][i];

    var desc = document.createTextNode(answers[2][i]);
    var newLine = document.createElement("br");

    newDiv.appendChild(titleLink);
    var newLine = document.createElement("br");
    newDiv.appendChild(newLine);
    var newLine = document.createElement("br");
    newDiv.appendChild(newLine);
    newDiv.appendChild(desc);
    var newLine = document.createElement("br");
    newDiv.appendChild(newLine);

    var newLine = document.createElement("br");
    newDiv.appendChild(newLine);

    document.getElementById("results").appendChild(newDiv);

  }

}

document.getElementById("buttonForSearch").addEventListener("click", getQueryData);
document.getElementById("searchBar").addEventListener("keypress", function(e) {

  if (e.which == 13) {

    e.preventDefault();
    getQueryData();

  }

  return false;

});

document.getElementById("randomArticle").addEventListener("click", getRandomArticle);

CSS:

 body {
      background: radial-gradient(circle, black, white);
      height: 700px;
      background-size: cover;
      text-align: center;
    }

#searchBar {
  padding-left: 10px;
  background: black;
  color: white;
  border-radius: 7px;
  border: 2px solid black;
  height: 25px;
}

a {
  font-family: 'Tangerine', serif;
  color: grey;
  font-size: 45px;
}

#results {
  height: 700px;
  margin-left: 550px;
}

.searchResults {
  padding-left: 10px;
  padding-top: 10px;
  background: black;
  color: white;
  height: auto;
  width: 650px;
  text-align: left;
  margin-top: 10px;
  font-size: 20px;
}

#buttonForSearch {
  height: 30px;
  width: 90px;
  margin-left: 50px;
  border: 2px solid black;
  size: 14px;
  border-radius: 7px;
  color: white;
  background: black;
}

#randomArticle {
  height: 30px;
  width: 150px;
  margin-left: 50px;
  border: 2px solid black;
  size: 14px;
  border-radius: 7px;
  color: white;
  background: black;
}

h1 {
  margin-top: 50px;
  font-family: 'Tangerine', serif;
  font-size: 48px;
  text-shadow: 4px 4px 4px #aaa;
}

#searchFields {
  margin: 45px;
  text-align: center;
}

HTML:

<link rel="stylesheet" type="text/css" href="https://fonts.googleapis.com/css?family=Tangerine">

<body>

  <script src="http://code.jquery.com/jquery-latest.min.js">
  </script>

  <script src='http://okfnlabs.org/wikipediajs/wikipedia.js'></script>
  <h1>Wikipedia Search</h1>

  <div id="searchFields">
    <input type="text" placeholder="Type To Search" id="searchBar">
    <input type="button" value="Search" id="buttonForSearch"><br /><br /><br/>
    <input type="button" value="Random Article" id="randomArticle">
  </div>

  <div id="results"></div>

</body>

Full Code Here: http://codepen.io/jpninanjohn/pen/GZrzoG

|improve this question
\$\endgroup\$
  • 1
    \$\begingroup\$ Does it have a special reason why you declare the counting-variable that way : "var i = 0; for (; i < answers[1].length; i++) {" ? Instead of "for (var i = 0; i < answers.length, i++) {" ?? \$\endgroup\$ – michael.zech Jun 8 '16 at 6:21
  • \$\begingroup\$ I first typed var i=0; with the idea of using a while loop. I then changed my mind to use a for loop. So I just used the already initialized variable i \$\endgroup\$ – leoOrion Jun 8 '16 at 8:28
4
\$\begingroup\$

Here, it looks like you are trying to go around the async behaviour of AJAX:

$.ajax(wikiUrl, {

    dataType: "jsonp",
    success: function(wikiResponse) {
      //alert(wikiResponse);
      //alert(wikiResponse.length); Always 4.
      //alert(wikiResponse[0]); Search String
      //alert(wikiResponse[1]); Titles
      //alert(wikiResponse[2]); Explanations
      //alert(wikiResponse[3]); Links
      answers = wikiResponse;

    }

});

setTimeout(formatResults, 1500);

Whilst it does work, it could be better, because you're specifically saying 1500 milliseconds -- what if the request takes slightly longer?

Instead, use callbacks, like this:

$.ajax(wikiUrl, {
    dataType: "jsonp",
    success: formatResults             
});

This will call the formatResults function when the AJAX request is successful, passing the wikiResponse as a parameter. You'd also need to add answers as a parameter to the function itself:

function formatResults(answers) {
    ...
}

and then you can get rid of your var answers at the top! :)

|improve this answer
\$\endgroup\$
1
\$\begingroup\$

Looks like the code is working. So congratulations there!

Some suggestions:

  • Call formatResults in the success function itself. No need to manually call it later using setTimeout

  • If you know jQuery, then do use it instead of native JavaScript code. Why let the library go in waste? Use it more, especially in formatResults function.

  • Too fancy CSS. Preferably use Google fonts.

  • Some code can be made more concise, like directly replacing the spaces with underscores, and doing away with writing a function for redirection (instead just write an anonymous function as the event handler). If you start using jQuery, event handling would become much more convenient.

That's it, I guess. Good job!

|improve this answer
\$\endgroup\$
  • \$\begingroup\$ Thanks for the suggestions. Just one doubt, when I call formatResults directly, I only get the answers on the second click. I assumed that's because the results are not ready immediately. Hence the timeout to obtain the results and then render it on the screen. Please correct me if I'm wrong here. \$\endgroup\$ – leoOrion Jun 7 '16 at 17:39
  • 1
    \$\begingroup\$ Exactly. That's why, to make sure that answers are ready, this function should be called as a callback function (success function in this case) to the Ajax request. \$\endgroup\$ – sakshamsaxena Jun 8 '16 at 7:22
1
\$\begingroup\$

One tip: group DOM reading and writing. It can speed it up. So instead of:

var a = document.createElement('div')
foo.appendChild(a)
var b = document.createElement('div')
foo.appendChild(b)
var c = document.createElement('div')
foo.appendChild(c)
document.getElementById('output').appendChild(foo)

you should do:

var output = document.getElementById('output')

var a = document.createElement('div')
var b = document.createElement('div')
var c = document.createElement('div')

foo.appendChild(a)
foo.appendChild(b)
foo.appendChild(c)
output.appendChild(foo)
|improve this answer
\$\endgroup\$
  • \$\begingroup\$ By speed up, do you mean speed up the rendering of the elements on the page?? \$\endgroup\$ – leoOrion Jun 7 '16 at 17:36
  • \$\begingroup\$ @leoOrion Yes, the browser's rendering. \$\endgroup\$ – gcampbell Jun 7 '16 at 17:37

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.