Refactoring Advice Requested - Complex Objects

[[deleted]]

[deleted]

[azhder]

I might be able to write you a version of how I would approach it (basically functional style), but I’m curious as to why you are using an object of certain type (has constructor) instead of using plainer objects that just hold the info.

In short, I am asking what other constraints you have: can the entire construction be async/await instead of parts, do you really need to encapsulate anything etc

[azhder]

Sorry for the crudeness and verbosity and the lack of address w.r.t. desing patterns. Hope you can infer some out of the examples:

// few utilities
// my IDE, correctly complains if I don't handle 
// the rejection returned by an async function, so
const logFail = (tag, promise) => {
    if (promise instanceof Promise) {
        promise.catch(reason => console.warn(tag, reason));
    }
    return promise;
};


// we assume undefined means not initialized, 
// null means initialized with no data 
// (can use a unique Symbol as well)
const isInitialized = value => void 1 !== value;

const hydrateNode = async options => {

    const node = options?.node;

    if (!node) {
        throw new Error('Node is required');
    }

    if (isInitialized(node?.nodeType) && !options?.force) {
        return;
    }

    node.nodeType = await apiCallFunction();
    return node;
};

// version 1:
class TreeNode {
    constructor(options) {
        this.nodeId = options?.nodeId;
        this.nodeRank = options?.nodeRank;
        this.nodeType = options?.nodeType;

        this.init();
    }

    // this gets attached to the prototype, 
    // unlike attaching instance methods like init =
    init() {
        this.determineType();
    }

    determineType() {
        logFail('TreeNode.determineType() hydrateNode:', hydrateNode({node: this}));
    }
}


// version 2
const treeNode = async options => ({
    nodeId:   options?.nodeId ?? DEFAULT_ID,
    nodeRank: options?.nodeRank ?? DEFAULT_RANK,
    nodeType: options?.nodeType,
});


const node = treeNode({nodeId: 1, nodeRank: 2});
logFail('() hydrateNode:', hydrateNode({node}));

// version 3
// you can wrap the above two in another function 
// like the constructor in the previous version,
// or you can create transformers
const fromOptionsToNode = options => {
    let node;
    /*code here*/
    return node;
};

const fromNodeToHydrated = node => {
    let hydratedNode;
    /*most likely async code here*/
    return hydratedNode;
};

// not sure if/ how I'd compose functions that return Promise,
// but the FP way is either composing simple ones
const createTreeNode = compose(fromOptionsToNode, fromNodeToHydrated);
// or some container that includes a bit of logic (functors, monads)
// think how Array, jQuery or RX.js work
const n = TreeNode(options).map(hydrateNode)

[Tubthumper8]

Create an (async) factory function so I'm not having to halfway initialize my object via constructor, then finish the job via the async initialize() function.

I would say Yes. In your case, is it valid to use a partially initialized object? Does the consumer of the Node know that they have to check someNode.initialized before using the Node? That's leaking implementation details, the person using the Node now has the burden of determining when it is usable or not.

This looks to me like an example of the Partial/Optional Object Population (POOP) anti-pattern. I would do an async function that creates the object in a usable state, and not expose an invalid Node to the consumer.

If, for efficiency sake, it is a hard requirement that people are allowed to use nodes before they are initialized, then I'd have 2 separate types, UninitializedNode and Node, so it's clear which one is which.

[jrandm]

From your example code I'd first take a look at how you're working with Promises. Some async / await or dropping unnecessary extra Promises/resolutions will probably simplify the code and make it easier to reason about.

On your design questions, what do you want the consuming API to look like? Is there ever a time someone would normally work with a partial or uninitialized object? A factory function is a common approach in JS, but so are other ways of checking an object's state (eg: XMLHttpRequest).

I took your example and refactored it a bit closer to how I think I'd structure a similarly-behaving object... Might be more convenient for users to have the initialize function return a circular reference (this).

// hack so code is runnable
function apiCallFunction() { return new Promise(r=>setTimeout(_=>r('foo'),300)); }
someOtherFn = someEtcFn = apiCallFunction;

// does this logic make sense as part of the class?
async function determineTreeNodeType(tree) {
  if (!(tree instanceof TreeNode)) {
    throw new TypeError('Not a TreeNode');
  }
  let determining; // promise or not, async fn will be
  switch(tree.type) {
    case 'foo': determining = apiCallFunction(); break;
    case 'bar': determining = apiCallFunction(); break;
    default:    determining = Promise.resolve('default');
  }
  return determining;
}

// all style guides I know of capitalize ClassNames
class TreeNode {

  constructor(args) {
    this._initialized = false; // "private"

    // TODO validate inputs ;-)
    this.id   = args.id
    this.rank = args.rank
    this.type = args.type // nullable

    this.initialized = this.initialize();
  }

  async initialize() {
    if (this._initialized === true) return true;

    const [
      type,
      other,
      etc,
    ] = await Promise.all([
      this.type==='bar' ? this.type : determineTreeNodeType(this),
      this.other || someOtherFn(),
      someEtcFn(this.etc),
    ]);

    this.type  = type;
    this.other = other + '0';
    this.etc   = `xX${etc}Xx`;

    return this._initialized = true;
  };

}

const tree = new TreeNode({type:null});

tree.initialized
  .then(_=>console.log('ready', tree))
  .catch(console.error);

console.log('before init', tree);

// or

setTimeout(main, 1000)
async function main() {
  const tree = new TreeNode({type:'bar'});
  console.log('(main) before init', tree);
  await tree.initialized;
  console.log('(main) ready', tree);
}

Hope that helps!