4
\$\begingroup\$

The project below is about a Telegram bot that implements both inline and offline modes. The idea is to get a brief info about a book, searched by title(it's author, rating, short description). I decided to retrieve the info from GoodReads. Besides the parse implementation, I have trouble in correctly accessing the Telegram API. Every recommendation and criticism is welcomed!

The GoodReads access to retrieve data

import goodreads
from goodreads import client
import random



class GoodreadsBook():

  def __init__(self):
    self.client_key = "[data]"
    self.client_secret = "[data]"

  def authenticate(self):
    self.auth_client = client.GoodreadsClient(self.client_key, self.client_secret)

  def parse_book(self, book):
    return {
        'title': book_data.title,
        'author': book_data.authors[0],
        'rating': book_data.rating,
        'review': book_data.review,
        'cover': book_data.image_url,
        'link': book_data.link
    }

  def book(self):
    """ Get info about a random book """
    max_book_num = 10 000 000
    index = random.randint(1, max_book_num)
    book = self.auth_client.book(index)

    return parse_book(book)

  def book_search(self, q, page=1, search_field='all'):
    """ Get the most popular books for the given query. This will search all
        books in the title/author/ISBN fields and show matches, sorted by
        popularity on Goodreads.
        :param q: query text
        :param page: which page to return (default 1)
        :param search_fields: field to search, one of 'title', 'author' or
        'genre' (default is 'all')
    """
    books = self.auth_client.search_books(str(q), page, search_field)

    return map(parse_book, books)

Main implementation of the bot

import telebot
from telebot import types
import gd

token = '[data]'
bot = telebot.TeleBot(token)


@bot.message_handler(commands=['start'])  # greeting
def send_welcome(message):
    bot.reply_to(message.chat.id, "Hi! How are you?")

@bot.message_handler(commands=['help'])  # command list
def send_welcome(message):
    bot.reply_to(message.chat.id, "This bot can do: ")

@bot.message_handler(commands=['random']) # /random
def random(message):
    book = random_book()
    text = "Some text"
    bot.send_message(message.chat.id, text, disable_web_page_preview=False) 

@bot.inline_handler(lambda query: query.query == 'text')  # inline session
def query_text(query):
    raise Http404("Poll does not exist")
    book_req = book_info(query)
    try:
        result = []
        i = 0
        for book in book_req:
            result.append(types.InlineQueryResultArticle(id=i, 
                title=book[i]['title'],
                url=book[i]['link'],
                hide_url=True,
                description=book[i]['review'],
                thumb_url=book[i]['cover'], thumb_width=48, thumb_height=48
                ))
            i += 1
        bot.answer_inline_query(query.id, result)
    except Exception as e:
        print("{!s}\n{!s}".format(type(e), str(e)))

def main_loop():
    bot.polling(True)
    while 1:
        time.sleep(3)

if __name__ == '__main__':
    try:
        main_loop()
    except KeyboardInterrupt:
        print >> sys.stderr, '\nExiting by user request.\n'
        sys.exit(0)
\$\endgroup\$
  • 4
    \$\begingroup\$ Please add a description as to what your code is doing. As this is tagged programming-challenge you should add the challenge description, preferably with a link if possible. \$\endgroup\$ – Peilonrayz Oct 25 '16 at 11:01
  • \$\begingroup\$ I've initiated a redaction so that the keys cannot be found from the edit history. Also, please do not modify the original code after receiving answers. \$\endgroup\$ – Jamal Oct 25 '16 at 19:19
  • \$\begingroup\$ Please revoke your token with BotFather as soon as possible, for security reasons. Your token is still valid and open to abuse. \$\endgroup\$ – grooveplex Oct 27 '16 at 21:16
2
\$\begingroup\$
i = 0
for book in books:
    title = book[i].title
    info_dict['title'] = title

    author = book[i].authors[0]
    info_dict['author'] = author

    rating = book[i].average_rating
    info_dict['rating'] = rating

    review = book[i].description
    info_dict['review'] = review

    cover = book[i].image_url
    info_dict['cover'] = cover

    link = book[i].link
    info_dict['link'] = link
    i += 1
    book_list.append(info_dict)

I don't really get this part, either the books objects got some weird structure, or it seems like (and most likely is) that each object in this collection got also information about the whole collection. So this might be just replaced with:

for book in books:
    book_list.append({
        'title': book.title,
        'author': book.authors[0],
        'rating': book.rating,
        'review': book.review,
        'cover': book.image_url,
        'link': book.link
    })

which you might want to replace with list comprehession if you want to:

book_list = [{'title': book.title,
              'author': book.authors[0],
              'rating': book.rating,
              'review': book.review,
              'cover': book.image_url,
              'link': book.link
              } for book in books]

But if I'm wrong and this books got really weird structure you can simplify your code as:

for i, book in enumerate(books):
    book_data = book[i]
    book_list.append({
        'title': book_data.title,
        'author': book_data.authors[0],
        'rating': book_data.rating,
        'review': book_data.review,
        'cover': book_data.image_url,
        'link': book_data.link
    })

now your random_book function can be prettified a bit as:

def random_book():
    index = random.randint(1, 1000000)
    book = gc.book(index)

    return {
        'title': book_data.title,
        'author': book_data.authors[0],
        'rating': book_data.rating,
        'review': book_data.review,
        'cover': book_data.image_url,
        'link': book_data.link
    }

Now as we can see, these both two functions are having deal with same structure, so you might want to create a parse function. And in the end your code will be like:

import random
from goodreads import client

client_key = "[redacted]"
client_secret = "[redacted]"

gc = client.GoodreadsClient(client_key, client_secret)

def parse_book(book):
    return {
        'title': book_data.title,
        'author': book_data.authors[0],
        'rating': book_data.rating,
        'review': book_data.review,
        'cover': book_data.image_url,
        'link': book_data.link
    }

def book_info(title):
    books = gc.search_books(str(title), page=1, search_field='all')

    return map(parse_book, books)

def random_book():
    index = random.randint(1, 1000000)
    book = gc.book(index)

    return parse_book(book)

Also in random_book I would replace index = random.randint(1, 1000000) with a call that returns a number of books in goodreads (if there is such) so you wont get an error here.

\$\endgroup\$
  • \$\begingroup\$ @Graipher, I think having a client as global variable here is kind of fine, if this project was bigger I might have used some kind of lazy creating of this one (e.g function that returns client if it was already created or create and return one). But this is not a case for such simple project. \$\endgroup\$ – Alex Oct 25 '16 at 14:06
  • \$\begingroup\$ I would still avoid it, because it is quite unnecessary here. The function becomes a lot easier to test if it does not depend on the global state. If the project becomes bigger, I would expect these functions to become member functions of a class, accessing self.client. \$\endgroup\$ – Graipher Oct 25 '16 at 15:13
  • 1
    \$\begingroup\$ And I think your indentations of book_info and random_book are still too large (or did you mean to nest them under parse_book?) \$\endgroup\$ – Graipher Oct 25 '16 at 15:15
  • \$\begingroup\$ @Alex, Unfortunately, I can't find a call in GoodReads API to get the number of books in their repository \$\endgroup\$ – Ira Nazarchuk Oct 25 '16 at 19:42
  • \$\begingroup\$ @IraNazarchuk then try to make a call with a huge number that will lead you to error on server side. Then check the response and make a handling of it, so your code will not crash as soon as it will get it. \$\endgroup\$ – Alex Oct 26 '16 at 8:01
1
\$\begingroup\$
  • while 1: time.sleep(...) would always look bad. The example for telebot even shows to use bot.poll(...) so I'd use that unless there's some reason not to.
  • Compatibility with Python 3 is a good idea, consider not using constructs like print >> ... for forwards compatibility.
  • The raise Http404(...) in query_text looks suspicious - is the rest of the function not supposed to be run or what?

Otherwise (together with the changes from the first answer) I find the code to be readable and well structured.

\$\endgroup\$

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.