Refactoring Strategies For Ugly Code The Does Everything

Refactoring: Ugly Code That Does Everything

Gabe Schmidt Articles, Development Technologies & Tools, JavaScript, Node.js, Programming, Tutorial Leave a Comment

Attention: The following article was published over 4 years ago, and the information provided may be aged or outdated. Please keep that in mind as you read the post.

If you’ve been writing code for a significant amount of time, you’re sure to have seen the anything but godly “God Method.”

A “God Method” is a method that performs way too many processes in the system and has grown beyond all realistic logic to become “The Method That Does Everything.” It can be a single unsightly method that spans dozens, if not hundreds, of lines. Sometimes even over 1,000! This type of “ugly code” is an unbearable beast to maintain. This is why it’s considered a “code smell” or anti-pattern.

In this tutorial, we’ll walk through a tangible “God Method” and step through the process to refactor it into something more manageable and human-readable. Our code is written in JavaScript for a Node.js service, but the principles apply to any language.

Refactoring SOLIDly

SOLID is a mnemonic acronym for five object-oriented design principles intended to make software designs more understandable, flexible, and maintainable. S.O.L.I.D stands for:

This post will focus primarily on the “S” in SOLID principles.

I encourage readers to learn about each portion of the acronym to write and refactor to code that is easy to maintain and extend without stepping into the “God Method” territory.

Code Refactoring: Quality Ground Rules

Any fool can write machine-readable code. The machine doesn’t care what your code looks like as long as it compiles. Making code human-readable is more of an art. I believe the code should tell a story—a good story—sans meaningless details.

Just give me the gist! Okay, these are the general quality rules I go by:

  • Each method should be no more than 10 lines
  • No more than 4 parameters per method (2 or 3 is usually preferred)
  • Use meaningful variable names
  • Method names should be action words or an action/object combination
    • getTransactions
    • sendMail
    • save

Now that the ground rules are set for our JavaScript refactoring example, let’s dive in!

Our Ugly “God” Method

The following code is for syncing a transaction with a third-party billing provider. It happens to be written in JavaScript for a Node.js service, but the rules apply no matter your language of choice.

As you read the following ugly code, keep in mind that our primary focus will be on the improvement of the sendTransactionToIntegrationProvider method.

const keyGenerator = require('/opt/api-key-generator');
const request = require('request-promise-native');
const uuidv4 = require("uuid/v4");
const moment = require('moment');
const { SERVICE_URL, INTEGRATION_URL, INTEGRATION_API_KEY, INTEGRATION_API_VERSION, INTEGRATION_ENABLED } = process.env;
 
exports.handler = async event => {
  const transaction = event;
  console.log('Client Id:', transaction.clientId);
  console.log('Transaction Id:' , transaction.id);
  console.log('TransactionCurrency:' , transaction.transactionCurrency);
  const generator = new keyGenerator.ApiKeyGenerator();
  const apiKey = generator.generateApiKey(transaction.clientId);
 
  const result = await sendTransactionToIntegrationProvider(apiKey, transaction);
 
  await updateTransaction(apiKey, transaction.id, result);
};
 
