Welcome to the Treehouse Community

Want to collaborate on code errors? Have bugs you need feedback on? Looking for an extra set of eyes on your latest project? Get support with fellow developers, designers, and programmers of all backgrounds and skill levels here with the Treehouse Community! While you're at it, check out some resources Treehouse students have shared here.

Looking to learn something new?

Treehouse offers a seven day free trial for new students. Get access to thousands of hours of content and join thousands of Treehouse students and alumni in the community today.

Start your free trial

JavaScript

My solution for the final challenge - feedback wanted.

I have tried with a different approach where i use an input field instead of the prompt function to gather the users input.

I have included the HTML and JS code below. I have added some questions in the comments in the JS file and would appreciate just an overall feedback on this approach.

Here is a link to a snapshot of the workspace: https://w.trhou.se/guecjfao5l

Thanks in advance!

The HTML index.html file:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title>Students</title>
  <link rel="stylesheet" href="css/styles.css">
</head>
<body>
<h1>Students Search</h1>

<p>
  Search for a user by entering their name in the field below and click the search button.<br>
</p>


<input type="text" name="search" id="searchBox" autofocus="autofocus" >
<input type="submit" value="search" id="searchBtn">


<div id="output">
</div>

<script src="js/student_data.js"></script>
<script src="js/students.js"></script>

</body>
</html>

The students.js file:

var student; // Holds a student object in the for loops
var studentsFound = []; // Holds student objects that where found as part of the search
var searchField = document.getElementById("searchBox"); // Holds the searchBox input field
var searchBtn = document.getElementById("searchBtn"); // Hpæds the button used to initiate the search

/*sets the focus to the search input field upon loading the page.
NOTE:   I uncommented this in favour of 'autofocus' on the input field.
        I am not sure if this .focus() should go straight here or as part of some document.ready / onload function?
searchField.focus();
*/

// Add an eventlistener that listens for click event on the searchBtn. Once clicked it runs the 'searchForStudent' function.
searchBtn.addEventListener('click', searchForStudent);

/*
Walks through the students.
If one or more matches are found:
  save to arr which is printed at the end.
If no matches are found:
  print a message that no users where found.
empty the studentsFound array
*/
function searchForStudent () {
    for(i = 0; i < students.length; i++) {
      student = students[i];
      if (searchField.value.toLowerCase() === student.name.toLowerCase()) {
        studentsFound.push(students[i]);
      } else if (searchField.value.toLowerCase() !== student.name.toLowerCase()) {
        print("<i>A user with the name: <b>" + searchField.value + "</b> was not found. Please try another search.</i>");
      }
    }
    for (var i = 0; i < studentsFound.length; i++) {
      print(buildOutput(studentsFound));
    }
  studentsFound = [];
}

/*
takes an array as input and for each of the objects in the array builds
a html representation of those objects using string concatination.
the result is returned and used with the print function in the searchForStudents function
*/
function buildOutput(studArr) {
  var html = '';
  for (i = 0; i < studArr.length; i++) {
    html +=  '<small>Result #' + (i + 1) + '</small><hr>';
    html += '<p>Name: <b>' + studArr[i].name + '</b></p>';
    html += '<p>Track: ' + studArr[i].track + '</p>';
    html += '<p>Achievements: ' + studArr[i].achievements + '</p>';
    html += '<p>Points: ' + studArr[i].points + '</p>';
  }
  return html;
}

// prints the given argument to the '#output' div's innerHTML.
function print(message) {
  var outputDiv = document.getElementById('output');
  outputDiv.innerHTML = message; 
}

2 Answers

Joseph Rimar
Joseph Rimar
17,101 Points

Rasmus,

Your code looks fine and works well. You would only need to put your focus() function in a 'load' eventListener if you loaded the script at the beginning of your HTML file, in which case all your global getElementById() functions would have thrown errors as well, but since you loaded it at the end eventListener is not needed. For constructive feedback I would suggest a few changes...

First, you don't need to declare a global student variable. Your searchForStudent function already goes through each index of the passed array, setting a student variable equal to student[i] here is redundant. Simply comparing the value of students[i].name to your searchField value produces the same result.

Second, in the same function you're telling the logic to write some HTML every time students[i].name does not match your search criteria, then erasing it and writing new HTML in your second for loop if there are matches. It would be better to write 'results not found' after searching the entire array. You can do this by first checking if studentsFound has length.

if(studentsFound.length) {
  //loop through studentsFound and print results
 for (var i = 0; i < studentsFound.length; i++) {
      print(buildOutput(studentsFound));
    }
} else {
  print("<i>A user with the name: <b>" + searchField.value + "</b> was not found. Please try another search.</i>");
}

Be careful when naming your functions as well. Print() is a native js function that prints the page, so you're overriding it in this situation. It's not really important here but it's a good idea to stay away from overriding things. To be a little more modular you could also write this function to accept an element as an argument as well. That way if you want to add more divs later you don't have to write separate yet nearly identical functions for each one.

function printMessage(message, element) {
   document.getElementById(element).innerHTML = message;
}

The last thing I would suggest is really more along the lines of style. Personally I prefer using native array functions over loops where possible, as they let you do much more without having to write separate helper functions. I know you purposely passed a named function to your 'click' eventListener, that seems fine to me. Here is my solution using an anonymous one, no global variables, and native array functions.

// Add an eventlistener that listens for click event on the searchBtn. Once clicked it runs the 'searchForStudent' function.
searchBtn.addEventListener('click', function() {
  var test = document.getElementById("searchBox").value.toLowerCase();
  var outputDiv = document.getElementById('output');

  //Populate studentsFound by filtering students array to return only matches
  var studentsFound = students.filter(function(e) {
    return e.name.toLowerCase() === test;
  });

  // Reset ouputDiv on each click
  outputDiv.innerHTML = "";

  // If any match loop through matches and build output divs html
  if(studentsFound.length) {
    studentsFound.forEach(function(e, i) {
      var html = '';

      html +=  '<small>Result #' + (i + 1) + '</small><hr>';
      html += '<p>Name: <b>' + e.name + '</b></p>';
      html += '<p>Track: ' + e.track + '</p>';
      html += '<p>Achievements: ' + e.achievements + '</p>';
      html += '<p>Points: ' + e.points + '</p>';

      outputDiv.innerHTML += html;
    });
  }
  // If no matches show user no results for search were found
  else outputDiv.innerHTML = "<i>A user with the name: <b>" + test + "</b> was not found. Please try another search.</i>";
});

Again this is just personal style but doing it this way lets me keep the global namespace completely free and write the entire app in a few short lines with anonymous functions. If you had several buttons to add 'click' events to I would suggest leaving the elements as global namespace objects so you don't have to repeat getting them in every function.

Hopefully this was of some help. I think your code works great. You're definitely better than when I first started! Keep up the good work and If you have any questions please feel free to ask!

Steven Parker
Steven Parker
243,656 Points

Communicating through page elements as you have done is much more common in real-world situations.

I think the use of alerts and prompts in the course is simply because the focus is teaching JavaScript.

Thanks - i thought so as well but i just wanted to see if i could avoid the prompt.

Would you say the code is clean / OK? I took the approach of writing a function that handles the logic and then associating that with the event listener instead of putting that logic inside the anonymous function.