DEV Community

Cover image for Refactoring A Junior’s React Code - Reduced Complexity With A Single Source Of Truth
Johannes Kettmann
Johannes Kettmann

Posted on • Originally published at profy.dev

Refactoring A Junior’s React Code - Reduced Complexity With A Single Source Of Truth

#react #javascript #webdev #frontend

As an (aspiring) Junior React developer it’s hard to understand if you’re code is any good and how you can improve it. It would be great to get a review from an experienced dev. But if you don’t work in a team or have the funds to pay a mentor your chances are low.

Still, you can learn from reviews of other developers’ code. And that’s the goal of this series of articles and videos.

This time, we step by step review and refactor the code of one of my students. By

  • using a single source of truth
  • replacing a buggy feature with a simpler solution
  • and removing programmatic styles in favor of simple CSS

we greatly reduce the complexity of the code, simplify the data flow, and flush out a few bugs along the way.

If you’re rather a video person you can watch the complete review and refactoring session here.

If you’re interested in getting a code review like this yourself feel free to reach out to me at review@profy.dev. I’m always looking for more examples. To increase your chances request a review of one feature rather than a whole application.

Table Of Contents

  1. The Component To Review
    1. The UI & Functionality
    2. The Filters Component
  2. Code Review: The Filter Values
    1. How Are The Filter Values Used?
    2. How Are The Filter Values Updated?
    3. Where Are The Filter Values Stored?
    4. The Data Flow
    5. The Core Problem: Duplicate Data
    6. An Alternative Solution
  3. Refactoring: A Single Source Of Truth
    1. Deriving The Filters Values From The URL
    2. Updating The Filters Directly In The URL
    3. A Layer Of Abstraction Made The Refactoring Simple
    4. Removing The useEffect
    5. Recap
  4. Code Review: Incorrect Default Values
    1. The Problem
    2. Refactoring: Initialize The Input Elements
  5. Code Review: The Project Name Autocomplete
    1. The Problem
    2. Refactoring: Remove Project Input Autocomplete
  6. Code Review: Programmatic Styles
    1. The Problem
    2. Refactoring: Fix Select Overlay Workaround
    3. Refactoring: Replace Programmatic Mobile Styles With CSS
  7. Final Result

The Component To Review

The UI & Functionality

The feature to be reviewed is the filter bar as highlighted in this screenshot. It is used to filter the data in the table.

This happens when the user selects a filter (try it out yourself here):

  1. a new request is sent to an API
  2. the table updates with the new data
  3. the selected filter value is added to the URL (as shown below).

The first impression: The UI looks very good. The functionality also works as expected. At least at first glance.

The Filters Component

Now that we’ve seen the UI let’s have a look at the code. Here is the Filters component:

Try to review the code yourself first. What problems do you see and can you find alternative solutions? You can find the complete code here on GitHub.

import React, {
  useState,
  useEffect,
  useCallback,
  useRef,
  useContext,
} from "react";
import { useRouter } from "next/router";
import { useWindowSize } from "react-use";
import { Select, Option, Input, NavigationContext } from "@features/ui";
import { useFilters } from "../../hooks/use-filters";
import { IssueLevel, IssueStatus } from "@api/issues.types";
import { useProjects } from "@features/projects";
import * as S from "./filters.styled";

export function Filters() {
  const { handleFilters, filters } = useFilters();
  const { data: projects } = useProjects();
  const router = useRouter();
  const routerQueryProjectName =
    (router.query.projectName as string)?.toLowerCase() || undefined;
  const [inputValue, setInputValue] = useState<string>("");
  const projectNames = projects?.map((project) => project.name.toLowerCase());
  const isFirst = useRef(true);
  const { width } = useWindowSize();
  const isMobileScreen = width <= 1023;
  const { isMobileMenuOpen } = useContext(NavigationContext);
  const handleChange = (input: string) => {
    setInputValue(input);

    if (inputValue?.length < 2) {
      handleProjectName(undefined);
      return;
    }

    const name = projectNames?.find((name) =>
      name?.toLowerCase().includes(inputValue.toLowerCase())
    );

    if (name) {
      handleProjectName(name);
    }
  };

  const handleLevel = (level?: string) => {
    if (level) {
      level = level.toLowerCase();
    }
    handleFilters({ level: level as IssueLevel });
  };

  const handleStatus = (status?: string) => {
    if (status === "Unresolved") {
      status = "open";
    }
    if (status) {
      status = status.toLowerCase();
    }
    handleFilters({ status: status as IssueStatus });
  };

  const handleProjectName = useCallback(
    (projectName?: string) =>
      handleFilters({ project: projectName?.toLowerCase() }),
    [handleFilters]
  );

  useEffect(() => {
    const newObj: { [key: string]: string } = {
      ...filters,
    };

    Object.keys(newObj).forEach((key) => {
      if (newObj[key] === undefined) {
        delete newObj[key];
      }
    });

    const url = {
      pathname: router.pathname,
      query: {
        page: router.query.page || 1,
        ...newObj,
      },
    };

    if (routerQueryProjectName && isFirst) {
      handleProjectName(routerQueryProjectName);
      setInputValue(routerQueryProjectName || "");
      isFirst.current = false;
    }

    router.push(url, undefined, { shallow: false });
  }, [filters.level, filters.status, filters.project, router.query.page]);

  return (
    <S.Container>
      <Select
        placeholder="Status"
        defaultValue="Status"
        width={isMobileScreen ? "97%" : "8rem"}
        style={{
          ...(isMobileMenuOpen && {
            opacity: 0,
          }),
        }}
      >
        <Option value={undefined} handleCallback={handleStatus}>
          --None--
        </Option>
        <Option value="Unresolved" handleCallback={handleStatus}>
          Unresolved
        </Option>
        <Option value="Resolved" handleCallback={handleStatus}>
          Resolved
        </Option>
      </Select>

      <Select
        placeholder="Level"
        defaultValue="Level"
        width={isMobileScreen ? "97%" : "8rem"}
        style={{
          ...(isMobileMenuOpen && {
            opacity: 0,
          }),
        }}
      >
        <Option value={undefined} handleCallback={handleLevel}>
          --None--
        </Option>
        <Option value="Error" handleCallback={handleLevel}>
          Error
        </Option>
        <Option value="Warning" handleCallback={handleLevel}>
          Warning
        </Option>
        <Option value="Info" handleCallback={handleLevel}>
          Info
        </Option>
      </Select>

      <Input
        handleChange={handleChange}
        value={inputValue}
        label="project name"
        placeholder="Project Name"
        iconSrc="/icons/search-icon.svg"
        style={{
          ...(isMobileScreen && { width: "94%", marginRight: "3rem" }),
          ...(isMobileMenuOpen && {
            opacity: 0,
          }),
        }}
      />
    </S.Container>
  );
}
Enter fullscreen mode Exit fullscreen mode

There’s a lot going on. Especially the top of the component is pretty crowded with lots of variables with unclear responsibilities. So let’s review and refactor the code one by one.

Code Review: The Filter Values

We start with the elephant in the room: The filter values. Let’s try to answer some basic questions regarding the data flow: How are the filter values used, how are they updated, and where are they stored?

How Are The Filter Values Used?

The filters object comes from the useFilters() hook and is only used in the useEffect to update the query params in the URL.

Note: The code is a bit easier to read in the original article since the important lines are highlighted there. This is not supported here as far as I know.

// ./components/filters.tsx

export function Filters() {
  const { handleFilters, filters } = useFilters();

  useEffect(() => {
    const newObj: { [key: string]: string } = {
      ...filters,
    };

    Object.keys(newObj).forEach((key) => {
      if (newObj[key] === undefined) {
        delete newObj[key];
      }
    });

    const url = {
      pathname: router.pathname,
      query: {
        page: router.query.page || 1,
        ...newObj,
      },
    };

    if (routerQueryProjectName && isFirst) {
      handleProjectName(routerQueryProjectName);
      setInputValue(routerQueryProjectName || "");
      isFirst.current = false;
    }

    router.push(url, undefined, { shallow: false });
  }, [filters.level, filters.status, filters.project, router.query.page]);

  ...

  // interestingly the filters aren't used in the components at all
  return (
    <S.Container>
      <Select
        placeholder="Status"
        defaultValue="Status"
        width={isMobileScreen ? "97%" : "8rem"}
        style={{
          ...(isMobileMenuOpen && {
            opacity: 0,
          }),
        }}
      >
        <Option value={undefined} handleCallback={handleStatus}>
          --None--
        </Option>
        <Option value="Unresolved" handleCallback={handleStatus}>
          Unresolved
        </Option>
        <Option value="Resolved" handleCallback={handleStatus}>
          Resolved
        </Option>
      </Select>

      ...
    </S.Container>
  );
}
Enter fullscreen mode Exit fullscreen mode

Interestingly the filters aren’t used in any of the component props. And as we’ll see later, this causes a bug.

How Are The Filter Values Updated?

The handleFilters function that is returned by the useFilters hook is used in the change callbacks. These are connected to the Select and Input components.

// ./components/filters.tsx

export function Filters() {
  const { handleFilters, filters } = useFilters();

  ...

  const handleLevel = (level?: string) => {
    if (level) {
      level = level.toLowerCase();
    }
    handleFilters({ level: level as IssueLevel });
  };

  const handleStatus = (status?: string) => {
    if (status === "Unresolved") {
      status = "open";
    }
    if (status) {
      status = status.toLowerCase();
    }
    handleFilters({ status: status as IssueStatus });
  };

  const handleProjectName = useCallback(
    (projectName?: string) =>
      handleFilters({ project: projectName?.toLowerCase() }),
    [handleFilters]
  );

  ...

  // interestingly the filters aren't used in the components at all
  return (
    <S.Container>
      <Select
        placeholder="Status"
        defaultValue="Status"
        width={isMobileScreen ? "97%" : "8rem"}
        style={{
          ...(isMobileMenuOpen && {
            opacity: 0,
          }),
        }}
      >
        <Option value={undefined} handleCallback={handleStatus}>
          --None--
        </Option>
        <Option value="Unresolved" handleCallback={handleStatus}>
          Unresolved
        </Option>
        <Option value="Resolved" handleCallback={handleStatus}>
          Resolved
        </Option>
      </Select>

      ...
    </S.Container>
  );
}
Enter fullscreen mode Exit fullscreen mode

Note that the change callbacks look a bit strange as they have to transform the input parameter before using it as a filter value. The Select component also looks counterintuitive with the handleCallback prop being set on the Option component. But that’s a topic for later.

Indirectly handleFilters is also called inside the useEffect whenever there’s a projectName query param in the URL.

// ./components/filters.tsx

export function Filters() {
  const router = useRouter();
  const routerQueryProjectName =
    (router.query.projectName as string)?.toLowerCase() || undefined;
  const isFirst = useRef(true);

  ...

  useEffect(() => {
    const newObj: { [key: string]: string } = {
      ...filters,
    };

    Object.keys(newObj).forEach((key) => {
      if (newObj[key] === undefined) {
        delete newObj[key];
      }
    });

    const url = {
      pathname: router.pathname,
      query: {
        page: router.query.page || 1,
        ...newObj,
      },
    };

    if (routerQueryProjectName && isFirst) {
      handleProjectName(routerQueryProjectName);
      setInputValue(routerQueryProjectName || "");
      isFirst.current = false;
    }

    router.push(url, undefined, { shallow: false });
  }, [filters.level, filters.status, filters.project, router.query.page]);

  return (
    <S.Container>
      ...
    </S.Container>
  );
}
Enter fullscreen mode Exit fullscreen mode

After some testing the highlighted line never seems to be executed though. As mentioned, the rest of the useEffect is responsible for updating the query parameters in the URL whenever a filter changes.

There’s also a small bug as isFirst in the condition is always truthy. isFirst.current should have been used instead.

Where Are The Filter Values Stored?

Now we know how the filters object and the handleFilters function are used. But where are they defined? This is a good time to have a look at the useFilters hook. The hook itself simply exposes a context.

// ./hooks/use-filters.ts

import { useContext } from "react";
import { FiltersContext } from "../context/filters-context";

export const useFilters = () => useContext(FiltersContext);
Enter fullscreen mode Exit fullscreen mode

The context itself isn’t very complex either. Basically, it stores the filter values in a state and exposes a function to update this state (aka the handleFilters function).

// ./context/filters-context.tsx

import React, {
  useState,
  useMemo,
  useCallback,
  createContext,
  ReactNode,
} from "react";
import { IssueFilters } from "@api/issues.types";

export const FiltersContext = createContext<{
  filters: IssueFilters;
  handleFilters: (filter: IssueFilters) => unknown;
}>({
  filters: { status: undefined, level: undefined, project: undefined },
  handleFilters: (_filter: IssueFilters) => {},
});

type FiltersProviderProps = {
  children: ReactNode | ReactNode[];
};

export function FiltersProvider({ children }: FiltersProviderProps) {
  const [filters, setFilters] = useState<IssueFilters>({
    status: undefined,
    level: undefined,
    project: undefined,
  });

  const handleFilters = useCallback(
    (filter: any) =>
      setFilters((prevFilters) => ({ ...prevFilters, ...filter })),
    []
  );

  const memoizedValue = useMemo(
    () => ({ filters, handleFilters }),
    [filters, handleFilters]
  );

  return (
    <FiltersContext.Provider value={memoizedValue}>
      {children}
    </FiltersContext.Provider>
  );
}
Enter fullscreen mode Exit fullscreen mode

There are likely some opportunities for refactoring here. For example, the usage of useCallback and useMemo might not be necessary, and some of the types could be improved (esp. any). But as we’ll see in a bit we can completely replace this code with a simpler solution.

