4
\$\begingroup\$

Some days ago I've written JavaScript bot for reddit. All it does is replacing not well-formatted gfycat URLs. I would love to hear feedback about this code!

const chalk = require('chalk')
const Snoowrap = require('snoowrap')
const async = require('async')

console.log(chalk.cyan.bold('GfycatDetailsConvert is booting up...'))

let lastChecked

require('dotenv').config()

if (typeof process.env.REDDIT_USERAGENT === 'undefined') {
  throw new Error('Please fill your .env file. For more information, go here: https://github.com/ImJustToNy/GfycatDetailsConvert')
}

const r = new Snoowrap({
  userAgent: process.env.REDDIT_USERAGENT,
  clientId: process.env.REDDIT_CLIENTID,
  clientSecret: process.env.REDDIT_CLIENTSECRET,
  username: process.env.REDDIT_USERNAME,
  password: process.env.REDDIT_PASSWORD
})

r.config({
  requestDelay: 2000,
  continueAfterRatelimitError: true,
  maxRetryAttempts: 5,
  debug: process.env.NODE_ENV != 'production'
})

setInterval(() => {
  r.getNew('all', {
    before: lastChecked,
    show: 'all',
    amount: 1000
  }).then(posts => {
    if (posts.length > 0) {
      lastChecked = posts[0].name

      async.every(posts, (post, callback) => {
        if (post.domain === 'gfycat.com' && /(\/[a-z][a-z])?\/gifs\/detail/g.test(post.url)) {
          post.fetch().comments.map(comment => comment.author.name).then(participants => {
            callback(null, true)
            if (!participants.includes(process.env.REDDIT_USERNAME)) {
              console.log(chalk.red(chalk.bold('Found new post: ') + post.title + ' [/r/' + post.subreddit.display_name + ']'))

              post.reply('[Proper Gfycat URL](' + post.url.replace(/(\/[a-z][a-z])?\/gifs\/detail/g, '') + ') \n\n' + '^^I\'m ^^just ^^a ^^bot, ^^bleep, ^^bloop. [^^[Why?]](https://gist.github.com/ImJustToNy/cb3457e36f22123eb93864f0af639da3) [^^[Source ^^code]](https://github.com/ImJustToNy/GfycatDetailsConvert)')
            }
          })
        }
      })
    }
  })
}, 5000)
\$\endgroup\$
  • \$\begingroup\$ Are you sure that post.fetch().comments.map(...).then(...) is correct? Looking at the documentation, it looks like it should be post.fetch().then(...). ... and the documentation is unclear how to access the comments from there. \$\endgroup\$ – Patrick Roberts Sep 2 '17 at 17:37
  • 1
    \$\begingroup\$ Ah, I figured out the documentation after looking at the source. It should be post.fetch().then(post => post.comments.map(...)).then(...) \$\endgroup\$ – Patrick Roberts Sep 2 '17 at 17:43
  • \$\begingroup\$ Oh, double correction, I just read the notice about proxy support when accessing an unknown property. TL;DR what's shown is correct, but inefficient. \$\endgroup\$ – Patrick Roberts Sep 2 '17 at 17:55
1
\$\begingroup\$

Overall, it gets the job done, but your code (especially the area I'd refer to as "pyramid code"), could use a little more organization and clarity.

Your if statement could be a little more concise, and, at the same time, more canonical about what it's actually there for:

if (!('REDDIT_USERAGENT' in process.env)) {
  ...
}

You can write concise code without using cryptic variable names. Change r to something more meaningful like robot or even just bot.

One way to resolve pyramid code is to look for places where your logic can be decoupled or flattened. While it's possible to continue to use the async dependency in order to achieve this, it's probably much easier to just use async/await since you're already using promises anyway. This will also give you an opportunity to get rid of your global variable lastChecked:

function matchesRequirements (post) {
  return post.domain === 'gfycat.com' && /(\/[a-z][a-z])?\/gifs\/detail/g.test(post.url)
}

async function tryReply (post) {
  const { comments, subreddit, title, url } = await post.fetch()
  const participants = comments.map(comment => comment.author.name)

  if (!participants.includes(process.env.REDDIT_USERNAME)) {
    console.log(chalk.red(`${chalk.bold('Found new post:')} ${title} [/r/${subreddit.display_name}]`))

    const proper = url.replace(/(\/[a-z][a-z])?\/gifs\/detail/g, '')
    const reason = 'https://gist.github.com/ImJustToNy/cb3457e36f22123eb93864f0af639da3'
    const source = 'https://github.com/ImJustToNy/GfycatDetailsConvert'

    const text = `[Proper Gfycat URL](${proper}) \n\n^^I'm ^^just ^^a ^^bot, ^^bleep, ^^bloop. [^^[Why?]](${reason}) [^^[Source ^^code]](${source})`

    return post.reply(text)
  }
}

async function pollNewPosts () {
  let lastChecked

  while (true) {
    const posts = await bot.getNew('all', {
      before: lastChecked,
      show: 'all',
      amount: 1000
    })

    if (posts.length > 0) {
      lastChecked = posts[0].name
    }

    await Promise.all(posts
      .filter(matchesRequirements)
      .map(tryReply)
    )

    await new Promise(resolve => setTimeout(resolve, 5000))
  }
}

pollNewPosts()

One of the things in this rewrite to flatten your pyramid code, I changed the order in which .fetch() is called, because the way your code accessed the comments property relied on the usage of a proxy get trap in order to work properly, which is both unnecessary and inefficient.

Lastly, I cleaned up your generated text to break it up into a few lines, otherwise it's quite unreadable. You might consider storing that text to an external template file using a minimal template engine (possibly written yourself just for this specific case) in order to avoid hard-coding it in here.

\$\endgroup\$
  • \$\begingroup\$ Have you tested this? I believe the result from fetch needs to call .json() and await that? \$\endgroup\$ – kamoroso94 Sep 2 '17 at 20:32
  • \$\begingroup\$ @kamoroso94 no I haven't \$\endgroup\$ – Patrick Roberts Sep 2 '17 at 21:38
  • \$\begingroup\$ @kamoroso94 I've tested it. It works. @PatrickRoberts Roberts I have 2 questions. First, do we really need that fetch()? If it lazy-loads everything it wants, maybe we can remove it? And second, I think you shouldn't wrap that much of a code in single if(participants.includes[...]), instead, why don't just do if(!participants.includes[...]) return? \$\endgroup\$ – ToNy Sep 2 '17 at 21:52
  • \$\begingroup\$ The point about using the fetch() is to avoid forcing the program to depend on Proxy(), which is very inefficient. As for your second point, that is a very good suggestion, as it reduces the complexity of tryReply(). I didn't intentionally avoid doing that, it just didn't occur to me the first time around since there were a few larger readability issues to deal with first. \$\endgroup\$ – Patrick Roberts Sep 2 '17 at 22:11
  • 1
    \$\begingroup\$ @PatrickRoberts Thanks for all of your advices! I've rewritten some parts of this project! I really appreciate your help! If you would like to, you can check it out here: github.com/ImJustToNy/GfycatDetailsConvert \$\endgroup\$ – ToNy Sep 2 '17 at 23:15

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.