CodeNewbie Community

loading...

How do I improve this controller function?

Tim Rohrer
Returning to my roots
・2 min read

I have a Controller class for which my delete function is out of control. I've come to this conclusion because my tests are complicated, the function is greater than 30 lines, I have nested if/then statements, and multiple things are happening inside the function.

Deciding to break up the delete function, I am unclear on the best approach. Oh, and unless I'm confused, I want to transition the code from imperative to more declarative.

Here is the method as it stands today:

  static async delete(req: express.Request, res: express.Response) {
    try {

      // extract and validate apiAccess token
      const { apiAccessToken } = req.body;
      if (!apiAccessToken || typeof apiAccessToken !== 'string') {
        res.status(400).json({ error: 'Bad password format, expected string.' });
        return;
      }

      // extract jwt from headers
      const userJwt = req.get('Authorization').slice('Bearer '.length);

      // generate a Registrant object representing the claimed user
      const userClaimResult = Registrant.decoded(userJwt);
      if (userClaimResult.err) {
        const { message } = userClaimResult.val;
        res.status(401).json({ message });
        return;
      }
      const userClaim = userClaimResult.val;

      // attempt to get registrant record from database, and validate
      const getRegistrantResult = await RegistrantsDAO.getRegistrant(userClaim.email);
      if (getRegistrantResult.err) {
        const { message, httpCode } = getRegistrantResult.val;
        res.status(httpCode).json({ message });
      } else {
        const registrant = new Registrant(getRegistrantResult.val);
        const validateUserClaimResult = await registrant.compareAPIAccessToken(apiAccessToken);
        if (validateUserClaimResult.err) {
          const { message, httpCode } = validateUserClaimResult.val;
          res.status(httpCode).json({ message });
        } else {
          const deleteResult = await RegistrantsDAO.deleteRegistrant(userClaim.email);
          if (deleteResult.err) {
            const { message, httpCode } = deleteResult.val;
            res.status(httpCode).json({ message })
          } else res.json(deleteResult.val);
        }
      }
    } catch (e) {
      res.status(500).json(e);
    }
  }
}
Enter fullscreen mode Exit fullscreen mode

The method performs four tasks as I see it:

  1. Extract and validate the API access token. This code may be handled by the express-validator lib, and so I'm not too worried about it right now.
  2. Extract JWT and create a Registrant object as the user claim. The Registrant object (returned from decoded) representing the claim is what I want for the next stage...
  3. Authenticate the Registrant (i.e., user). This involves use of the dependent DAO to retrieve and validate the user's claim. Or, it could be argued, the authenticated user is what I really want, right? Or is this more accurately referred to as a validated user claim?
  4. Finally, delete the user.

Testing this function requires mocking (inside isolateModules because my DAO is a singleton) the Registrant constructor and two of its functions, the RegistrantDAO and req/res. Ideally, breaking this up should simplify the test.

It does seem there are a couple of approaches I could consider:

  1. Write a separate private function that provides the user claim (or error) back to this delete function, followed by a separate private function to validate the claim against the database.
  2. Write a single private function that is passed the extracted JWT which then returns true/false if the Registrant and claim are both valid.

I'm leaning towards the second option, but suspect this just shifts the difficult testing to a different code block since both the Registrant and RegistrantDAO classes would be used in this new method. If I split generateUserClaim from validateUser, then it might simplify testing a little bit.

Am I on track or should I be considering a different approach?

Discussion (1)

Collapse
timrohrer profile image
Tim Rohrer Author

I received a tip on Discord to explore custom middleware as a way to break this function up. That is the approach I’m now researching.