7
\$\begingroup\$

In my freetime, I did this little Telegram bot in Node.js that provides some basic text-based commands and one command where a user can search for an image in order for the bot to send back an image related to that word.

Is there anything about this code I can optimize regarding performance and / or code quality in general?

// Request module
const request = require('request');

// Telebot module
const Telebot = require('telebot');

// Bot
const bot = new Telebot("YOUR TELEGRAM API TOKEN");

// DOMParser
const jsdom = require('jsdom');
const { JSDOM } = jsdom;


// /start command
bot.on('/start', (msg) => msg.reply.text('Hello! For informations about usage, creator, etc., please use the command /help!'));

// /help command
bot.on('/help', (msg) => msg.reply.text('Usage:\n /imageof <your_word> sends you an random image.\nExample:\n/imageof dog - You\'ll get an random image of a dog.\n\nAbout:\nVersion: 2.0\nCreator: @CodeFox\nGitHub: https://github.com/CodeF0x/image-search-telegram-bot\nAll images are taken from https://unsplash.com.'));

// /imageof command
bot.on(/^\/imageof (.+)$/, (msg, props) => {
    request(`https://unsplash.com/search/photos/${props.match[1]}`, function (error, response) { // Get the search results of bing
        var html = new JSDOM(response.body); // Parse the response 
        var images = html.window.document.getElementsByClassName('_2zEKz'); // Get all images - in this case by class name, otherwise we would get profile pictures too
        var sources = []; // Array to pick random url from
        for (var i = 0; i < images.length; i++) { // Loop through all images and push only valid url to the array
            if (images[i].src.includes('https')) {
                sources.push(images[i].src);
            }
        }
        // Check if the array containing the url has any values
        if (typeof sources[0] !== "undefined") {
            sendPhoto(msg, sources[Math.floor(Math.random() * sources.length)]); // Random url as parmeter
        } else {
            sendError(msg, props);
        }
    });
});

// Actual function to send the photo
const sendPhoto = (msg, url) => msg.reply.photo(url); // Send the photo


// Function to send an error message
const sendError = (msg, props) => msg.reply.text(`⚠️ Sorry, I couldn't find any image for "${props.match[1]}". ⚠️`);

bot.start();
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$
// Request module
const request = require('request');

// Telebot module
const Telebot = require('telebot');

// Bot
const bot = new Telebot("YOUR TELEGRAM API TOKEN");

// DOMParser
const jsdom = require('jsdom');
const { JSDOM } = jsdom;

First of all is the comment noise. It's pretty clear what the code is already doing. You don't need to put comments on code that's already obvious. If you really need comments, save it for "weird" cases or things that is normally done one way yet you did it in another.

// /start command
bot.on('/start', (msg) => msg.reply.text('Hello! For informations about usage, creator, etc., please use the command /help!'));

// /help command
bot.on('/help', (msg) => msg.reply.text('Usage:\n /imageof <your_word> sends you an random image.\nExample:\n/imageof dog - You\'ll get an random image of a dog.\n\nAbout:\nVersion: 2.0\nCreator: @CodeFox\nGitHub: https://github.com/CodeF0x/image-search-telegram-bot\nAll images are taken from https://unsplash.com.'));

This is fine for now. But should your app grow, consider putting the messages in a dedicated object. This allows you to easily look up and modification of messaging without diving into code.

    var html = new JSDOM(response.body); // Parse the response 
    var images = html.window.document.getElementsByClassName('_2zEKz'); // Get all images - in this case by class name, otherwise we would get profile pictures too
    var sources = []; // Array to pick random url from

I find it strange that you use const on the stuff above, but var down here.

    var images = html.window.document.getElementsByClassName('_2zEKz'); // Get all images - in this case by class name, otherwise we would get profile pictures too
    var sources = []; // Array to pick random url from
    for (var i = 0; i < images.length; i++) { // Loop through all images and push only valid url to the array
        if (images[i].src.includes('https')) {
            sources.push(images[i].src);
        }
    }

Given the existence of array methods that let you transform one array to another, I find this pattern of "create array and push items from another array into it" an anti-pattern. Use array.map and array.filter instead. Also, you might want to check if https is actually the start of src.

const sources = Array.prototype.slice.call(images) // Convert NodeList to regular array.
  .filter(img => img.src.indexOf('https') === 0)   // Only https images
  .map(img => img.src)                             // Extract src
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.