I have a function whose responsibility is to send a push notification to a user. I call this function from a view in flask framework.

I have read multiple articles around refactoring and good designs but I want to know from those who have been doing this for a while what I can improve.

Here is the function.

 def send_push(api_key, cid, username,request_signature):

        ## ---> DB CALLS
        master_db = get_db('master_db')
        cname = master_db.companies.find_one({'company_id': cid})['company_name']
        company_db = get_db(cname)
        application = company_db.applications.find_one({'app_config.api_key':api_key})

        if not application:
            #error_log_writer(cid=cid, error='API Key is invalid')
            raise InvalidApiKey('The API Key is incorrect.')

        appname = application['app_name']

        # application = company_db.applications.find_one({'app_name': appname})
        username_appname = username + '_' + appname
        redis_db = get_redis()

        ## DB CALLS
        if redis_db.hgetall(username_appname):
            # error_log_writer(cid=cid, error='The PUSH request is already in process', appname=appname)
            raise SomeCustomException(' The request is already in process.')

        auth_id = generate_random_alphanumeric(10)

        from pyfcm import FCMNotification

        push_service = FCMNotification(

        ## DB CALLS
        user = company_db.users.find_one({"_id": username})

        if not user:
            raise UserNotFound

        ## DB CALLS
        registration_id = company_db.users.find_one({"_id": username})['device_token']

        data_message = {
            "Nick" : "Mario",
            "body" : "great match!",
            "app_name": appname,
            "cid": cid,
            "auth_id" : auth_id,
            "api_key": api_key,
        message_body = "authenticate app?"
        username_appname = username + '_' + appname
        username_appname_response = username_appname + 'response'

        ## DB CALLS
        redis_db.setex(name=username_appname_response, value='waiting', time=180)
        redis_data = {'auth_id': auth_id, 'ts': datetime.datetime.now().strftime('%s')}
        redis_db.hmset(username_appname, redis_data)
        redis_db.expire(username_appname, 180)

        result = push_service.notify_single_device(registration_id=registration_id,  data_message=data_message)

I think this has many code smells. Ideally, I would like to refactor it so that I get

  • Easy unit testing
  • Avoiding mocks for unit testing where necessary
  • Minimal side effects
  • Readable code.

Current issues that I think are there:

  • Lots of database calls scattered around the core business logic.
  • Unit testing is difficult as lots of db calls requires mocking
    them. This tightly couples the test to my implementation rather than behaviour of the function.
  • Lots of indirection to logic is introduced because of scattered calls to mongo db & redis.

Possible solutions:

  • pass db objects to function rather than creating them inside the function that handles business logic(but this will just defer the object creation to a higher level so db calls will have to be mocked anyways)

How would you refactor this function considering best practices?

Is it a general practice while developing CRUD app to have multiple mocks on average for every function?

  • \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. \$\endgroup\$
    – Mast
    Aug 23, 2019 at 10:32
  • \$\begingroup\$ @Mast I have changed the title to be more specific \$\endgroup\$
    – anekix
    Aug 23, 2019 at 10:35
  • \$\begingroup\$ Remove the concerns from the title, put the concerns in the body of the question. The title should indicate what the code is doing, perhaps why. Not what your concerns (request to refactor, requiring to be unit-testable) are. \$\endgroup\$
    – Mast
    Aug 23, 2019 at 10:42
  • 1
    \$\begingroup\$ But thanks for pointing those out, might have to fix their titles later. \$\endgroup\$
    – Mast
    Aug 23, 2019 at 11:39
  • 2
    \$\begingroup\$ I shortened that discussion a bit by editing your title to the actual thing your code does (you were almost there ;)) I also edited some sentence structure/grammar. Feel free to edit again if I misconstrued anything. \$\endgroup\$
    – Graipher
    Aug 23, 2019 at 12:06


Your Answer

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

Browse other questions tagged or ask your own question.