1
\$\begingroup\$

As part of a web-based claims system I have created, there is a view for logging telephone calls. If a call is incoming, the claims handler asks the caller a series of data protection questions to validate who they are. The questions are generated from a database and each question has one or more valid answers. The valid answers are also generated from the database.

Example view:

enter image description here

Things of note for my question:

  • In addition to the valid answers, there are also 'Unanswered' and 'Incorrect Response' answers. These are added via client code and are not part of the original valid answers generated by the database.
  • Each valid answer has a 'check' icon
  • The invalid answer has a 'cross' icon
  • The unanswered answer has no icon

Here's my client code for each type of answer:

class Answer {
    constructor(answer, state) {
        this.el = document.createElement('option');
        this.el.value = answer;
        this.el.textContent = answer;

        state.apply(this);
    }
}

class AnswerState {
    constructor(validationIcon, validationColor) {
        this.validationIcon = validationIcon;
        this.validationColor = validationColor;
    }

    apply(answer) {
        answer.el.setAttribute('data-validation-icon', this.validationIcon);
        answer.el.setAttribute('data-validation-color', this.validationColor);
    }
}

class UnansweredState extends AnswerState {
    apply(answer) {
        super.apply(answer);
        answer.el.setAttribute('data-unanswered', '');
    }
}

class ValidAnswerState extends AnswerState {
    apply(answer) {
        super.apply(answer);
        answer.el.setAttribute('data-valid', '');
        answer.el.setAttribute('data-icon', 'icons/round-done-24px.svg');
    }
}

class InvalidAnswerState extends AnswerState {
    apply(answer) {
        super.apply(answer);
        answer.el.setAttribute('data-invalid', '');
        answer.el.setAttribute('data-icon', 'icons/round-clear-24px.svg');
    }
}

export { Answer, UnansweredState, ValidAnswerState, InvalidAnswerState };

And this is how I create the answer select element for each question:

function createAnswerSelect(validAnswers) {
    const select = document.createElement('select');
    const unansweredState = new UnansweredState('remove', 'grey');
    const invalidAnswerState = new InvalidAnswerState('clear', 'red');
    const validAnswerState = new ValidAnswerState('done', 'green');

    select.add(new Answer('Unanswered', unansweredState).el);

    validAnswers.forEach(answer => {
        select.add(new Answer(answer.answer, validAnswerState).el);
    });

    select.add(new Answer('Incorrect response', invalidAnswerState).el);

    return select;
}

Is this a reasonable way to encapsulate the variability of the different type of answers (unanswered, valid, invalid)? It feels like 'unanswered' should be the state of a question, not an answer.

As per usual, I am questioning myself. The code works, but I have a nagging feeling it is a 'bad' design somehow.

\$\endgroup\$
  • \$\begingroup\$ What you're doing is predefining the final 'state' of the question; 'not answered', 'incorrect', and 'correct', already in the answer options, using data-* properties. It is a practical solution, and I have no problem seeing that as a part of an answer. \$\endgroup\$ – KIKO Software Apr 13 '19 at 11:25
1
\$\begingroup\$

Object literals

By the looks this is client side UI only and the actual results are stored in the database.

Your concern is not that of the data structure, but rather one of representation.

The task is defined by what you have in createAnswerSelect(validAnswers) {

Your code is attempting to imitate a higher level abstraction unrelated to the task you need to complete. This has forced you to presented 4 additional interfaces to the scope you are working in Answer, UnansweredState, ValidAnswerState, InvalidAnswerState that you are unsure of.

Javascript lets you define objects as literals, you can side step the noise related to defining named Objects, Prototypes, functions, calls to super etc, yet still keep the same underlying relationship of shared and unique state.

Example

The best code is that which does it quickly and as simply as possible. Creating an inherited structure just to hold some shared and unique states is too complex.

This example is an alternative (based on the information you have provided)

It defines the 3 states as named constants that access state information via a Map of named states, rather than the 3 objects you defined UnansweredState, ValidAnswerState, InvalidAnswerState

const UNANSWERED = 1;
const INVALID = 2;
const VALID = 3;

const createElement = (type, opts = {}) => Object.assign(document.createElement(type), opts);
const common = (icon, color) => ({icon, color});
const states = new Map([
  [UNANSWERED, {...common("remove", "grey"), data: {unanswered: ""}}],
  [INVALID, {...common("clear", "red"), data: {valid: "", icon: "icons/round-done-24px.svg"}}],
  [VALID, {...common("done", "green"), data: {invalid: "", icon: "icons/round-clear-24px.svg"}}],
]);
const option = (answer, stateId) => {
  const state = states.get(stateId);
  const option = createElement("option", {value: answer, textContent: answer });
  Object.assign(option.dataset, state.data);
  option.dataset.validationIcon = state.icon;
  option.dataset.validationColor = state.color;   
  return option;
}        

function createAnswerSelect(answers) {
  const select = createElement('select');
  select.add(option("Unanswered", UNANSWERED));
  answers.forEach(answer => select.add(option(answer.answer, VALID)) );
  select.add(option("Incorrect response", INVALID));
  return select;           
}          
export { createAnswerSelect };
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for responding. I take your point that creating an inheritance hierarchy, in this instance, is probably overkill. But I would argue that my solution is easier to read and understand (to my eyes, at least), which is useful to me if I need to revisit it at some point later. Granted, my question was not ‘how readable is my code?’. Nonetheless, it is something I consider important. \$\endgroup\$ – datahandler Apr 13 '19 at 19:36

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.