0
\$\begingroup\$

I am a part of a student project to build a website that offers users a way to search for items in an online library catalog that are all part of some collection. The details of the project are not super important. I am responsible for writing the "backend", which in this case is just writing a function to send a request to the library's RESTful API and returning JSON objects. The front end team will use the JSON objects returned to populate a results page built with React. This is my first serious project with JS and I want to make sure I am doing things correctly. Below is my ApiWrapper.js file.

var $ = require('jquery')

class ApiWrapper{
    static makeCall({subject="", topic="", rights="", title="", format="", collection="", state="", creator="", date="", other="", language="", page=""} = {}) {
        // sends a request to the dpla api and returns a list of json objects
        // example call: ApiWrapper.makeCall({language:"english"})
        var url = "https://api.dp.la/v2/items?"
        var apiKey = "&api_key=foo"
        var parameters = "sourceResource.description=some+collection"
        var results

        if(subject !== ""){
            subject = String(subject)
            subject = subject.split(" ").join("+")
            // replaces all the spaces with + 
            parameters = parameters + "&sourceResource.subject=" + subject
        }
        if(rights !== ""){
            rights = String(rights)
            rights = rights.split(" ").join("+")
            parameters = parameters + "&sourceResource.rights=" + rights
        }
        if(title !== ""){
            title = String(title)
            title = title.split(" ").join("+")
            parameters = parameters + "&sourceResource.title=" + title
        }
        if(format !== ""){
            format = String(format)
            format = format.split(" ").join("+")
            parameters = parameters + "&sourceResource.format=" + format
        }
        if(collection !== ""){
            collection = String(collection)
            collection = collection.split(" ").join("+")
            parameters = parameters + "&sourceResource.collection=" + collection
        }
        if(state !== ""){
            state = String(state)
            state = state.split(" ").join("+")
            parameters = parameters + "&sourceResource.spatial.state=" + state
        }
        if(creator !== ""){
            creator = String(creator)
            creator = creator.split(" ").join("+")
            parameters = parameters + "&sourceResource.creator=" + creator
        }
        if(date !== ""){
            date = String(date)
            date = date.split(" ").join("+")
            parameters = parameters + "&sourceResource.date.displayDate=" + date
        }
        if(other !== ""){
            other = String(other)
            other = other.split(" ").join("+")
            parameters = parameters + "&q=" + other
        }
        if(language !== ""){
            language = String(language)
            language = language.split(" ").join("+")
            parameters = parameters + "&sourceResource.language=" + language
        }
        if(page !== ""){
            page = String(page)
            page = page.split(" ").join("+")
            parameters = parameters + "&page=" + page
        }

        url = url + parameters + apiKey
        $.getJSON(url, function(result){
            results = result.docs
        });
        // getJSON directly returns the json object returned from the dpla api in result
        // results returns the array contained in the docs field of the response
        return results
    }
}

export default ApiWrapper;

Here is my test code/example of how I expect my code to be called:

var $ = require('jquery')
$.ajaxSetup({
  async: false
});

var results = ApiWrapper.makeCall({language:"english"})
console.log(results)

This is functional, but I am not sure how elegant/"correct" it is. Are there standard practices I am ignoring? Is there a more JS-ic way to do this (a la doing things the pythonic way)? Currently I'm returning an array of JSON objects to be parsed by the front end guys. Is there a friendlier way to return the data to be used in a React UI?

\$\endgroup\$
1
\$\begingroup\$

Yessir, few improvements you can make...

String conversion

subject = String(subject)

This is not necessary. It's just more stuff you're making the computer do. If the value is not a string already it will be implicitly converted when you concat it to a string or use it in a string function like the one I mention below...

url encoding

subject = subject.split(" ").join("+")

The browser will replace spaces in the url automatically, so this is not nessecary, however, what if someone passes in "blahblah#importantstuffhere" for that param? The browser will ignore everything after the hash unless you properly urlencode it. JS has a built in url encode function, behold:

subject = encodeURIComponent(subject)

Building strings

parameters = parameters + "&sourceResource.creator=" + creator

When you build a string like this you are loading the entire string into memory every time you add to it. In this case it's likely not going to be a big deal, but it's generally a good idea to use a buffer to build your strings, which basically means push()ing string segments into an array and then join()ing them together when you have all the parts ready, eg..

var parameters = ["sourceResource.description=some+collection"];

if(subject !== ""){
    // ...
    parameters.push("&sourceResource.subject=" + subject);
}

// ...
url = url + parameters.join('') + apiKey

Just let jQuery do it, that's what it's there for

Now that I've scrolled down and see that you're using jQuery to make the request, I'm going to suggest that you ignore all my other advice and just let jQuery do all this for you. You can add a second parameter to the getJSON() function which is an object of data that jQuery will turn into a query string for you:

$.getJSON(
      url, 
      {
          "sourceResource.description": "some collection",
          "sourceResource.rights": rights,
          "sourceResource.title": title
          // ....
      }
      function(result){
        results = result.docs
    });

Requests are asynchronous

You're setting the your callback function that sets results = result.docs will not be called until after you have already returned the result variable. Surely you have noticed the method is always returning nothing? (which means this code is not working and technically belongs on SO, but i've already written all this)... Explaining async code is off topic for this site, but if you need help with that I would suggest talking to your teacher.

Building your props object dynamically

Just start with an empty object and build it dynamically, like this..

var props = {"sourceResource.description":"somecollection"};
if(subject !== "") props["sourceResource.subject"] = subject;
if(rights !== "") props["sourceResource.rights"] = rights;
if(title !== "") props["sourceResource.title"] = title;
// ...

$.getJSON(url, props, function(result){
    results = result.docs;
});
\$\endgroup\$
  • \$\begingroup\$ Thanks. I wasn't sure how much casting/string handling JS did on its own so I assumed I had it on my own. Stuffing everything in the jQuery parameter is the kind of style I was worried I was missing. As for the asynchronous, right now we have ajax's asynchronous requests turned off for the sake of getting it to work. It's not pretty but right now we're more concerned with getting something functional. \$\endgroup\$ – Son of a Sailor Oct 12 '17 at 18:23
  • \$\begingroup\$ Follow up question about doing the parameters via jQuery. How would i handle a blank parameter? If I have sourceResource.title in there like the example, but the user doesn't provide a title to search on then wouldn't that cause &sourceResource.title="" to get added to the request? This would cause the wrong results to get returned. I may actually adopt your earlier advice after all so I can make sure only the right parameters get added. \$\endgroup\$ – Son of a Sailor Oct 12 '17 at 18:43
  • \$\begingroup\$ @sommerjj I'll update my answer after my smoke break :) \$\endgroup\$ – I wrestled a bear once. Oct 12 '17 at 18:46
  • \$\begingroup\$ Appreciate it. I tinkered a little bit and I think what I need to do is create an object, check which parameters aren't blank, add those parameters to the object and then stick that object in the data parameter. \$\endgroup\$ – Son of a Sailor Oct 12 '17 at 18:55
  • \$\begingroup\$ @sommerjj - I've added some example code showing how to do that at the bottom of my answer, hope that helps.. good luck with your project \$\endgroup\$ – I wrestled a bear once. Oct 12 '17 at 19:03

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.