// Send the transaction to Integration Provider
async function sendTransactionToIntegrationProvider(apiKey, transaction) {
  console.log(`Sending transaction to Integration Provider`);
  let productDescription = '';
 
  const b = await fetchEntity(apiKey, `${SERVICE_URL}/buyer/${transaction.buyerId}`);
  const s = await fetchEntity(apiKey, `${SERVICE_URL}/seller/${transaction.sellerId}`);
 
  let result = undefined;
  if (INTEGRATION_ENABLED === 'true') {
    if (!propertyDefined(s, 'integrationId')) {
      throw new Error('Missing integrationId, Seller not set up in Integration');
    }
 
    if (propertyDefined(transaction, 'productId')) {
      const productUrl = `${SERVICE_URL}/product/transaction.productId}`;
      const product = await fetchEntity(apiKey, productUrl);
 
      if (propertyDefined(product, 'description')) {
        productDescription = product.description;
      }
    }
 
    const body = {
      transaction_id: transaction.id,
      buyer_id: b.id,
      seller_id: s.id,
      date: moment(transaction.transactionTimestamp).format('YYYY-MM-DD'),
      total: formatCurrencyAmount(transaction.amount),
      tax_amount: formatCurrencyAmount(transaction.taxAmount),
      currency: transaction.transactionCurrency,
      details: [
        {
          sku: transaction.sku,
          description: productDescription,
          quantity: Number(transaction.quantity),
          unit_price: formatCurrencyAmount(transaction.unitPrice)
        }
      ]
    };
    const correlationId = uuidv4();
 
    console.log(body);
    console.log(`INTEGRATION x-correlation-id: ${correlationId}`);
 
    result = await request({
      method: 'POST',
      uri: `${INTEGRATION_URL}/${INTEGRATION_API_VERSION}/invoices`,
      headers: {
        'x-correlation-id': correlationId,
        Authorization: `Basic ${Buffer.from(INTEGRATION_API_KEY)}`
      },
      auth: {
        username: process.env.INTEGRATION_API_KEY,
        password: 'my$uper$3cretP@ssw0rd'
      },
      json: true,
      body
    });
 
    console.log(result);
 
    if (!propertyDefined(result, 'id') || !propertyDefined(result, 'bill_date') || !propertyDefined(result, 'due_date')) {
      throw new Error('Missing required values from INTEGRATION response');
    }
  } else {
    console.log('INTEGRATION turned off, faking result');
 
    result = {
      id: undefined,
      bill_date: moment().add(1, 'days').format('YYYY-MM-DD'),
      due_date: moment().add(b.paymentTerms + 1, 'days').format('YYYY-MM-DD')
    };
 
    console.log(result);
  }
 
  return result;
}
 
async function fetchEntity(apiKey, url) {
  const entity = await request({
    uri: url,
    headers: {
      Authorization: `x-api-key ${apiKey}`
    },
    json: true
  });
 
  return entity;
}
 
// Check to see if a property exists on an object and that it has a value
function propertyDefined(objectNm, propertyNm) {
  return (objectNm.hasOwnProperty(propertyNm) && objectNm[propertyNm] !== '' && objectNm[propertyNm] !== null);
}
 
// Integration provider expects currency amounts to be returned as integers with implied 2 decimal places
function formatCurrencyAmount(amountString) {
  return parseFloat(amountString) * 100;
}
 
// Update the transaction with the results from IntegrationProvider
async function updateTransaction(apiKey, transactionId, resultBody) {
  await request({
    method: 'PATCH',
    uri: `${SERVICE_URL}/transaction/${transactionId}`,
    headers: {
      Authorization: `x-api-key ${apiKey}`
    },
    json: true,
    body: {
      integrationId: resultBody.id,
      statementDate: resultBody.bill_date,
      paymentDate: resultBody.due_date
    }
  });
}

Code Refactoring: The “Method Too Large” Play By Play

The aforementioned sendTransactionToIntegrationProvider method is 77 lines long! That’s too much. Let’s break it down.

Related Posts:  From AI Resumes to Fake Candidates: Protecting Your Company from Hiring Scams

There’s a ton going on in that first if statement. I’ll start by breaking that out.

Most IDEs have an extract method menu item built into IntelliSense (like Visual Studio, VSCode, etc.). “IntelliSense” is a general term for code features including code completion, parameter info, quick info, and member lists. It’s super handy.

Extraction is generally accomplished by highlighting the desired code, right-clicking, and selecting “extract method” from the menu.

I’ll do that for the outermost if/else statement.

// Send the transaction to Integration Provider
async function sendTransactionToIntegrationProvider(apiKey, transaction) {
  console.log(`Sending transaction to Integration Provider`);
  let productDescription = '';
 
  const b = await fetchEntity(apiKey, `${SERVICE_URL}/buyer/${transaction.buyerId}`);
  const s = await fetchEntity(apiKey, `${SERVICE_URL}/seller/${transaction.sellerId}`);
 
  let result = undefined;
  if (INTEGRATION_ENABLED === 'true') {
    result = await postTransactionToIntegrationProvider(transaction, apiKey, seller);
  } else {
    result = getFakeResult(transaction, apiKey, buyer);
  }
  console.log(result);
  return result;
}

That looks better, but there’s still a little bit more we can do.

What’s with those one-letter variable names? Those are hard to follow. Let’s make them more meaningful.

