Node.js Pun Generator

Question

Longtime lurker here. I'm making a pun generator in JavaScript, and I was looking for some feedback/guidance. I don't have much experience building applications with Node, and so a lot of my code feels a bit like rambling. This class is meant to be used like so:

let p = new Phrase("The dog likes cheese.")
p.generatePun().then((response) => {
    console.log(response)
})

Specifically, I think the web api call could be structured better, but I don't quite know how. I'd appreciate any feedback!

Phrase.js

const util = require("./util.js");
const wordsApi = require("./rhymeApi.js");

const letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";

class Phrase {
    constructor(string, badwords = ["the", "on"]) {
        this.tokens = string.split(" ");
        this.badwords = badwords;
    }

    generatePun() {

        let nonBadWordsIndices = [];
        for (let i = 0; i < this.tokens.length; i++) {
            if (!this.badwords.includes(this.tokens[i].toLowerCase())) {
                nonBadWordsIndices.push(i)
            }
        }



        let replaceIndex = util.getRandomElement(nonBadWordsIndices);
        let wordToBePunned = this.tokens[replaceIndex];

        const isCapitalized = wordToBePunned[0] === wordToBePunned[0].toUpperCase();
        const isAllCaps = wordToBePunned === wordToBePunned.toUpperCase();
        const punctuation= wordToBePunned[wordToBePunned.length - 1];
        const isPunctuated = !letters.includes(punctuation);

        if (isPunctuated) {
            wordToBePunned = wordToBePunned.slice(0, wordToBePunned.length - 1)
        }

        return wordsApi.getRhyme(wordToBePunned).then((rhymes) => {
            let rhyme = util.getRandomElement(rhymes).word;
            if (isCapitalized) {
                rhyme = rhyme.replace(/\b\w/g, l => l.toUpperCase())
            }
            if (isAllCaps) {
                rhyme = rhyme.toUpperCase();
            }
            if (isPunctuated) {

                rhyme = rhyme + punctuation
            }



            let punnedPhrase = this.tokens.slice();
            punnedPhrase.splice(replaceIndex, 1, rhyme);
            return punnedPhrase.join(" ")

        }).catch((err) => {
            return "Error in generating pun: " + err;
        });

    }

    toString() {
        return this.tokens.join(" ");
    }
}

module.exports = Phrase;

util.js

let getRandomElement = (array) => {
    return array[Math.floor(Math.random()*array.length)];
}

//returns random int in [0, stop)
let getRandomIntInRange = (stop) => {
    return Math.floor(Math.random()*stop)
}


module.exports = {
    getRandomElement,
    getRandomIntInRange
}

rhymeApi.js

const rp = require('request-promise');

const options = {
    method: 'GET',
    uri: 'https://api.datamuse.com',
    json: true
}


let getRhyme = (word) => {
    let rhymeOptions = Object.assign({}, options);
    rhymeOptions.uri += '/words?rel_rhy=' + word;
    return rp(rhymeOptions)
}




module.exports = {
    getRhyme
}

I did this mistake as well - using let everywhere instead of const — Dejan Toteff
– Dejan Toteff, Commented Feb 19, 2018 at 12:15

Gerrit0 · Accepted Answer · 2018-02-19 17:42:55Z

This is mostly nitpicks, there really isn't anything blatantly wrong with your code. Good work!

In util.js, getRandomElement could use getRandomIntInRange - since it isn't used anywhere else, if getRandomElement doesn't use it, it should be dropped.
Avoid unnecessary mixing of let and const. Both nonBadWordsIndices and replaceIndex are not reassigned but for some reason let is used instead of const while you use const elsewhere. (Also in util.js and rhymeApi.js)
What should happen if the empty string is passed as your phrase? I suspect current behavior isn't desired.
Swallowing errors is bad 99% of the time. Libraries that swallow errors make logging errors to be able to quickly discover the problem and fix it much more difficult.
It may not be required for this use case, but when building URLs, you should always use encodeURI or encodeURIComponent as required to avoid problems like trying to get rhymes for words containing &. This is especially important if it is to be used in a web application.

The following points are more my personal preference than objectively wrong.

If supported by your environment, prefer destructuring to Object.assign. (Also, why is the /words?rel_rhy= appended here instead of being stored in the options object? There might be a good reason, but it's something to consider)
```
let getRhyme = (word) => {
    return rp({
        ...options,
        uri: options.uri + '/words?rel_rhy=' + encodeURIComponent(word)
    })
}
```
Async functions are great! Use them instead of explicitly using promises if possible.
isPunctuated would be more clearly defined with /[a-z]$/i.test(wordToBePunned), similarly, isCapitalized could be defined as /^[A-Z]/.test(wordToBePunned) These also seem like great utility methods to me, I would likely move them to a utils file if they are used anywhere else in your codebase - and maybe even if they aren't to ensure that if you need them anywhere else you don't end up forgetting you already did the work and rewriting them.
If isAllCaps is true, then isCapitalized must also be true, I would prefer to check isCapitalized only if isAllCaps is false when modifying the returned word.
Avoid double negatives, it makes code not non-harder to read. nonBadWordsIndices is equivalent to goodWordsIndices

Stack Exchange Network

Node.js Pun Generator

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Node.js Pun Generator

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions