Problem
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.nnAbout:nVersion: 2.0nCreator: @CodeFoxnGitHub: https://github.com/CodeF0x/image-search-telegram-botnAll 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();
Solution
// 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.nnAbout:nVersion: 2.0nCreator: @CodeFoxnGitHub: https://github.com/CodeF0x/image-search-telegram-botnAll 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