Welcome to the Treehouse Community

The Treehouse Community is a meeting place for developers, designers, and programmers of all backgrounds and skill levels to get support. Collaborate here on code errors or bugs that you need feedback on, or asking for an extra set of eyes on your latest project. Join thousands of Treehouse students and alumni in the community today. (Note: Only Treehouse students can comment or ask questions, but non-students are welcome to browse our conversations.)

Looking to learn something new?

Treehouse offers a seven day free trial for new students. Get access to thousands of hours of content and a supportive community. Start your free trial today.

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
212,265 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,956 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 :)