From Prototype to Production: Refactoring Your Internal APIs

Intro

This post was originally written for my blog refactormycode.dev

Many internal projects start with quickly built APIs that can become difficult to manage over time. This article provides practical advice on refactoring these “prototype” APIs, transforming them into stable and efficient solutions suitable for production environments.

Disclaimer: All the provided code examples have been redacted for the sake of brevity

How the mess started

A few years ago, I worked for a small Italian company in the professional development training sector. The code in this article is from an internal web application needed to file and organize any correspondence sent and received that could be assigned to an existing project. This included quotes, contracts, invoices, payment receipts, etc.

When the requirement came in it was just what you read above, pretty simple right?

All internal resources were already saturated with other client’s projects, which is pretty common in small companies, and this project had a lower priority so a decision was made to commission it to an offshore team, at least for the initial bootstrap phase, and then my team would pick it up from there.

My responsibility was to put together a specification document for an MVP, find external contractors, and do a bit of project management. I was not involved in coding or reviews.
Even if I didn’t have much time allocated to this project I’ve tried to be diligent about gathering the requirements, talking to the admin team to understand their real needs, and also finding the right offshore team to work with.

A naive approach

Together with the external team, we decided to use Firebase which seemed to fit our use case perfectly:

• Serverless platform so no devops overhead
• Easy to deploy
• Seamless integration of Angular front end with Google Cloud Function for our backend API

After the initial meeting with the team lead all the requirements seemed clear and they were ready to start.

I was not involved in any code review since they “allegedly” had a full team working on it including a senior team lead, so I would touch base with him every couple of days or when some clarification was needed, they seemed professional so I trusted them.

The big ball of mud

After 4-5 weeks, when the first beta version was ready for an internal demo I took a look at the code and it was already very… “unstructured”, here’s an example:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72

export async function create(req: Request, res: Response) {
	try {
		let document:any = null
		const valid = await documentSchema.noUnknown(true).isValid(req.body);
		if (!valid) return res.status(400).send(await documentSchema.validate(req.body));
		const validDate = await documentSchema.noUnknown(true).validate(req.body);
		document = validDate

        if([3,6].includes(document.docType) && document.sourceType === 2){
            return res.status(400).send({  message: 'Invalid - Document Type' })
        }

        if([1,2,3,4,6].includes(document.docType) && !document.expDate){
            return res.status(400).send({  message: 'undefined - expDate is Required' })
        }

        if(document.docType !== 4){
            document.docRef = undefined;
        }

        if(document.docRef) {
            const doc = (await db.collection("documents")
                .doc(document.docRef).get()).data();
            if(!doc) {
                return res.status(403).send({
                    message:"undefiend - docRef points to an invalid document"
                })
            }
        }

        if(document.projectRef){
            const project = (await db.collection("projects")
                .doc(`${document.projectRef}`).get()).data();
            if(!project) {
                return res.status(403).send({
                    message:"undefiend - projectRef points to an invalid project"
                })
            }
        }

        //Conditions based on sourceType
        if(document.sourceType === 1 && !document.sender){
            return res.status(400).send({ message: 'undefined - sender is required' })
        }

        //Conditions based on sourceType
        if(document.sourceType === 2 && !document.recipients){
            return res.status(400).send({ message: 'undefined - recipients is required' })
        }

        if(document.sourceType === 1 && document.recipients){
            document.recipients = undefined;
        }

        if(document.sourceType === 2 && document.sender){
            document.sender = undefined;
        }

        if(![1,2,3,4,6].includes(document.docType) && document.expDate){
            document.expDate = undefined;
        }

        [...omitted code here]

        const docRef = await db.collection('documents').doc(code).set(document)


		return res.status(201).send({ id: docRef.id })
	} catch (err) {
		return handleError(res, err)
	}
}

This was supposed to be a controller for the nodejs API but it ended up being “everything”, it’s handling data validation, business logic, database access, and much more.
The rest of the API followed the same pattern.

