3
\$\begingroup\$

This function takes a list of questions and list of answers provided by the user.

The list of answers is always a list of booleans (for true and false) and the list of questions takes the following form:

{ quadrant: Number }

If the user answers "true" to a question then they gain a point towards that quadrant. (Think of it like a personality test). If they answer false, nothing happens.

The quadrants are arranged along two axes like this:

Q1 | Q4
---+---
Q2 | Q3

The X axis is called "Warmth" and the Y axis is called "Dominance".

This function aims to calculate which quadrant you are in given your answers. If you are on a boundary, it returns an array with the quadrants that you span. If there is a 4-way tie, Quadrant 4 is assumed.

It looks very messy and difficult to read. How could I improve it? I'd specifically like to pull the whole thing into a reduce function if I can to avoid the nasty if/else tree at the end.

calculateResult = (questions, answers) => {
    const res = _.zip(questions, answers)
      .filter(x => x[1]) // Only get correct answers
      .map(x => x[0].quadrant) // Pluck the quadrant number
      .reduce(
        (res, x) => {
          if (x == 1 || x == 4) {
            res.dominance++;
          } else {
            res.dominance--;
          }

          if (x == 3 || x == 4) {
            res.warmth++;
          } else {
            res.warmth--;
          }
          return res;
        },
        {
          dominance: 0,
          warmth: 0
        }
      );

    if (res.dominance > 0) {
      if (res.warmth > 0) {
        return [4];
      } else if (res.warmth < 0) {
        return [1];
      } else {
        return [1, 4];
      }
    } else if (res.dominance < 0) {
      if (res.warmth > 0) {
        return [3];
      } else if (res.warmth < 0) {
        return [2];
      } else {
        return [2, 3];
      }
    } else {
      if (res.warmth > 0) {
        return [3, 4];
      } else if (res.warmth < 0) {
        return [1, 2];
      } else {
        return [4];
      }
    }
};

Test Cases

The "questions" parameter pretty much always looks the same. (The program pulls 3 questions from each quadrant)

var questions = [
  { quadrant: 1 },
  { quadrant: 1 },
  { quadrant: 1 },
  { quadrant: 2 },
  { quadrant: 2 },
  { quadrant: 2 },
  { quadrant: 3 },
  { quadrant: 3 },
  { quadrant: 3 },
  { quadrant: 4 },
  { quadrant: 4 },
  { quadrant: 4 }
];

The "answers" parameter is always a list of booleans denoting true/false. Here are some test cases from my test suite.

const answersForQuadOne = [ // Should return [ 1 ]
  true,
  true,
  true,
  false,
  false,
  false,
  false,
  false,
  false,
  false,
  false,
  false
];

const answersOnBoundary = [ // Boundary should return [ 1, 2 ]
  true,
  true,
  true,
  true,
  true,
  true,
  false,
  false,
  false,
  false,
  false,
  false
];
\$\endgroup\$
  • 1
    \$\begingroup\$ This is interesting... You might want to add a test case (sample questions, sample answers, sample result) so that everyone can gauge their answers. \$\endgroup\$ – Joseph Mar 6 '17 at 17:02
  • 1
    \$\begingroup\$ @JosephtheDreamer That's a great idea, I'll do that \$\endgroup\$ – Steve Mar 6 '17 at 17:22
  • \$\begingroup\$ shouldn't the final return you provide be [1,2,3,4]? \$\endgroup\$ – Vogel612 Mar 6 '17 at 17:23
  • 1
    \$\begingroup\$ @Vogel612 No, sorry I forgot to mention. If there is a 4-way tie, Q4 is assumed. I will edit the Q \$\endgroup\$ – Steve Mar 6 '17 at 17:29
1
\$\begingroup\$

Let me review your code first and then provide an alternative:

  • The naming for the reduce-function is a little confusing, since you're just using x. Generally the lambda parameters are problematically named. Only fixing the naming should already give huge boosts to maintainability:

    const res = _.zip(questions, answers)
        .filter(qa => qa[1])
        .map(qa => qa[0].quadrant)
        .reduce(
            (res, quadrant) => {
            [...]
        );
    

that was the simple part. Along the way we kind of made the comments in the code superfluous. I'll not review the assignment of the return quadrants though it seems like there could be some simplifications if you didn't try to directly return a hardcoded array.


Let's reexamine your approach. You already stated that you're basically trying to accumulate increments and decrements along two axes. Let's consider the following then:

.filter(qa => qa[1])
.map(qa => {
  switch (qa[0].quadrant) {
     case 1:
       return { dominance: 1, warmth: -1 };
     case 2:
       return { dominance: -1, warmth: -1 };
     case 3:
       return { dominance: -1, warmth: 1 };
     case 4:
       return { dominance: 1, warmth: 1 };
     default: 
       return { dominance: 0, warmth: 0 };
  }
}).reduce(
  (res, assessment) => {
    return { dominance: res.dominance + assessment.dominance,
             warmth: res.warmth + assessment.warmth }
  },
  { dominance: 0, warmth: 0 }
);

This clarifies your logic tremendously and makes obvious to the reader which quadrant entails which change in assessment

\$\endgroup\$
1
\$\begingroup\$

Why bother calculating dominance and warmth if you want to return quadrants anyway? Count questions per quadrant and return those which have maximum:

const quadrants = _.zip(questions, answers)
    .filter(qa => qa[1])
    .map(qa => qa[0].quadrant);

const countByQuadrant = _.countBy(quadrants, _.identity);
const maxCount = _.max(_.values(countByQuadrant));
const quadrantsWithMaxCount = _.keys(
    _.filter(countByQuadrant, count => count === maxCount)
);

return quadrantsWithMaxCount.length === 4 ? [4] : quadrantsWithMaxCount;

Assuming _ to be Lodash.

\$\endgroup\$

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.