2
\$\begingroup\$

I'm working on a little project and although it all works, I think I might be doing it in an incorrect way. The front end consists of a simple form that gathers input data (make and color) and once submitted, it sends the input data to a POST endpoint in express via AJAX and gets filtered to return the items I need in a JSON file. Once the data posts, I use another AJAX call to get the filtered data back to my front end to display. Again this all works, but I'm posting and getting from the same route in express. Is this all good or can I accomplish this in a better manner?

Front end:

var carList = [];

form.addEventListener('submit', function(e) {
  e.preventDefault();

  var make  = document.querySelector('.make').value.toLowerCase(),
      color = document.querySelector('.color').value.toLowerCase();

  $.ajax({
    method: 'POST',
    url: '/cars',
    data: {
      make: make,
      color: color
    },
    success: function(data) {}
  });

  $.ajax({
    method: 'GET',
    url: '/cars',
    success: function(data) {
      carList = data;
      populateList();
    }
  });
});

ExpressJS:

function readJSONFile(filename, callback) {
  fs.readFile(filename, (err, data) => {
    if(err) {
      callback(err);
      return;
    }
    try {
      callback(null, JSON.parse(data));
    } catch(exception) {
      callback(exception);
    }
  });
}


let make  = '',
    color = '';

app.get('/cars', (req, res) => {
  let newList = [];

  readJSONFile('cars.json', (err, json) => {
    var items = json.cars.filter((item) => {
      if (item.type.toLowerCase() == make || item.color.toLowerCase() == color) {
        newList.push(item);
      }
    });
    if (err) throw err;
    res.send(newList);
  });
});


app.post('/cars', (req, res) => {
  make = req.body.make;
  color = req.body.color;
});
\$\endgroup\$
3
\$\begingroup\$
  1. Avoid making multiple calls to express server, you can return filtered data from the POST call. Use single AJAX POST call. As your calls are being made parallel there is no way to know if the make and color would be set when you make the GET request.

  2. Use === to strict-compare values

  3. Array.filter() returns a filtered array so you can remove the newList. Look into Destructing

    readJSONFile('cars.json', (err, json) => {
        if (err) {
            res.status(400).json({
                error: err
            })
        }
        const items = json.cars
            .filter((item) =>
                item.type.toLowerCase() === make || item.color.toLowerCase() === color
            );
        res.send(items);
    });
    
  4. Convert the callback functions to promise and chain for readability. Promisify

\$\endgroup\$
  • \$\begingroup\$ Hey thanks a lot for the reply. Your first point was my biggest concern there. I'll definitely check out the other ideas and use them. \$\endgroup\$ – justDan Apr 27 '18 at 13:23
1
\$\begingroup\$

I agree with Deep's answer - especially about just sending one POST request and having the POST endpoint return the data.

I noticed that the front end code appears to be selecting the elements that have the make and color values by class name:

var make  = document.querySelector('.make').value.toLowerCase(),
  color = document.querySelector('.color').value.toLowerCase();
  1. It would be wise to use an id and fetch those elements using document.getElementById()

    var make = document.getElementById('makeInput').value.toLowerCase();
    
  2. It would be wise to cache those DOM references once the DOM is ready in constants and then reference those constants when the form is submitted.

document.addEventListener('DOMContentLoaded', function() {
  const make = document.getElementById('make');
  const color = document.getElementById('color');
  document.forms[0].addEventListener('submit', function(e) {
    console.log('make value: ', make.value, 'color value: ', color.value);
    e.preventDefault();
  });
});
<form>
  <div>Make: <input id="make" /></div>
  <div>Color: <input id="color" /></div>
  <input type="submit"/>
</form>

\$\endgroup\$
  • \$\begingroup\$ Thanks for the input here. I didn't even think about caching those items. \$\endgroup\$ – justDan Apr 27 '18 at 13:25

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.