The first console.log statement and the productDescription variable are specific to postTransactionToIntegrationProvider.

Let’s move them while we’re at it. Now we have this slightly improved code:

// Send the transaction to Integration Provider
async function sendTransactionToIntegrationProvider(apiKey, transaction) {
  const buyer = await fetchEntity(apiKey, `${SERVICE_URL}/buyer/${transaction.buyerId}`);
  const seller = await fetchEntity(apiKey, `${SERVICE_URL}/seller/${transaction.sellerId}`);
 
  let result = undefined;
  if (INTEGRATION_ENABLED === 'true') {
    result = await postTransactionToIntegrationProvider(transaction, apiKey, seller);
  } else {
    result = getFakeResult(transaction, apiKey, buyer);
  }
  console.log(result);
  return result;
}

Yes, that’s better, but there’s more we can do.

The buyer is required by both the postTransactionToIntegrationProvider and getFakeResult methods, but we only need seller in postTransactionToIntegrationProvider, so let’s move that declaration.

Now we have this:

// Send the transaction to Integration Provider
async function sendTransactionToIntegrationProvider(apiKey, transaction) {
  const buyer = await fetchEntity(apiKey, `${SERVICE_URL}/buyer/${transaction.buyerId}`);
  let result = undefined;
  if (INTEGRATION_ENABLED === 'true') {
    postTransactionToIntegrationProvider(transaction, apiKey, buyer);
  } else {
    getFakeResult(transaction, apiKey, buyer);
  }
  console.log(result);
  return result;
}

That looks good, but we’re not quite done yet.

The comment above the method does nothing for us since the method signature tells us everything we need to know. Replace the if/else statement with a ternary operator and we have our final, cleaned-up sendTransactionToIntegrationProvider method.

const sendTransactionToIntegrationProvider = async (transaction, apiKey) => {
  const result = (INTEGRATION_ENABLED === 'true')
    ? await postTransactionToIntegrationProvider(transaction, apiKey)
    : await getFakeResult(transaction, apiKey);
  console.log(result);
  return result;
}

And the resulting getFakeResult method.

const getFakeResult = async (transaction, apiKey) => {
    console.log('Integration Provider turned off, faking result');
    const buyer = await fetchEntity(apiKey, `${SERVICE_URL}/buyer/${transaction.buyerId}`);
    return {
        id: undefined,
        bill_date: moment().add(1, 'days').format('YYYY-MM-DD'),
        due_date: moment().add(buyer.paymentTerms + 1, 'days').format('YYYY-MM-DD')
    };
}

Looks good. Nothing more to do there.

As for postTransactionToIntegrationProvider—well, there’s still some work to be done.

I’ll take the same approach to this method that I did above.

const postTransactionToIntegrationProvider = async (transaction, apiKey) => {
    console.log(`Sending transaction to Integration Provider`);
    const seller = await fetchEntity(apiKey, `${SERVICE_URL}/seller/${transaction.sellerId}`);
    if (!propertyDefined(seller, 'integrationId')) {
        throw new Error('Missing integrationId, Seller not set up in Integration');
      }
 
      let productDescription = '';      
      if (propertyDefined(transaction, 'productId')) {
        const productUrl = `${SERVICE_URL}/product/transaction.productId}`;
        const product = await fetchEntity(apiKey, productUrl);
  
        if (propertyDefined(product, 'description')) {
          productDescription = product.description;
        }
      }
  
      const body = {
        transaction_id: transaction.id,
        buyer_id: buyer.id,
        seller_id: seller.id,
        date: moment(transaction.transactionTimestamp).format('YYYY-MM-DD'),
        total: formatCurrencyAmount(transaction.amount),
        tax_amount: formatCurrencyAmount(transaction.taxAmount),
        currency: transaction.transactionCurrency,
        details: [
          {
            sku: transaction.sku,
            description: productDescription,
            quantity: Number(transaction.quantity),
            unit_price: formatCurrencyAmount(transaction.unitPrice)
          }
        ]
      };
      const correlationId = uuidv4();
  
      console.log(body);
      console.log(`INTEGRATION x-correlation-id: ${correlationId}`);
  
      result = await request({
        method: 'POST',
        uri: `${INTEGRATION_URL}/${INTEGRATION_API_VERSION}/invoices`,
        headers: {
          'x-correlation-id': correlationId,
          Authorization: `Basic ${Buffer.from(INTEGRATION_API_KEY)}`
        },
        auth: {
          username: process.env.INTEGRATION_API_KEY,
          password: 'my$uper$3cretP@ssw0rd'
        },
        json: true,
        body
      });
  
      console.log(result);
  
      if (!propertyDefined(result, 'id') || !propertyDefined(result, 'bill_date') || !propertyDefined(result, 'due_date')) {
        throw new Error('Missing required values from INTEGRATION response');
      }
}

By breaking the code down and extracting out getIntegrationProviderTransactionBody, getIntegrationProviderHeaders, and postToIntegrationProvider, the postTransactionToIntegrationProvider method has been reduced from 59 lines down to 10 and became much more human-readable in the process.

const postTransactionToIntegrationProvider = async (transaction, apiKey) => {
  console.log(`Sending transaction to Integration Provider`);
  const seller = await fetchEntity(apiKey, `${SERVICE_URL}/seller/${transaction.sellerId}`);
  if (!propertyDefined(seller, 'integrationId')) {
    throw new Error('Missing integrationId, Seller not set up in Integration Provider');
  }
  const body = await getIntegrationProviderTransactionBody(transaction, seller, apiKey)
  console.log(body);
  const headers = await getIntegrationProviderHeaders(transaction)
  console.log(headers)
  return await postToIntegrationProvider(headers, body);
}

The Final Refactored Result

Now we’ve successfully refactored our code!

Related Posts:  HTMX in Action: How to Build a Lightweight Signup Form

The finished project goes way farther in the goal to write extendable and maintainable code using SOLID principles. We made a huge impact, specifically in better implementing the “S,” the single-responsibility principle.

Here’s the final code:

const keyGenerator = require('/opt/api-key-generator');
const request = require('request-promise-native');
const uuidv4 = require("uuid/v4");
const moment = require('moment');
const { SERVICE_URL, INTEGRATION_URL, INTEGRATION_API_KEY, INTEGRATION_API_VERSION, INTEGRATION_ENABLED } = process.env;
 
exports.handler = async event => {
  const transaction = event;
  console.log('Client Id:', transaction.clientId);
  console.log('Transaction Id:' , transaction.id);
  console.log('TransactionCurrency:' , transaction.transactionCurrency);
  const generator = new keyGenerator.ApiKeyGenerator();
  const apiKey = generator.generateApiKey(transaction.clientId);
 
  const result = await sendTransactionToIntegrationProvider(transaction, apiKey);
  await updateTransaction(transaction.id, apiKey, result);
};
 
const sendTransactionToIntegrationProvider = async (transaction, apiKey) => {
  const buyer = await fetchEntity(apiKey, `${SERVICE_URL}/buyer/${transaction.buyerId}`);
  const result = (INTEGRATION_ENABLED === 'true')
    ? await postTransactionToIntegrationProvider(transaction, apiKey, buyer)
    : await getFakeResult(buyer);
  console.log(result);
  return result;
}
 
const postTransactionToIntegrationProvider = async (transaction, apiKey, buyer) => {
  console.log(`Sending transaction to Integration Provider`);
  const seller = await fetchEntity(apiKey, `${SERVICE_URL}/seller/${transaction.sellerId}`);
  if (!propertyDefined(seller, 'integrationId')) {
    throw new Error('Missing integrationId, Seller not set up in Integration Provider');
  }
  const body = await getIntegrationProviderTransactionBody(transaction, seller, buyer, apiKey)
  console.log(body);
  const headers = await getIntegrationProviderHeaders(transaction)
  console.log(headers)
  return await postToIntegrationProvider(headers, body);
}
 
const getFakeResult = async (buyer) => {
  console.log('Integration Provider turned off, faking result');  
  return {
    id: undefined,
    bill_date: moment().add(1, 'days').format('YYYY-MM-DD'),
    due_date: moment().add(buyer.paymentTerms + 1, 'days').format('YYYY-MM-DD')
  };
}
 
const getIntegrationProviderTransactionBody = async (transaction, seller, buyer, apiKey) => {
  return {
    transaction_id: transaction.id,
    buyer_id: buyer.id,
    seller_id: seller.id,
    date: moment(transaction.transactionTimestamp).format('YYYY-MM-DD'),
    total: formatCurrencyAmount(transaction.amount),
    tax_amount: formatCurrencyAmount(transaction.taxAmount),
    currency: transaction.transactionCurrency,
    details: [
      {
        sku: transaction.sku,
        description: await getProductDescription(transaction, apiKey),
        quantity: Number(transaction.quantity),
        unit_price: formatCurrencyAmount(transaction.unitPrice)
      }
    ]
  };
}
 
const getProductDescription = async (transaction, apiKey) => {
  let productDescription = ''
  if (propertyDefined(transaction, 'clientProductId')) {
    const productUrl = `${SERVICE_URL}/product/mapping/client/${transaction.clientId}/product/${transaction.clientProductId}`;
    const clientProduct = await fetchEntity(apiKey, productUrl);
    if (propertyDefined(clientProduct, 'description')) {
      productDescription = clientProduct.description;
    }
  }
  return productDescription
}
 
const getIntegrationProviderHeaders = async (transaction) => {
  const correlationId = uuidv4();
  console.log(`Integration Provider x-correlation-id: ${correlationId}`);  
  let headers = {
    'x-correlation-id': correlationId,
    Authorization: `Basic ${Buffer.from(INTEGRATION_API_KEY)}`
  };
  if (transaction.testDate) {
    headers = {
      ...headers,
      'x-test-invoice-bill-date-calc': 'invoice_date'
    };
  }
  return headers;
}
 
const postToIntegrationProvider = async (headers, body) => {
  const result = await request({
    method: 'POST',
    uri: `${INTEGRATION_URL}/${INTEGRATION_API_VERSION}/invoices`,
    headers,
    auth: {
      username: process.env.INTEGRATION_API_KEY,
      password: ''
    },
    json: true,
    body
  });
  if (!propertyDefined(result, 'id') || !propertyDefined(result, 'bill_date') || !propertyDefined(result, 'due_date')) {
    throw new Error('Missing required values from Integration Provider response');
  }
  return result
}
 
const fetchEntity = async(apiKey, url) => {
  const entity = await request({
    uri: url,
    headers: {
      Authorization: `x-api-key ${apiKey}`
    },
    json: true
  });
 
  return entity;
}
 
// Check to see if a property exists on an object and that it has a value
const propertyDefined = (objectNm, propertyNm) => {
  return (objectNm.hasOwnProperty(propertyNm) && objectNm[propertyNm] !== '' && objectNm[propertyNm] !== null);
}
 
// Integration Provider expects currency amounts to be returned as integers with implied 2 decimal places
const formatCurrencyAmount = (amountString) => {
  return parseFloat(amountString) * 100;
}
 
// Update the transaction with the results from Integration Provider
const updateTransaction = async (transactionId, apiKey, resultBody) => {
  await request({
    method: 'PATCH',
    uri: `${SERVICE_URL}/transaction/${transactionId}`,
    headers: {
      Authorization: `x-api-key ${apiKey}`
    },
    json: true,
    body: {
      integrationId: resultBody.id,
      statementDate: resultBody.bill_date,
      paymentDate: resultBody.due_date
    }
  });
}

While I’m generally not a fan of code comments – again, the code itself should tell the story – those in the last three methods provide some useful detail that can’t simply be gleaned from the method signature. That said, I would advise most developers to use code comments sparingly.

Conclusion

We’ve taken an unsightly god method and, using some ground rules, broken it down into several smaller chunks resulting in cleaner, more manageable code.

While refactoring a method is just one of many clean coding techniques, I find myself using this one most often—almost daily.

I encourage developers to break down the refactoring process into manageable chunks and perform timely testing before moving on. Refactoring is an ongoing maintenance project and while it may feel tedious while you’re doing it, anybody tasked with maintaining your code, including your future self, will be thankful that it’s easy to read.

Visit one of the following links for more on clean coding:

Related Posts

0 0 votes
Article Rating
Subscribe
Notify of
guest
0 Comments
Oldest
Newest Most Voted
Inline Feedbacks
View all comments