This code is not easy to test (if that’s even possible) which will also make it very difficult to maintain and debug.

Let’s break down the issues

First of all, if you read the code above can you understand any business logic? Not much, right?

I believe that every time a new developer joins the team they should be able to figure out at least the main purpose or any given portion of code.
Let’s get into what I think is the right approach for a gradual refactoring that also brings business value so your manager/boss won’t yell at you.

1. Make the code more readable
2. Gradually separate the code by defining “responsibilities”
3. Write tests

1. Make the code more readable

The first issue I see is not using any constants for document.docType or document.sourceType

1
2
3

if([3,6].includes(document.docType) && document.sourceType === 2){
    return res.status(400).send({  message: 'Invalid - Document Type' })
}

What type of docType are 3 and 6 and why they can’t have a sourceType of 2? Let’s try to fix this by mapping docType and sourceType and use predefined strings for validation errors

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18

export enum DocumentType {
    INVOICE = 1,
    RECEIPT = 2,
    SURETY = 3,
    PAYMENT = 4,
    COMMUNICATION = 5,
    PAYMENT_NOTICE = 6
}

export enum DocumentSource {
    INCOMING = 1,
    OUTGOING = 2
}

const ValidationErrors = {
    INVALID_DOCUMENT_TYPE: 'Invalid document type',
    [...add all validation errors here]
};

so the previous code becomes

1
2
3
4

if ([DocumentType.SURETY, DocumentType.PAYMENT_NOTICE].includes(document.docType) &&
    document.sourceType === DocumentSource.OUTGOING) {
    return res.status(400).send({ message: ValidationErrors.INVALID_DOCUMENT_TYPE });
}

now it should be more clear that an OUTGOING document cannot be a SURETY or PAYMENT_NOTICE.
Also adding some comments is always a good practice, especially in situations like this one where the business logic is scattered around the code.

2. Gradually separate the code by defining “responsibilities”

Should a controller contain business logic or database access code? Hmm… I don’t think so.

I’m a big fan of the clean architecture and I try to use it every time I start a new project of medium+ complexity.

In a refactoring like this one though, it’s not practical to just scrap the existing code to re-build the whole codebase using the “ideal” architecture.
We are trying to bring business value and improve the code at the same time while not getting yelled at by our manager, remember?

So for this initial refactoring phase, I believe the code should look like this:

• Controllers:
  • process incoming requests and basic data validation no business logic
  • call services to validate business logic
  • format response
• Services:
  • validate business logic
  • call repositories for database operations
• Repositories:
  • read/write to DB

Let’s add these changes to our code now.

The first part of the controller was just doing some basic data validation using yup and we are not going to change that for now, we’ve only changed some variable names to make them more meaningful and indented the code so it’s more readable.

One noticeable improvement is creating an interface for the incoming request, this way we (and anybody else working on this project) know exactly what data structure this endpoint is expecting.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34

export interface CreateDocumentRequest {
  organizationId: string;
  sourceType: DocumentSource;
  sender?: string;
  recipients?: Array<string>;
  docType: DocumentType;
  amount?: number;
  docRef: string;
  notes?: string;
  projectRef: string;
}

export async function create(req: Request, res: Response) {
	try {
        // data validation
		let parsedDocument: CreateDocumentRequest;
		const isValid = await documentSchema.noUnknown(true).isValid(req.body);
		if (!isValid)  {
            return res.status(400).send(await documentSchema.validate(req.body));
        }

		parsedDocument = await documentSchema.noUnknown(true).validate(req.body);

        // validate business logic and save to db
        const documentId = await documentService.create(parsedDocument)
        
        // format response
		return res.status(201).send({ id: documentId })

    } catch (err) {
        // all validation errors will be caught here
		return handleError(res, err)
	}
}

The business logic has been moved to a separate class called DocumentService that will handle all the business logic and call the repository to store our documents in the database

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17

export class DocumentService {

