Rewrote my Quote Generator: Object Oriented style. Looking for feedback with my implementation

Code Feedback
1

My work

The app is not yet finished (I have yet to add a prevent duplicate feature as well as the share to twitter implementation), but I’m more interested on how can I improve my current setup. I use two classes, RandomQuoteGenerator and QuoteAPI. They look like this:

// RandomQuoteGenerator.js
const QuoteAPI = require('./QuoteAPI.js');

class RandomQuoteGenerator {
    constructor(button, quote, author){
        this.button = button;
        this.quote = quote;
        this.author = author;
    }

    /* Initializes the app.
     * Handles click events.
    */
    start(){
        this.fetch();
        this.button.on("click", () => {
            this.fetch();
        });
    }

    /* Makes a QuoteAPI object that makes the API call*/
    fetch(){
        const API = new QuoteAPI(this);
        API.call("http://quotes.stormconsultancy.co.uk/random.json", this.parse, this)
    }

    /* Displays the content of @json on to the @ref properties. */
    parse(json, ref){
        ref.quote.text(json.quote);
        ref.author.text(json.author)
    }
}

module.exports = RandomQuoteGenerator

// QuoteAPI.js
class QuoteAPI {
    /* Makes an AJAX call using @link
     * Passes the result to @callback
    */
    call(link, callback, ref){
        $.getJSON(link,  json => {
            callback(json, ref);
        })
    }
}


module.exports = QuoteAPI

What stood out as a problem for me is that the implementation of parse method on RandomQuoteGenerator. It used to look like this:

parse(json){
   this.quote.text(json.quote);
   this.author.text(json.author);
}

BUT when passed into the call method of QuoteAPI, this is undefined! So I did a bandaide fix and added a reference parameter to parse and call to give context on what object is parse working with. Is there a cleaner way to do this?

Since I’m already making a post, any critique on my design choices? I’m always erring on the side of minimalist choices because I’m freaking terrible with colors.

Thanks a lot, campers!

EDIT: There seems to be an issue over serving the api through https. I’ll work on it!

2

There are a couple of naming choices, call and fetch that I would say are “bad”. call is a native JavaScript method on function objects, and fetch is the new AJAX API that’s replacing XHR in the browser. Using these method names is, at best, confusing for other developers, but could lead to complications as well.

I would also rethink the API class. What you have right now is a leaky abstraction. $.getJSON by itself is pretty easy to use, and by isolating it out in its own class you’ve arguably made it more difficult to use, which is the opposite of what an abstraction should do. Generally, I tend to stay away from the OO syntax in JS as it tends to weird contortions, but experimentation is a good thing. What would be better for you is to revisit this project after another few projects and reflect on what you thought worked well and what you didn’t like as much.

As for your design, I think it’s pretty great. :thumbsup:

1 Like
3

I like your direction and organization. I have to agree with @PortableStick that you could choose names that don’t collide with native Javascript functions and that you could probably move $.getJSON into a method inside RandomQuoteGenerator instead of its own class.

If you were writing your own native-javascript AJAX class then a separate class makes sense. For only one call to a well-tested function it’s a little overkill.

If you want to leave getJSON in its own helper class you could at least instantiate it in the constructor so you’re not creating a new object every time fetch() is called.

Other than that, I really like your organization.

1 Like
4

Holy smokes. I didn’t realize that they already exist, I wonder why JS didn’t throw an error at runtime.

This is pretty much what happens anytime I refactor my code. My thinking process is that since QuoteAPI does one separate thing, it deserves its own class. But the takeaway here is that one separate thing is pretty much $.getJSON itself, abstracting it away to its own Class is counter-productive (in a sense that I’m not really adding value, I’m just making a terrible re-implementation of $.getJSON)[quote=“PortableStick, post:2, topic:84727”]
As for your design, I think it’s pretty great. :thumbsup:
[/quote]

Thank you for the feedback. One last thing: is there a workaround to my current issue where Github Pages (HTTPS) blocks my api calls (served through HTTP)? I’m going to replace it with another API for now, but it would be awesome if I can use the programming quotes API.

5

I’d like to share that I solved the confusing part where I need to pass around this in the parse() method by using bind when I pass parse as an argument like this:

fetch(){
   ...
   API.call( link, this.parse.bind(this) )
}

Which is a really cool concept to me because I thought you can only use bind on function calls.

Mind showing me how this might look like?

6

Yes! Prepend a cors proxy onto your API’s endpoint. https://cors-anywhere.herokuapp.com is reliable and easy to use. It’ll look like this:

https://cors-anywhere.herokuapp.com/http://api.whereever.com/yadda?yadda=yadda
7

Sure thing!, the general idea would be making the API variable a class property so you only have to call new QuoteAPI once

class RandomQuoteGenerator {

const API;

constructor(button, quote, author){
    this.button = button;
    this.quote = quote;
    this.author = author;
    this.API = new QuoteAPI();
}

This way, the API object is only created once and can be re-used every time you call RandomQuoteGenerator.fetch()

If it’s not working like I describe please let me know and I’ll throw it into codepen to create a quick demo.

8

Neat! It’s working now :smiley: Thanks a bunch!

9

This way, the API object is only created once and can be re-used every time you call RandomQuoteGenerator.fetch()

Oh wow, that makes a lot of sense. I refactored my current structure into a single Class so I won’t be using this, but it’s a nice thing to know when working with multiple Classes. Thanks for the tip :smiley: