-2
\$\begingroup\$

I have this watcher in my app.vue that makes sure the right theme attribute is set on the html tag. I have a nested if/else loop, and as you can see in the code below I have two different conditions but two times the same statements.

Is there some way to code this is a more reusable way? I can imagine in some cases there are going to be more conditions with maybe the same statements.

  created() {
    const media = window.matchMedia("(prefers-color-scheme: dark)");

    this.$store.watch(
      (state) => {
        return state.settings.colorSettings.automatic;
      },
      (currentAutomaticValue) => {
        if (currentAutomaticValue) {
          media.addEventListener("change", this.setTheme);

          window.matchMedia &&
          window.matchMedia("(prefers-color-scheme: dark)").matches
            ? setThemeAttribute(themes.dark) // 1
            : setThemeAttribute(themes.default); // 2
        } else {
          media.removeEventListener("change", this.setTheme);

          this.colorSettings.darkMode
            ? setThemeAttribute(themes.dark) // 1
            : setThemeAttribute(themes.default); // 2
        }
      },
      { immediate: true }
    );
  },
\$\endgroup\$
2
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    Jul 29, 2021 at 14:41
  • 1
    \$\begingroup\$ What does the method setThemeAttribute do? \$\endgroup\$ Jul 29, 2021 at 14:47

1 Answer 1

2
\$\begingroup\$

So you have a boolean, and from this boolean you go with either themes.dark or themes.default. Easy, extract a method that takes a boolean and properly calls setThemeAttribute.

function setThemeBoolean(bool) {
    setThemeAttribute(bool ? themes.dark : themes.default);
}
...
if (currentAutomaticValue) {
    media.addEventListener("change", this.setTheme);
    setThemeBoolean(window.matchMedia &&
      window.matchMedia("(prefers-color-scheme: dark)").matches);
} else {
    media.removeEventListener("change", this.setTheme);
    setThemeBoolean(this.colorSettings.darkMode);
}
\$\endgroup\$
3
  • \$\begingroup\$ This is the best answer I could come up with, but I'd recommend posting more context next time. I just have a feeling that you're not doing things in the most optimal way. I suspect the whole this.$store.watch might not be necessary. \$\endgroup\$ Jul 29, 2021 at 14:51
  • \$\begingroup\$ Modern JS?? matchMedia has full support caniuse.com/matchmedia so no need to predicate its existence, you should use the optional chaining operator if needing to call an unknown. EG matchMedia?.("foo").matches, and window is the default globalThis so its use is almost always redundant. Thus the split line becomes setThemeBoolean(matchMedia("foo").matches) \$\endgroup\$
    – Blindman67
    Jul 30, 2021 at 6:26
  • 3
    \$\begingroup\$ @Blindman67 I just addressed the main question, I'd recommend you post your findings in another answer. \$\endgroup\$ Jul 30, 2021 at 12:20

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.