My solution :) Is this good coding practice / anything I could have done better?

Question

/*  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?

Answer 1 · 2017-04-26T17:39:51Z

April 26, 2017 5:39pm

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);       
}

Answer 2 · 2017-04-26T17:41:05Z

April 26, 2017 5:41pm

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);
        });
    }
}

Answer 3 · 2017-05-10T01:40:30Z

May 10, 2017 1:40am

Thanks everyone, will take those into account :)

Welcome to the Treehouse Community

Looking to learn something new?

Asia Chan

Asia Chan

My solution :) Is this good coding practice / anything I could have done better?

3 Answers

Steven Parker

Steven Parker

You can make it more compact.

Thomas Nilsen

Thomas Nilsen

Asia Chan

Asia Chan