To get to that solution, let’s have a closer look at the data flow.

The Data Flow

The current filters data is defined in a context and in the query parameters in the URL. Here you can see how this data changes:

  1. After the initial render the filters are undefined both inside the context and the URL.
  2. Whenever the user changes a filter value the state in the context is updated.
  3. The state update triggers a re-render and executes the useEffect. This in turn updates the query parameters in the URL via router.push(...).

The last re-render after the update of the URL isn’t really important at the moment. But it could become a performance issue in the future. So we should keep it in mind.

The Core Problem: Duplicate Data

The core problem that leads to a lot of complexity including the useEffect is the duplication of the filter values:

  1. The filters are stored in a state inside a context.
  2. The filters are also stored in the URL.

The question is: Can we either get rid of the state or the URL parameters?

Removing the URL parameters isn’t an option. They are a hard requirement for the feature as they make the filter values persist a page refresh and allow the user to share a certain filtered view with another user.

But what about the state?

An Alternative Solution

Can we simply derive the filter values from the URL? E.g. like this:

const router = useRouter();
const filters = {
  status: router.query.status,
  ...
}
Enter fullscreen mode Exit fullscreen mode

That seems worth a try. When a user changes a filter value we could directly update the URL. And that in turn would update the derived filters object. The data flow would look like this:

This might not seem like a big change. But as you’ll see during the refactoring this will greatly simplify our code as we don’t need the context and the synchronization logic in the useEffect anymore.

React on the job - email course

Refactoring: A Single Source Of Truth

Deriving The Filters Values From The URL

Based on the proposal for the alternative solution we directly derive the filters variable from the URL’s query parameters instead of using the context. We can do all this in the useFilters hook.

// ./hooks/use-filters.ts

import { useRouter } from "next/router";
import { IssueFilters } from "@api/issues.types";

export const useFilters = () => {
  const router = useRouter();

  const filters = {
    status: router.query.status,
    level: router.query.level,
    project: router.query.project,
  } as IssueFilters;

  return { filters };
};
Enter fullscreen mode Exit fullscreen mode

Updating The Filters Directly In The URL

Next, we add the handleFilters function. It accepts a new filters object, builds a new query from this object and the existing query, and updates the URL via router.push(...).

// ./hooks/use-filters.ts

import { useRouter } from "next/router";
import { IssueFilters } from "@api/issues.types";

export const useFilters = () => {
  const router = useRouter();

  const filters = {
    status: router.query.status,
    level: router.query.level,
    project: router.query.project,
  } as IssueFilters;

  const handleFilters = (newFilters: IssueFilters) => {
    const query = { ...router.query, ...newFilters };
    router.push({ query });
  };

  return { filters, handleFilters };
};
Enter fullscreen mode Exit fullscreen mode

Now that the hook doesn’t use the context anymore we can completely remove it (including the old filters state). Of course, we also need to remove all references to the context from the rest of the app (e.g. the FiltersProvider). But I won’t show that here.

A Layer Of Abstraction Made The Refactoring Simple

There’s one thing that’s easy to miss: the author of the original code did a great job of introducing a layer of abstraction by creating the useFilters hook.

We were able to refactor the internals of the hook and get rid of the context and its state. But since we return the same object from the hook with the same filters object and handleFilters function as before we didn’t change the API of the hook.

This way we were able to change the complete filters implementation without changing a single line in the Filters component.

This is a great example of how custom hooks can greatly improve the maintainability of your code. Kudos to the author.

Removing The useEffect

While we don’t need to change the Filters component we can get rid of one of the main complexities in the component: the useEffect that was responsible for updating the query parameters in the URL.

// ./components/filters.tsx

export function Filters() {
  const { handleFilters, filters } = useFilters();

  const [inputValue, setInputValue] = useState<string>("");
  const projectNames = projects?.map((project) => project.name.toLowerCase());
  const { data: projects } = useProjects();

  const { width } = useWindowSize();
  const isMobileScreen = width <= 1023;
  const { isMobileMenuOpen } = useContext(NavigationContext);

  const handleChange = (input: string) => {...};

  const handleLevel = (level?: string) => {...};

  const handleStatus = (status?: string) => {...};

  const handleProjectName = useCallback(...);

  return (
    <S.Container>
      ...
    </S.Container>
  );
}
Enter fullscreen mode Exit fullscreen mode

Recap

By simply deriving the filter values directly from the URL instead of keeping them in a state we were able to reduce the complexity and remove ~80 lines of code (the useEffect and the context). You can find the complete code changes here on GitHub.

This was possible because of our analysis of the data flow and a simple change to storing the filter values. The rest of this review & refactoring session won’t be that impactful to be honest. So you’re key takeaway from this article should be:

