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 :) Is this good coding practice / anything I could have done better?

/*  js/students.js  */

var students = [ 
  { 
   name: 'Asia',
    track: 'Front End Development',
    achievements: 158,
    points: 14730
  },
  {
    name: 'Bien',
    track: 'iOS Development with Swift',
    achievements: '175',
    points: '16375'
  },
  {
    name: 'Bea',
    track: 'PHP Development',
    achievements: '55',
    points: '2025'
  },
  {
    name: 'Basil',
    track: 'Learn WordPress',
    achievements: '40',
    points: '1950'
  },
  {
    name: 'Faye',
    track: 'Rails Development',
    achievements: '5',
    points: '350'
  }
];

/*  student_report.js  */

var message = "",
      name = "",
      search,
      track,
      achievements,
      points;

function print(message) {
  var outputDiv = document.getElementById('output');
  outputDiv.innerHTML = message;
}

while (true) {
    search = prompt('Search student records: type a name [David] (or type "quit" to end)');
    if (search === "quit") {
        break;
    } else { 
      for (prop in students) {
          search = search.toLowerCase(); // capitalization fool-proof
          name = students[prop]['name'].toLowerCase(); // capitalization fool-proof
          if (search === name) {
                name = students[prop]['name'];
                track = students[prop]['track'];
                achievements = students[prop]['achievements'];
                points = students[prop]['points']; 
                message += '<h2>Student: ' + name + '</h2>';
                message += '<p>Track: ' + track + '</p>';
                message += '<p>Achievements: ' + achievements + '</p>';
                message += '<p>Points: ' + points + '</p>';
                print(message);       
            } 
        }
    }
}

Code works as instructed by the instructor, although I'd like some critique and suggestions on how I can better the code, if there is any?

3 Answers

Steven Parker
Steven Parker
228,991 Points

You can make it more compact.

You asked nearly the same thing a few hours ago. But there's a few differences in this one so I'll modify my example. Just as before, there's nothing "wrong" with the code as is, but I made it a good bit more compact by:

  • eliminating variables where possible
  • iterating through the array elements directly instead of using indexes
  • using dot notation to access object members instead of bracket notation
  • eliminating "else" after an if that performs a break
  • using template interpolation to create the output string

I also slightly altered the logic by making search lower case right away (so the test for "quit" would benefit from it also) and by calling print only once after the search loop finishes.

// var students = ... (no change to students array)

function print(message) {
    document.getElementById("output").innerHTML = message;
}

var message = "";
while (true) {
    let search = prompt('Search student records: type a name [David] (or type "quit" to end)').toLowerCase();
    if (search === "quit") break;
    for (let student of students) {
        if (search === student.name.toLowerCase()) {
              message += `<h2>Student: ${student.name}</h2>
                          <p>Track: ${student.track}</p>
                          <p>Achievements: ${student.achievements}</p>
                          <p>Points: ${student.points}</p></br>`;
          } 
     }
     print(message);       
}
Thomas Nilsen
Thomas Nilsen
14,957 Points

There nothing really wrong with your current solution, but a few things can be improved.

  • I would replace 'for in' loop with a regular 'for loop'. There is nothing wrong with 'for in', but there are a couple of cases to watch out for. Read more here
  • There is no need to store everything as variables, before you add it to the message-variable.

Here is the version containing the suggestions;

/*  student_report.js  */

var message = "";

function print(message) {
    document.write(message);
}

while (true) {
    search = prompt('Search student records: type a name [David] (or type "quit" to end)').toLowerCase();
    if (search === "quit") {
        break;
    } else { 
        for(var i = 0; i < students.length; i++) {
            var student = students[i];
            if(search == student.name.toLowerCase()) {
                message += '<h2>Student: ' + student.name + '</h2>';
                message += '<p>Track: ' + student.track + '</p>';
                message += '<p>Achievements: ' + student.achievements + '</p>';
                message += '<p>Points: ' + student.points + '</p>';
                print(message);
            }
        }
    }
}

Also, If you want to practice callbacks here is another take on this;

var message = "",
    search = "";

function print(message) {
    document.write(message);
}

var searchForStudent = function(arr, name, callback) {
    arr.some(e => {
        if(e.name.toLowerCase() === name) {
            callback(e);
        }
    });
}.bind(null, students);

while (true) {
    search = prompt('Search student records: type a name [David] (or type "quit" to end)').toLowerCase();
    if (search === "quit") {
        break;
    } else { 
                //our function with callback
        searchForStudent(search, function(student) {
            message += '<h2>Student: ' + student.name + '</h2>';
            message += '<p>Track: ' + student.track + '</p>';
            message += '<p>Achievements: ' + student.achievements + '</p>';
            message += '<p>Points: ' + student.points + '</p>';
            print(message);
        });
    }
}

Thanks everyone, will take those into account :)