2
\$\begingroup\$

I have a form in Laravel that goes over several templates (Blade files). For each template I am implementing simple conditional logic to show and hide groups of questions.

For the templates, I'm using HTML5 for efficient client-side validation. I wrote the Javascript below to show and hide fields using HTML attributes. The script is working great.

The example below responds to a yes/no question by exposing hidden sets of radio buttons. One set is for answering Yes, one is for No.

My question is, do I need to refactor this? Is there a more efficient way I should be writing this Javascript, or am I doing ok?

I'm using an event listener so that clicks can bubble up, understanding that this is most efficient. Since I have more than one page, I have more than one event listener, one for each template. Is there a better way to do that?

// "Purpose of Notification" Blade
document.addEventListener('click', function (event) {
  // When yes or no is choosen
  if (event.target.matches('#waiver30100byes')) {
    // show the patient level group container
    document.getElementById("patient-level").removeAttribute("hidden");
    // and show 100 level for yes and make it required
    document.getElementById("patient-level-100").removeAttribute("hidden");
    document.getElementById("level1").required = true;
    // hide and un-require the 30 level radio group
    document.getElementById("patient-level-30").hidden = true;
    document.getElementById("level3").required = false;
  }
  else if (event.target.matches('#waiver30100bno')) {
    // show the patient level group
    document.getElementById("patient-level").removeAttribute("hidden");
    // and show 30 level for no and make it required
    document.getElementById("patient-level-30").removeAttribute("hidden");
    document.getElementById("level3").required = true;
    //hide and un-required the 100 level radio group
    document.getElementById("patient-level-100").hidden = true;
    document.getElementById("level1").required = false;
    //
  }
}, false);
\$\endgroup\$
3
  • \$\begingroup\$ I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have. \$\endgroup\$ Feb 23 at 17:06
  • \$\begingroup\$ Thanks @Toby, whatever works best here is fine with me. This is my first question on Code Review-- folks over on regular SE recommended it. My question was only about refactoring, though, not about HOW to do it. If that new title you put in is best, I'm good with it. \$\endgroup\$
    – LeraA
    Feb 23 at 17:08
  • \$\begingroup\$ This is code review, where we review the code you have written. We might suggest that it needs to be refactored, but you can't request that - the help center should clarify what to expect, or you could look at recent high-scoring questions. \$\endgroup\$ Feb 24 at 7:14

1 Answer 1

2
\$\begingroup\$

I will create some function to abstract the functionality of the codes like below:

// "Purpose of Notification" Blade
document.addEventListener(
    "click",
    function (event) {
        // When yes or no is choosen
        if (event.target.matches("#waiver30100byes")) {
            showParentLevelContainer();
            show100LevelRadioGrp();
            addReqLevel1();
            hideLevel30RadioGrp();
            removeReqLevel3();
        } else if (event.target.matches("#waiver30100bno")) {
            showParentLevelContainer();
            showLevel30RadioGrp();
            addReqLevel3();
            hide100LevelRadioGrp();
            removeReqLevel1();
        }
    },
    false
);

function hideLevel30RadioGrp() {
    document.getElementById("patient-level-30").hidden = true;
}

function showLevel30RadioGrp() {
    document.getElementById("patient-level-30").removeAttribute("hidden");
}

function addReqLevel1() {
    document.getElementById("level1").required = true;
}

function removeReqLevel1() {
    document.getElementById("level1").required = false;
}

function addReqLevel3() {
    document.getElementById("level3").required = true;
}

function removeReqLevel3() {
    document.getElementById("level3").required = false;
}

function showParentLevelContainer() {
    document.getElementById("patient-level").removeAttribute("hidden");
}

function show100LevelRadioGrp() {
    document.getElementById("patient-level-100").removeAttribute("hidden");
}

function hide100LevelRadioGrp() {
    document.getElementById("patient-level-100").hidden = true;
}
\$\endgroup\$
4
  • \$\begingroup\$ Is it better, since it requires more lines of code and only gets used once on the page? I confess to not knowing which is better. A nice abstraction or fewer lines of code. \$\endgroup\$
    – LeraA
    Feb 23 at 16:35
  • 1
    \$\begingroup\$ @LeraA. If one day you want to change the id patient-level-100 to level-100, you don't need to change for each line but one line only. But ofc, it is up to your use case. \$\endgroup\$
    – ikhvjs
    Feb 23 at 16:40
  • \$\begingroup\$ I'm going to try this abstraction. Thank you for taking the time. \$\endgroup\$
    – LeraA
    Feb 23 at 16:45
  • \$\begingroup\$ I@ikhvjs's abstraction is working great. Will apply the technique to all of the blades now. \$\endgroup\$
    – LeraA
    Feb 23 at 17:26

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.