Use a single source of truth whenever possible. You’ll get around a lot of complicated and error-prone syncing logic (the useEffect in our case). Duplicate data is very common and might not be easy to detect. But whenever you encounter a useEffect pay close attention.

Now lets go ahead with the next step of the code review.

React on the job - email course

Code Review: Incorrect Default Values

As we’ve already seen, the filter values aren’t used in any of the Select or Input components. The result is that these components aren’t initialized with the correct values from the URL. They always show a hard-coded default value.

The Problem

This seems like an easy fix at first. And for the “Project Name” input that’s true as it is controlled via a state in the Filters component.


export function Filters() {
  const { handleFilters, filters } = useFilters();
  const [inputValue, setInputValue] = useState("");

  ...
Enter fullscreen mode Exit fullscreen mode

But for the Select components, this turns out to be a bigger problem. Currently, the defaultValue prop is hard-coded.

<Select
  placeholder="Status"
  defaultValue="Status"
  width={isMobileScreen ? "97%" : "8rem"}
  style={{
    ...(isMobileMenuOpen && {
      opacity: 0,
    }),
  }}
>
  <Option value={undefined} handleCallback={handleStatus}>
    --None--
  </Option>
  <Option value="Unresolved" handleCallback={handleStatus}>
    Unresolved
  </Option>
  <Option value="Resolved" handleCallback={handleStatus}>
    Resolved
  </Option>
</Select>
Enter fullscreen mode Exit fullscreen mode

Ideally, we could control the Select component via a value prop. But unfortunately, its current implementation doesn’t allow that. So we’ll have to find a workaround until we change the Select component itself in an upcoming refactoring session.

Refactoring: Initialize The Input Elements

As mentioned, initializing the “Project Name” input is simple as this is controlled by the inputValue state.

// ./components/filters.tsx

export function Filters() {
  const { handleFilters, filters } = useFilters();

  const [inputValue, setInputValue] = useState(filters.project || "");

  ...
Enter fullscreen mode Exit fullscreen mode

But fixing the initial value of the Select components are a bit more difficult as we can only use the defaultValue prop.

We could obviously refactor the Select component itself but that’s outside the scope of this session. We will tackle that problem in an upcoming refactoring session though.

To get the default value for each Select we introduce two functions. Those return the current default value (”Status” and “Level”) if the corresponding filter is not set. Otherwise, they transform the filter value to match the text rendered in the corresponding Option component.

// ./components/filters.tsx

import { capitalize } from "lodash";
import { IssueFilters, IssueLevel, IssueStatus } from "@api/issues.types";

...

function getStatusDefaultValue(filters: IssueFilters) {
  if (!filters.status) {
    return "Status";
  }
  if (filters.status === IssueStatus.open) {
    return "Unresolved";
  }
  return "Resolved";
}

function getLevelDefaultValue(filters: IssueFilters) {
  if (!filters.level) {
    return "Level";
  }
  return capitalize(filters.level);
}

export function Filters() {
  const { handleFilters, filters } = useFilters();

  ...

  return (
    <S.Container>
      <Select
        placeholder="Status"
        defaultValue={getStatusDefaultValue(filters)}
        width={isMobileScreen ? "97%" : "8rem"}
        style={{
          ...(isMobileMenuOpen && {
            opacity: 0,
          }),
        }}
      >
        <Option value={undefined} handleCallback={handleStatus}>
          --None--
        </Option>
        <Option value="Unresolved" handleCallback={handleStatus}>
          Unresolved
        </Option>
        <Option value="Resolved" handleCallback={handleStatus}>
          Resolved
        </Option>
      </Select>

      <Select
        placeholder="Level"
        defaultValue={getLevelDefaultValue(filters)}
        width={isMobileScreen ? "97%" : "8rem"}
        style={{
          ...(isMobileMenuOpen && {
            opacity: 0,
          }),
        }}
      >
        <Option value={undefined} handleCallback={handleLevel}>
          --None--
        </Option>
        <Option value="Error" handleCallback={handleLevel}>
          Error
        </Option>
        <Option value="Warning" handleCallback={handleLevel}>
          Warning
        </Option>
        <Option value="Info" handleCallback={handleLevel}>
          Info
        </Option>
      </Select>

      ...
    </S.Container>
  );
}
Enter fullscreen mode Exit fullscreen mode

Having to use the functions getStatusDefaultValue and getLevelDefaultValue isn’t really pretty here. But during a refactoring, it’s important to advance step by step. We can still remove these functions once we have a better implementation of the Select component.

You can find all changes of this bugfix here on GitHub. Now let’s continue with the next part of the code review.

Code Review: The Project Name Autocomplete

The change handler connected to the project name input has some kind of autocomplete functionality. Below you can see it in action: The user only enters a part of the project name in the input but the complete name (”Frontend - Web”) is added to the URL.

What happens here? The Filters component loaded the existing projects from the API. When the user starts typing, the code tries to find a project that matches the given input. If there is one it uses its name as a query parameter.

Here is the corresponding code:

// ./components/filters.tsx

export function Filters() {
  ...

  const { data: projects } = useProjects();
  const projectNames = projects?.map((project) => project.name.toLowerCase());
  const [inputValue, setInputValue] = useState<string>("");

  const handleChange = (input: string) => {
    setInputValue(input);

    if (inputValue?.length < 2) {
      handleProjectName(undefined);
      return;
    }

    const name = projectNames?.find((name) =>
      name?.toLowerCase().includes(inputValue.toLowerCase())
    );

    if (name) {
      handleProjectName(name);
    }
  };

  const handleProjectName = useCallback(
    (projectName?: string) =>
      handleFilters({ project: projectName?.toLowerCase() }),
    [handleFilters]
  );

  ...
Enter fullscreen mode Exit fullscreen mode

The Problem

The idea behind this code is very good and could lead to an improved user experience. But the implementation has some problems:

  • What if there are two projects that match the input? The code would take the first and the table would only show the data belonging to this one project. The second project’s data would be omitted.
  • The user doesn’t see that a particular project was selected. This would only add to the confusion.

A better implementation would show the different matches to the user similar to the Google search interface.

But building such a feature would be more complex of course.

Refactoring: Remove Project Input Autocomplete

As mentioned, the autocomplete is a good idea in general. But it’s better to remove this functionality altogether rather than leave the user confused. If this was an important feature I’d go back to the whiteboard and sketch out a proper solution that includes user feedback for better UX.

The simplest solution is updating the filters with every keystroke.

// ./components/filters.tsx

export function Filters() {
  const { handleFilters, filters } = useFilters();

  const handleChange = (project: string) => {
    handleFilters({ project });
  };

  ...
Enter fullscreen mode Exit fullscreen mode

This works but causes a re-render of the whole page whenever the user changes the input value. In our case, the performance implications of these re-renders aren’t that important though. The bigger problem is that every keystroke also triggers a new API request.

To reduce the number of requests we can debounce the updating of the filters. There’s a great small npm package called use-debounce that we can use to create a debounced version of the handleFilters function.

// ./components/filters.tsx

import { useDebouncedCallback } from "use-debounce";

...

export function Filters() {
  const { handleFilters, filters } = useFilters();

  const debouncedHandleFilters = useDebouncedCallback(handleFilters, 300);
  const [inputValue, setInputValue] = useState(filters.project || "");

  const handleChange = (project: string) => {
    setInputValue(project);
    debouncedHandleFilters({ project });
  };

  ...
Enter fullscreen mode Exit fullscreen mode

Now you can see that the project parameter in the URL only updates shortly after the user stopped typing.

You can find all code changes here on GitHub.

React on the job - email course

Code Review: Programmatic Styles

The last point of this code review is the styles. The application uses styled-components as a CSS solution. So we should be able to style almost anything with styled-components.

But interestingly the Filters component also uses conditional styles within the JSX:

// ./components/filters.tsx

export function Filters() {
  const { width } = useWindowSize();
  const isMobileScreen = width <= 1023;
  const { isMobileMenuOpen } = useContext(NavigationContext);

  ...

  return (
    <S.Container>
      <Select
        placeholder="Status"
        defaultValue="Status"
        width={isMobileScreen ? "97%" : "8rem"}
        style={{
          ...(isMobileMenuOpen && {
            opacity: 0,
          }),
        }}
      >
        ...
      </Select>

      <Select
        placeholder="Level"
        defaultValue="Level"
        width={isMobileScreen ? "97%" : "8rem"}
        style={{
          ...(isMobileMenuOpen && {
            opacity: 0,
          }),
        }}
      >
        ...
      </Select>

      <Input
        handleChange={handleChange}
        value={inputValue}
        label="project name"
        placeholder="Project Name"
        iconSrc="/icons/search-icon.svg"
        style={{
          ...(isMobileScreen && { width: "94%", marginRight: "3rem" }),
          ...(isMobileMenuOpen && {
            opacity: 0,
          }),
        }}
      />
    </S.Container>
  );
}
Enter fullscreen mode Exit fullscreen mode

The Problem

  1. The Select and Input components are styled depending on the window width. The magic number 1023 seems to be the desktop breakpoint. Using magic numbers in general is not recommended. But why don’t we use CSS media queries in the first place?
  2. The Select and Input components are basically set invisible if the navigation menu is open on mobile devices. This seems like a hacky solution to a problem that the author of this code encountered. So let’s see if we can find a proper alternative.

Refactoring: Fix Select Overlay Workaround

Next, let’s get rid of the programmatic styles. The first problem that we have to fix are the conditional styles when the mobile navigation menu is open. Let’s see what happens when we remove the following lines of code:

// ./components/filters.tsx

export function Filters() {
  ...

  return (
    <S.Container>
      <Select
        placeholder="Status"
        defaultValue={getStatusDefaultValue(filters)}
        width={isMobileScreen ? "97%" : "8rem"}
      >
        <Option value={undefined} handleCallback={handleStatus}>
          --None--
        </Option>
        <Option value="Unresolved" handleCallback={handleStatus}>
          Unresolved
        </Option>
        <Option value="Resolved" handleCallback={handleStatus}>
          Resolved
        </Option>
      </Select>

      <Select
        placeholder="Level"
        defaultValue={getLevelDefaultValue(filters)}
        width={isMobileScreen ? "97%" : "8rem"}
      >
        <Option value={undefined} handleCallback={handleLevel}>
          --None--
        </Option>
        <Option value="Error" handleCallback={handleLevel}>
          Error
        </Option>
        <Option value="Warning" handleCallback={handleLevel}>
          Warning
        </Option>
        <Option value="Info" handleCallback={handleLevel}>
          Info
        </Option>
      </Select>

      <Input
        handleChange={handleChange}
        value={inputValue}
        label="project name"
        placeholder="Project Name"
        iconSrc="/icons/search-icon.svg"
        style={{
          ...(isMobileScreen && { width: "94%", marginRight: "3rem" }),
        }}
      />
    </S.Container>
  );
}
Enter fullscreen mode Exit fullscreen mode

When we open the app in the responsive view we can see that the select and input elements overlay the mobile navigation menu.

Investigating the navigation menu via the Chrome dev tools shows that the only required change is setting a z-index to one of its parents.

Debugging CSS can be really difficult and annoying. But as you can see here finding the root cause of a styling issue helps avoid cumbersome workarounds.

You can find all changes here on GitHub.

Refactoring: Replace Programmatic Mobile Styles With CSS

As a final step in this refactoring let’s replace the remaining programmatic styles with simple CSS and media queries.

// ./components/filters.tsx

import * as S from "./filters.styled";

...

export function Filters() {
  const { handleFilters, filters } = useFilters();

  const debouncedHandleFilters = useDebouncedCallback(handleFilters, 300);
  const [inputValue, setInputValue] = useState(filters.project || "");

-  const { width } = useWindowSize();
-  const isMobileScreen = width <= 1023;

  ...

  return (
    <S.Container>
      <S.Select
        placeholder="Status"
        defaultValue={getStatusDefaultValue(filters)}
      >
        <Option value={undefined} handleCallback={handleStatus}>
          --None--
        </Option>
        <Option value="Unresolved" handleCallback={handleStatus}>
          Unresolved
        </Option>
        <Option value="Resolved" handleCallback={handleStatus}>
          Resolved
        </Option>
      </S.Select>

      <S.Select
        placeholder="Level"
        defaultValue={getLevelDefaultValue(filters)}
      >
        <Option value={undefined} handleCallback={handleLevel}>
          --None--
        </Option>
        <Option value="Error" handleCallback={handleLevel}>
          Error
        </Option>
        <Option value="Warning" handleCallback={handleLevel}>
          Warning
        </Option>
        <Option value="Info" handleCallback={handleLevel}>
          Info
        </Option>
      </S.Select>

      <S.Input
        handleChange={handleChange}
        value={inputValue}
        label="project name"
        placeholder="Project Name"
        iconSrc="/icons/search-icon.svg"
      />
    </S.Container>
  );
}
Enter fullscreen mode Exit fullscreen mode

This project uses styled-components so the new S.Select and S.Input components look like this.

// ./components/filters.styled.tsx

import styled from "styled-components";
import { breakpoint } from "@styles/theme";
import { Select as UnstyledSelect, Input as UnstyledInput } from "@features/ui";

...

export const Select = styled(UnstyledSelect)`
  width: 97%;
  @media (min-width: ${breakpoint("desktop")}) {
    width: 8rem;
  }
`;

export const Input = styled(UnstyledInput)`
  width: 94%;
  margin-right: 3rem;
  @media (min-width: ${breakpoint("desktop")}) {
    width: 17.5rem;
    margin-right: 0;
  }
`;
Enter fullscreen mode Exit fullscreen mode

This is where we hit a roadblock. The internal implementation of the Select component prevents the width defined in the CSS from being applied. Here you can see that its width is hard-coded internally:

You can find all changes here on GitHub.

Final Result

Below you can find the final code after this refactoring session. You can also see it here on GitHub.

We were able to remove a lot of complexity by using the URL as single source of truth for the filter values. This allowed us to get rid of a context. The resulting custom hook useFilters is very straightforward:

// ./hooks/use-filters.ts

import { useRouter } from "next/router";
import { IssueFilters } from "@api/issues.types";

export const useFilters = () => {
  const router = useRouter();

  const filters = {
    status: router.query.status,
    level: router.query.level,
    project: router.query.project,
  } as IssueFilters;

  const handleFilters = (newFilters: IssueFilters) => {
    const query = { ...router.query, ...newFilters };
    router.push({ query });
  };

  return { filters, handleFilters };
};
Enter fullscreen mode Exit fullscreen mode

The Filters component was also simplified especially by removing the useEffect.

// ./components/filters.tsx

import React, { useState } from "react";
import { useDebouncedCallback } from "use-debounce";
import { capitalize } from "lodash";
import { Option } from "@features/ui";
import { IssueFilters, IssueLevel, IssueStatus } from "@api/issues.types";
import { useFilters } from "../../hooks/use-filters";
import * as S from "./filters.styled";

function getStatusDefaultValue(filters: IssueFilters) {
  if (!filters.status) {
    return "Status";
  }
  if (filters.status === IssueStatus.open) {
    return "Unresolved";
  }
  return "Resolved";
}

function getLevelDefaultValue(filters: IssueFilters) {
  if (!filters.level) {
    return "Level";
  }
  return capitalize(filters.level);
}

export function Filters() {
  const { handleFilters, filters } = useFilters();

  const debouncedHandleFilters = useDebouncedCallback(handleFilters, 300);
  const [inputValue, setInputValue] = useState(filters.project || "");

  const handleChange = (project: string) => {
    setInputValue(project);
    debouncedHandleFilters({ project });
  };

  const handleLevel = (level?: string) => {
    if (level) {
      level = level.toLowerCase();
    }
    handleFilters({ level: level as IssueLevel });
  };

  const handleStatus = (status?: string) => {
    if (status === "Unresolved") {
      status = "open";
    }
    if (status) {
      status = status.toLowerCase();
    }
    handleFilters({ status: status as IssueStatus });
  };

  return (
    <S.Container>
      <S.Select
        placeholder="Status"
        defaultValue={getStatusDefaultValue(filters)}
        data-cy="filter-by-status"
      >
        <Option value={undefined} handleCallback={handleStatus}>
          --None--
        </Option>
        <Option value="Unresolved" handleCallback={handleStatus}>
          Unresolved
        </Option>
        <Option value="Resolved" handleCallback={handleStatus}>
          Resolved
        </Option>
      </S.Select>

      <S.Select
        placeholder="Level"
        defaultValue={getLevelDefaultValue(filters)}
        data-cy="filter-by-level"
      >
        <Option value={undefined} handleCallback={handleLevel}>
          --None--
        </Option>
        <Option value="Error" handleCallback={handleLevel}>
          Error
        </Option>
        <Option value="Warning" handleCallback={handleLevel}>
          Warning
        </Option>
        <Option value="Info" handleCallback={handleLevel}>
          Info
        </Option>
      </S.Select>

      <S.Input
        handleChange={handleChange}
        value={inputValue}
        label="project name"
        placeholder="Project Name"
        iconSrc="/icons/search-icon.svg"
        data-cy="filter-by-project"
      />
    </S.Container>
  );
}
Enter fullscreen mode Exit fullscreen mode

Unfortunately, we still need the functions getStatusDefaultValue and getLevelDefaultValue since the Select component can’t be controlled and doesn't handle the option value and display text separately yet. So a refactoring of the Select component would allow us to further simplify this code. But since this session already turned out quite long we’ll leave that for another time.

React on the job - email course

Top comments (1)

Collapse
 
atulranjandas28 profile image
Atul Ranjan Das •

Dose Code Refactoring really helps people to give a new structure to a program

Read next

goaqidev profile image

Optimizando la Integración de APIs de Blog: Lecciones Aprendidas con Dev.to y Hashnode

Joaquín Gutiérrez -

goaqidev profile image

JSDoc: La Guía Definitiva para Documentar tu Código JavaScript

Joaquín Gutiérrez -

maulikpaghdal profile image

Optimizing React Apps with useMemo and useCallback: A Complete Guide

Maulik Paghdal -

andrew_mclean_09 profile image

anyone wanna join me building my first app?(I'm a beginner bare with me)

Andrew Mclean -