  constructor(private documentRepository: DocumentRepository) {}

  async create(documentRequest: CreateDocumentRequest): Promise<string> {

       if ([DocumentType.SURETY, DocumentType.PAYMENT_NOTICE].includes(document.docType) &&
            document.sourceType === DocumentSource.OUTGOING) {
            throw ValidationErrors.INVALID_DOCUMENT_TYPE;
        }

       [...omitted code here]

       return this.documentRepository.save(document);

  }
}

The final piece is storing the document in the database, and it’s handled by the DocumentRepository.
I really like the repository pattern because it allows you to separate the data access from the rest of the code, this way you can easily mock the database layer in your unit tests.

We create a simple interface so our service is not tightly coupled to the Firestore implementation and we can easily switch database or mock it for our unit tests.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15

export interface DocumentRepository {
    save(document: Document): Promise<string>;
}

export class DocumentFirestoreRepository implements DocumentRepository {
    constructor(private readonly firestore: admin.firestore.Firestore) {}
    
    async save(document: Document): Promise<string> {

        const newDocumentRef = await this.firestore.collection('documents').add(document);

        return newDocumentRef.id;
    }
}
		

What’s the business value in just restructuring the code? One could ask…

The answer is simple, cleaner code is easier to test which will reduce the number of bugs and also the debugging time when a bug occurs.

So less development time is spent on fixing bugs and more time is spent to develop new features that will bring in more revenue for your company.

3. Write tests

This is actually the first thing any professional software developer should do, if you follow the TDD approach, as suggested by Uncle Bob in his book Clean Coder which I highly recommend.

But nobody is perfect and we all know that many teams don’t/can’t follow this approach for different reasons, so let’s just not be too romantic about this now and start writing the tests.

I think the first thing to test in this case would be the business logic so we need to write a test for our DocumentService class.
Since I’ve omitted most of the business rules we’ll create a test for the 2 possible validation failures we have in our service and also the happy path where a document is successfully created.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51

describe('Document', function () {
    describe('fails when trying to create document', function () {
        it('invalid OUTGOING document type PAYMENT_NOTICE', function () {
            const request: CreateDocumentRequest = {
                organizationId: '123456',
                sourceType: DocumentSource.INCOMING,
                sender: '1',
                docType: DocumentType.PAYMENT_NOTICE,
                amount: 100,
                docRef: '1',
                projectRef: '1'
            };

            assert.throws(() => documentService.create(request), ValidationErrors.INVALID_DOCUMENT_TYPE);
        });

        it('invalid OUTGOING document type SURETY', function () {
            const request: CreateDocumentRequest = {
                organizationId: '123456',
                sourceType: DocumentSource.INCOMING,
                sender: '1',
                docType: DocumentType.SURETY,
                amount: 100,
                docRef: '1',
                projectRef: '1'
            };

            assert.throws(() => documentService.create(request), ValidationErrors.INVALID_DOCUMENT_TYPE);
        });
    });

    describe('succeeds when trying to create document', function () {
        it('should create valid invoice', function () {
            const request: CreateDocumentRequest = {
                organizationId: '123456',
                sourceType: DocumentSource.INCOMING,
                sender: '1',
                docType: DocumentType.INVOICE,
                amount: 100,
                docRef: '1',
                projectRef: '1'
            };

            const id = await documentService.create(request);

            expect(typeof id).to.equal('string');
            expect(id.length).not.to.equal(0);

        });
    });
});

That’s it!

Well… kind of, there are many other improvements we can do like defining different entities for each document type with their properties and business rules and use factories to generate the right entity based on its type for example.

Conclusion

In this post, we’ve taken a not-very-well-designed web application and gradually improved it to make it more readable, maintainable, and testable. We have tried to use different design principles and apply them in a practical manner to make sure the refactoring time was a good return on investment for your company.

Important Links

• TDD
• Repository Pattern
• Clean Architecture
• Uncle Bob
• The Clean Coder