Be Careful when using Design Patterns
For over 7 years, I've immersed myself in software development, and consulting for the past 2 years. In my diverse career, I've noticed a prevalent issue: the haphazard use of design patterns. Some developers apply these patterns without a clear purpose.
Proficient developers strategically use design patterns when they meet a genuine need. While valuable, incorporating them without technical justification for enhancement is a detrimental practice.
Motivated, I've written this article emphasizing the importance of using tools, following rules, and adopting practices as they naturally become necessities. Understand patterns through simple and advanced examples. In real projects, exercise caution - let the need for a design pattern emerge naturally. Their inherent wisdom ensures purposeful utilization.
Forget About Rules
I had a project colleague (Wojtek); though not currently working together, I value his programming insights. His concise code snippets are my go-to when facing challenges or seeking ways to enhance my work. Despite our separation, his programming wisdom still guides and inspires me.
He often stressed simplicity with phrases like "It must just work, forget about everything else." Another gem: "If it's working, write tests unrelated to implementation; improvements will follow." And who could forget his advice on design patterns? "Apply them only if they're calling you, like your wife when she's ultra mad."
It's truly transformative, realizing its impact on your mindset. The awareness of life's limitations underscores the importance of efficiency. Crafting a magnificent codebase isn't always feasible; this reality reshapes one's approach to balance functionality and aesthetics within the constraints of time and priorities.
Example from Project
Life continues, and now I can share an example from a while back without any legal concerns. Likely, this small code snippet has either been removed, refactored, or undergone various transformations – as is often the fate of code in the dynamic world of development! 😄
I had a ticket to add a mechanism checking for user idle state – meaning no activity for a specific period, like 1 hour. After this time, we trigger a response in our application.
It seems straightforward, and I created a hook for this purpose, employing it in multiple areas of the application. Surprisingly, the hook implementation turned out quite intricate for such a simple feature. Yes, at that time, I was a fanatic about "good practices/standards/design patterns" 💢.
import { useRef, useEffect, useState } from "react";
// Define types for the different states
interface Interacting {
is: "interacting";
}
interface Idle {
is: "idle";
}
// Create a union type for the states
type State = Interacting | Idle;
// Configuration interface for the hook
interface Config {
delay?: number;
}
// Default state when the hook is initialized
const defaultState = { is: "interacting" } as State;
// Custom React hook for handling user inactivity
// Default delay is set to 10 seconds (10000 milliseconds)
export const useOnNoInteraction = ({ delay }: Config = { delay: 10000 }) => {
// State to track the current interaction state
const [state, setState] = useState(defaultState);
// Ref for storing the timeout ID
const timeout = useRef<NodeJS.Timeout | null>(null);
// Function to set the state to "idle"
const stop = (): void => {
setState({ is: "idle" });
};
// Function to start the timer for inactivity
const start = (): void => {
timeout.current = setTimeout(stop, delay);
};
// Effect hook to start the timer when the component mounts
useEffect(() => {
start();
// Cleanup: clear the timeout when the component unmounts
return () => {
timeout.current && clearTimeout(timeout.current);
};
}, []);
// Return the state and functions for starting/stopping the timer
return [state, { start, stop }] as const;
};Now, how we intended to use this hook:
// Example usage in a component
const Component = () => {
// Destructure the state and functions from the hook
const [state, { start, stop }] = useOnNoInteraction();
// Effect hook to log out the user when in an idle state
useEffect(() => {
if (state.is === "idle") {
// Log Out user!
console.log("User logged out due to inactivity.");
}
}, [state]);
// We can manually start/stop the interaction timer
start();
stop();
};Before diving into self-loathing with my colleague Wojtek, let's examine some patterns we employed. Firstly, we utilized a state machine to determine the state, with an unnecessary option to assign metadata (let's call it overengineering since it will never be needed). Additionally, we implemented the exhaustiveness checking technique from TypeScript to prevent access to disallowed properties – also entirely unnecessary in this scenario.
Additionally, I overlooked the absence of testing for this code section. As a result, there's no regression check if we make future changes to the provided example.
Identifying Issues and Overengineering
Time for a Code Review with Wojtek. Initially, he rightfully highlighted the absence of tests in the code.
Wojtek: No tests for the provided code.
So, I've added the following tests:
import { renderHook, act } from '@testing-library/react-hooks';
import { useOnNoInteraction } from './useOnNoInteraction';
describe('No interaction detection works when', () => {
beforeEach(() => {
jest.useFakeTimers();
});
afterEach(() => {
jest.useRealTimers();
});
it('initializes with default state', () => {
const { result } = renderHook(() => useOnNoInteraction());
const [state] = result.current;
expect(state.is).toBe('interacting');
});
it('transitions to "idle" state after the specified delay', () => {
const { result } = renderHook(() => useOnNoInteraction({ delay: 5000 }));
const [, { stop }] = result.current;
act(() => {
jest.advanceTimersByTime(5000);
});
const [state] = result.current;
expect(state.is).toBe('idle');
// Cleanup
stop();
});
it('does not transition to "idle" if manually stopped before the delay', () => {
const { result } = renderHook(() => useOnNoInteraction({ delay: 5000 }));
const [, { stop }] = result.current;
act(() => {
stop();
jest.advanceTimersByTime(5000);
});
const [state] = result.current;
expect(state.is).toBe('interacting');
});
it('restarts the timer when manually started', () => {
const { result } = renderHook(() => useOnNoInteraction({ delay: 5000 }));
const [, { start, stop }] = result.current;
act(() => {
jest.advanceTimersByTime(3000);
start();
jest.advanceTimersByTime(3000);
});
const [state] = result.current;
expect(state.is).toBe('interacting');
// Cleanup
stop();
});
it('logs out the user when in "idle" state', () => {
const mockLogout = jest.fn();
jest.spyOn(console, 'log').mockImplementation(() => {}); // Suppress console.log output
const { result } = renderHook(() => useOnNoInteraction({ delay: 5000 }));
const [, { start, stop }] = result.current;
act(() => {
jest.advanceTimersByTime(5000);
});
const [state] = result.current;
expect(state.is).toBe('idle');
// Mocking useEffect
jest.spyOn(React, 'useEffect').mockImplementationOnce((effect) => effect());
// Using the hook in a component
const { rerender } = renderHook(() => useOnNoInteraction({ delay: 5000 }));
rerender();
act(() => {
jest.advanceTimersByTime(5000);
});
expect(mockLogout).toHaveBeenCalled();
// Cleanup
stop();
});
});The tests I added exposed implementation details. Moreover, Wojtek recognized that the hook was overly complex. He invested time and presented me with a simpler version:
import { useRef, useEffect, useCallback } from "react";
export const useTimeout = (delay: number, callback: () => void) => {
const timeout = useRef<NodeJS.Timeout | null>(null);
useEffect(() => {
timeout.current = setTimeout(callback, delay);
// Cleanup: clear the timeout when the component unmounts
return () => {
timeout.current && clearTimeout(timeout.current);
};
}, [callback]);
};
// Example usage in a component
const Component = () => {
// The usage of "useCallback" is important here!
const logoutUser = useCallback(() => {
// Do something here...
}, []);
useTimeout(logoutUser);
};It's considerably simpler, and the hook's name implies its potential for broader use cases beyond just detecting a lack of interaction. He also proposed some test enhancements that assessed it comprehensively while concealing implementation details that I had inadvertently exposed.
import React from 'react';
import { render, act } from '@testing-library/react';
import { useTimeout } from './your-hook-file'; // Replace with the actual path to your hook file
// A simple component that uses the useTimeout hook
const TimeoutComponent = ({ callback, delay }: { callback: () => void, delay: number }) => {
useTimeout(delay, callback);
return <div>Timeout Component</div>;
};
describe('Timeout works when', () => {
jest.useFakeTimers();
it('action is performed only once', () => {
const callback = jest.fn();
const delay = 5000;
act(() => {
const { unmount } = render(<TimeoutComponent callback={callback} delay={delay} />);
jest.advanceTimersByTime(delay);
// Check if the callback is called after the specified delay
expect(callback).toHaveBeenCalled();
// Clear the timeout when the component unmounts
unmount();
jest.advanceTimersByTime(delay);
});
// Check if the callback is not called after the component unmounts
expect(callback).not.toHaveBeenCalled();
});
});At the end of this refactoring process, here's what he said:
Wojtek: Generally, I like your implementation, but I think for this use case, you can make the implementation/tests much simpler. Remember, it needs to work and don't make it too complicated. With my proposal, I provided you with an easier implementation, a broader scope of use, and simpler tests to maintain while hiding some implementation details. Of course, we can't hide all of them –
setTimeoutneeds to be called, so we have to use theuseFakeTimers()API.
Conclusion
This code is much better. This is what you should be focused on. First, aim for a working implementation, then write tests, attempting not to leak implementation details - though sometimes it's unavoidable, as you saw. Afterward, consider improving the implementation if time allows. Avoid overcomplicating by default.
Wojtek, in his refactor/reimplementation process, mostly used a callback to achieve what was needed.
I must say it loudly - "Design Patterns Should Be Used Wisely". If used without reason, they only overcomplicate your codebase. In this particular example, I employed the State Machine pattern, but it's more suitable for advanced state management use cases, such as handling API request reactions or similar scenarios.
If you're curious about patterns, here are several articles to explore:
https://4markdown.com/proxy-pattern-in-typescript/
https://4markdown.com/understanding-repository-pattern-in-nodejs-and-typescript/