Refactoring of a client API to avoid duplicated code and unclear passage of parameters

Question

I need to develop an API. The functions of the API are requests that call the services exposed by a server.

Initially the API worked like this:

import sys
reload(sys)
sys.setdefaultencoding('utf-8')

import logging
import suds
import json
import string
import socket
import signal
import traceback
from suds.client import Client
from suds.plugin import MessagePlugin
from suds.sudsobject import asdict
from suds.bindings.binding import Binding
from collections import namedtuple

from tornado import gen
from tornado.escape import json_decode, json_encode
from tornado.log import enable_pretty_logging
from tornado.netutil import bind_sockets
from tornado.options import define, options, parse_command_line

import time
import tornado.ioloop
from tornado.concurrent import run_on_executor
from concurrent.futures import ThreadPoolExecutor
import tornado.locale
import tornado.netutil
import tornado.tcpserver
import tornado.web
import tornado.websocket
from tornado.web import authenticated

class Server:
    def __init__(self):
        self.g_env_el = ""
        self.request = ""
        self.response = ""

        self.id = ""
        self.seqno = ""
        self.sessionid = ""

        self.server_status = 0
        self.server_amount = 0
        self.server_event = ""
        self.server_error = ""
        self.server_user = ""
        self.server_seqNo = 0

        self.client = self.create_client_Server()

    def create_client_Server(self):
        logging.info("::START - create_client_Server");
        url = "http://"+options.server_ip+"/axis2/services/"
        wsdlurl = url + 'BrueBoxService?wsdl'
        client = Client(url=wsdlurl, location=url+'BrueBoxService', plugins=[SoapFixer(self)])
        client.set_options(port='BrueBoxPort')
        logging.info("::END - create_client_Server");
        return client

    @gen.coroutine
    def startPayment(self, amount):
        """Ask the server to start the payment process and sets the amount to pay
        returns an integer
        0 unable to start the payment
        1 payment process started
        """
        logging.info("startPayment")
        try:
            client = self.client
            asynch = asyncExec()
            self.g_env_el = 'ChangeRequest'
            response =  yield asynch.callStartPayment(client, amount, self)
        except Exception as e:
            logging.error("exception %s" %e)
            response = 0
        raise gen.Return(response)

class asyncExec(object):
    def __init__(self,ioloop = None):
        self.executor = ThreadPoolExecutor(max_workers=10)
        self.io_loop = ioloop or tornado.ioloop.IOLoop.instance()

    @gen.coroutine
    def callStartPayment(self,client, amount, server):
        logging.info("START - serverapi::callStartPayment")
        val = self.doStartPayment(client, amount, server)
        logging.info("END - serverapi::callStartPayment")
        raise gen.Return(val)

    @run_on_executor
    def doStartPayment(self,client, amount, server):
        logging.info("START - serverapi::doStartPayment")
        try:
            response = (client.service.ChangeOperation(
                    server.id,
                    server.seqno,
                    server.sessionid,
                    amount,
                    None
                )
            )
        except Exception as e:
            logging.error("exception %s" %e)
            response = 0

        response = json2obj(suds_to_json(response))
        logging.info("END - gloryapi::doStartPayment")
        return(response)



if __name__ == "__main__":
    server = Server()
    server.initTCPServer()
    server.startPayment(amount=1500)

There was a lot of duplicated code and I didn't like the way it passed the arguments for the request. Because there are a lot of arguments I wanted to extract them from the request and make something more parametric.

So I refactored this way:

import sys
reload(sys)
sys.setdefaultencoding('utf-8')

import logging
import suds
import json
import string
import re
from suds.client import Client
from suds.bindings.binding import Binding

from tornado import gen
from tornado.log import enable_pretty_logging
from tornado.options import define, options, parse_command_line

