5
\$\begingroup\$

I have written a function buildGroupRequests() to build an array of multiple groups.

It checks the size of the request group. If a request group is greater than MAXSIZE = 50 then it will create another group in an array.

The logic seems to be complicated, is there a way to rewrite this to be more readable and maintainable in future?

console.log(
  buildGroupRequests([
    { put: "/firstName=Danny" },
    { put: "/lastName=Williams" },
    { put: "/email=danny.w@domain.com" }
  ])
);

function buildGroupRequests(lists) {
  const MAXSIZE = 50;

  return lists.reduce((group, insert) => {
    const LASTINDEX = group.length - 1;

    if (!group.length) {
      group.push({ put: "PUT " + insert.put });
      return group;
    }

    const putGroup = group[LASTINDEX];
    const tempPutSet = putGroup.put + " + " + insert.put;

    if (tempPutSet.length > MAXSIZE) {
      group.push({ put: "PUT " + insert.put });

      return group;
    }

    group[LASTINDEX].put = tempPutSet;
    return group;
  }, []);
}

\$\endgroup\$
2
\$\begingroup\$

Maintainability

Maintainability is a measure of how long it takes to modify some code.

Some things that help improve maintainability.

  • Use a consistent style throughout the code base. If the code is a group effort then that style must be established before the start of the project.

  • For naming adopt the language standardized or informal style (Informal for JS as it does not have a standard style). In JavaScript we use camelCase variable names, PascalCase for instantiate-able objects via the new operator.

    For constants UPPERCASE_SNAKE with the general rule that we uppercase read-only properties of objects. Though many will also use them for all constants. There is no real consensus regarding the naming of constants but it is not good to be inconsistent in the use of UPPERCASE_SNAKE.

    Arrays, lists, and array like objects should use the plural name to indicate that it is more than one entity.

    Be consistent in naming items. Do not call the same abstract different names in different locations. EG you called the list of requests lists and group. You called a request insert and putGroup. (see example)

  • Magic numbers and strings should be in one place so that they don't need to be hunted down to make changes. As they are separated from the code it pays to always comment these constants, include type, limits, and what they are used for. (see example)

    To ensure constants are not modified use object.freeze to make them read only. This instills trust in the state of the constants so the coder modifying the code does not have to hunt down each usage of the value to ensure it has not been modified if there are problems.

  • Reduce the complexity by reducing operator and token counts. Eg if (!foo.length) { a = b } else { b = a } is better as if (foo.length) { b = a } else { a = b }

    Reduce the number of return points and keep it DRY. Eg in two places you create a new group and return it. With a little rearranging of the logic the two can be one. (see example)

Readability

This comes hand in hand with maintainability. Low readability also means low maintainability.

Example

not quite how I would have written it (I would use a while loop and have the param requests AKA lists as a safe to mutate copy). Also would have used default params for the constants. However the example is consistent with your original.

I also prefer 4 space indent as I find it much more readable with my old eyes.

const grouping = Object.freeze({ // Request grouping constants for buildGroupRequests
    MAX_SIZE: 50,     // Max length of string per group. Last item may be longer.
    DELIMITER: " + ", // String used to separate groups. Len < MAX_SIZE
    PREFIX: "PUT ",   // String used to prefix groups. Len < MAX_SIZE
});

function buildGroupRequests(requests) {
    return requests.reduce((groupedReqs, request) => {
        if (groupedReqs.length) {
            const lastReq = groupedReqs[groupedReqs.length - 1];
            const putGroup = lastReq.put + grouping.DELIMITER + request.put;
            if (putGroup.length <= grouping.MAX_SIZE) {
                lastReq.put = putGroup;
                return groupedReqs;
            }
        }
        groupedReqs.push({put: grouping.PREFIX + request.put});
        return groupedReqs;
    }, []);
}



console.log(
    buildGroupRequests([
        { put: "/firstName=Danny" },
        { put: "/lastName=Williams" },
        { put: "/email=danny.w@domain.com" },
        { put: "/address=1984 Some St, Very long named place. Woop Woop 6162" }
    ])
);

Update

As requested in comment. Using a while loop and more along the lines as I would write it to be consistent with internal standards. Please note this is not as maintainable if used in a general JS work environment.

const grouping = Object.freeze({ 
    maxSize: 50, 
    delimiter: " + ",
    prefix: "PUT ",   
    requests(reqs, {maxSize: max, delimiter: del, prefix: pre} = grouping) {
        var prev = {put: pre + reqs.shift().put};
        const grouped = [prev];
        while (reqs.length) {
            const next = reqs.shift().put;
            const putGroup = prev.put + del + next;
            if (putGroup.length <= max) { prev.put = putGroup }
            else { grouped.push(prev = {put: pre + next}) }
        }
        return grouped;
    },
});


const requests = [
    { put: "/firstName=Danny" },
    { put: "/lastName=Williams" },
    { put: "/email=danny.w@domain.com" },
    { put: "/address=1984 Some St, Very long named place. Woop Woop 6162" }
];

console.log( grouping.requests([...requests]));

\$\endgroup\$
  • 1
    \$\begingroup\$ I think you mean "PascalCase" rather than "Pascal_Case". \$\endgroup\$ – MJ713 Jul 12 at 20:51
  • \$\begingroup\$ @MJ713 yes thank you for spotting it.. \$\endgroup\$ – Blindman67 Jul 12 at 20:56
  • \$\begingroup\$ Very nice answer, thank you. Could you provide an example how you would do it in while loop as well? \$\endgroup\$ – user11767153 Jul 12 at 21:32
  • \$\begingroup\$ @user11767153 I have added the while loop version under Update \$\endgroup\$ – Blindman67 Jul 12 at 22:14
  • \$\begingroup\$ @Blindman67 Thank you. I feel your first example is much cleaner and understandability. \$\endgroup\$ – user11767153 Jul 12 at 22:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.