# using a strategy pattern I was able to remove the duplication of code A and code B
# now send() receive and invoke the request I wanna send
class Server:
    def __init__(self):
        self.g_env_el = ""
        self.id = ""
        self.seqno = ""
        self.sessionid = ""

        self.server_status = 0
        self.server_amount = 0
        self.requestInfo = None

        self.client = self._create_client_Server()

    def _create_client_Server(self):
        url = "http://"+options.server_ip+"/axis2/services/"
        wsdlurl = url + 'BrueBoxService?wsdl'
        client = Client(url=wsdlurl, location=url+'BrueBoxService', plugins=[SoapFixer(self)])
        client.set_options(port='BrueBoxPort')
        return client

    ''' Send an async soap request '''
    def send(self, sendRequest):
        logging.info("START - sendRequest")
        try:
            asynch = asyncExec()
            response = sendRequest(self, asynch)
            asynch.executor.shutdown(wait=True)
        except Exception as e:
            self._exceptionOccurred(e)

        self._handleResponse()
        logging.info("END - sendRequest")
        return response

# Request contains all the requests and a list of the arguments used (requestInfo)
class Request:
    def __init__(self):
        self.requestInfo = {}
        pass

    # number and name of the arguments are not the same for all the requests
    # this function take care of this and store the arguments in RequestInfo for later use
    def setRequestInfo(self, **kwargs):
        if kwargs is not None:
            for key, value in kwargs.iteritems():
                self.requestInfo[key] = value

    @gen.coroutine
    @log_with(logClient)
    def startPayment(self, server, asynch):
        server.setG_env_el('ChangeRequest')
        response = yield asynch.doStartPayment(server, self.requestInfo)
        raise gen.Return(response)

# Async run the real request and wait for the answer
class Async:
    def __init__(self, ioloop = None):
        self.executor = ThreadPoolExecutor(max_workers=10)
        self.io_loop = ioloop or tornado.ioloop.IOLoop.instance()

    @run_on_executor
    @log_with(logClient)
    def doStartPayment(self, server, requestInfo):
        try:
            response = (server.client.service.ChangeOperation(
                    server.id,
                    server.seqno,
                    server.sessionid,
                    requestInfo["amount"],
                    requestInfo["cash"]
                )
            )
        except Exception as e:
            self._exceptionOccurred(e)
        return(response)


server = Server()
request = Request()

request.setRequestInfo(option=3)
server.send(request.startPayment)

The strategy pattern worked, the duplication is removed. Regardless, I'm afraid to have complicated things. Especially as regard to the arguments, I don't like the way I handle them because when I look at the code, it does not appear easy and clear.

So I wanted to know if there is a pattern or a better and clearer way to deal with this kind of client side API code.

I can't simply call client.firstRequest() because the requests are asynchronous and I need to use yield and raise gen.Return(response) for return the response and a couple of decorator to make it all work.

Answer 1

For a more generic approach, I wouldn't have introduced the Request class. It just add up on complexity and does not solve the issue of having a callXXX/doXXX style of repetition.

If the sole purpose of the callXXX method is to add logging before and after doXXX execution… then you should just log traces in doXXX which is already the case.

Instead, I would build on your idea of using the bound method to call as a parameter of Server.send, only keep doXXX methods in Asynch and rename them XXX, and use variable number of arguments in Server.send to handle genericity.

Something along the lines of:

class Server:
    @gen.coroutine
    def send(self, send_request, **kwargs):
        """Send an async soap request"""
        logging.info("START - sendRequest")
        try:
            self.g_env_el = 'ChangeRequest'
            kwargs['server'] = self
            kwargs['client'] = self.client
            response =  yield send_request(**kwargs)
        except Exception as e:
            logging.error("exception %s" %e)
            response = 0
        logging.info("END - sendRequest")
        raise gen.Return(response)

This will require that every method of Asynch defines a parameter named client and an other one named server.

You then use it with:

server = Server()
asynch = Asynch()
server.send(asynch.startPayment, amount=3)
server.send(asynch.stub_method, stub_param_1='a', stub_param_2=10)

And so on.

Now, for the rest of your code:

• Docstrings appears after the method declaration, not before.
• Remove the spaces around the = sign for parameters with default values.
• Use snake_case for both variables and functions names.
• Do not remove if __name__ == "__main__", it is good practice.
• Use format string syntax whenever possible:
  • url = "http://{}/axis2/services/BrueBoxService".format(options.server_ip)
  • Not sure if logging supports it, but at least it supports logging.error("exception %s", e) (I guess that your new _exceptionOccurred, which you’re not showing, still uses it).
• Remove pass statements in non-empty methods.