#Is this spaghetti code? Question from a none developer

7 messages · Page 1 of 1 (latest)

snow ridge
#

Hi guys, im wondering if my code has to look something like this or is there a better coding guideline to have it more beautified and readable? as i am a none developer i dont have much relationship too developers and so im missing some deeper insights and knowledge about regularities. I dont want to overcomplicate stuff, if not needed so its just a small benchmark for me, to understand this.
Thanks:)

callbackRouter.get("/", async (req, res) => {
  const apiClient = new pipedrive.ApiClient();
  let oauth2 = apiClient.authentications.oauth2;
  oauth2.clientId = process.env.CLIENT_ID; // OAuth 2 Client ID
  oauth2.clientSecret = process.env.CLIENT_SECRET; // OAuth 2 Client Secret
  oauth2.redirectUri = process.env.REDIRECT_URI; // OAuth 2 Redirection endpoint or Callback Uri
  const authCode = req.query.code;
  const promise = apiClient.authorize(authCode);

  promise.then(async function () {
    req.session.accessToken = apiClient.authentications.oauth2.accessToken;
    if (
      req.session.accessToken !== null &&
      req.session.accessToken !== undefined
    ) {
      apiClient.authentications.oauth2.accessToken = req.session.accessToken;
      const usersAPI = new pipedrive.UsersApi(apiClient);
      const userME = await usersAPI.getCurrentUser(); //get Pipedrive User that installs the App
      const { name, company_id, company_domain } = userME.data; //deconstruct vars
      req.session.company_id = company_id; //set company id
      req.session.company_domain = company_domain;

      const user = await getUserFromDB(name, company_id); //check if user is already created
      if (user === null) {
        //if user has no account
        const newUser = await createUserInDB(userME.data); //create user in database with the use of all the data.
        res.redirect("/dashboard");
      }
    }
    (exception) => {
      // error occurred, exception will be of type src/exceptions/OAuthProviderException
    };
  });
});
module.exports = callbackRouter;
rough pendant
#

Ya this code seems a smelly to me, The api client seems to be more like a middleware where you should only make one connection to it and then export the api client from the file and import into this so I would remove this from the /get method

 let oauth2 = apiClient.authentications.oauth2;
 oauth2.clientId = process.env.CLIENT_ID; // OAuth 2 Client ID
 oauth2.clientSecret = process.env.CLIENT_SECRET; // OAuth 2 Client Secret
 oauth2.redirectUri = process.env.REDIRECT_URI; // OAuth 2 Redirection endpoint or Callback Uri

Another thing I don't like is the then can't you just wrap this all in a try/catch block and use await on all the async items it would help improve the readability. Also instead of doing req.session.accessToken !== null && req.session.accessToken !== undefined I would just do !req.session.accessToken it does the same thing and is easier to read personally

tropic chasm
#

First thing I recommend is use const always... unless you're really needing to edit that later (let count; count = count + 1 ).

The other thing is you can remove the apiClient/oauth2 out of the router.

The other thing is that overall you're doing 2 things here,

  1. do the authorization, and reject if that doesn't work.
  2. the pipedrive thing.

So what we should do is seperate that into 2 "bits" that flow in order.

# 
callbackRouter.get("/", async (req, res) => {
  const authCode = req.query.code;
  
  try {
    await apiClient.authorize(authCode);
  } catch {
    // Or whatever you want to do when it fails
    res.send(400)
    return
  }

  req.session.accessToken = apiClient.authentications.oauth2.accessToken;

  if(req.session?.accessToken == null) {
    // Or whatever you want to do when it fails
    res.send(400)
    return
  }
  // user information code 
}
#

And with this we can write this as a middleware since there's probably other things we want to protect this way!

# 
async function checkAuth(req, res, next) {
  const authCode = req.query.code;

  try {
    await apiClient.authorize(authCode)
  } catch {
    next("Failed Auth") // or whatever express 5 does now?
  }

  req.session.accessToken = apiClient.authentications.oauth2.accessToken;

  if(req.session?.accessToken == null) {
    next("Failed Auth")
  }

  next()
}

callbackRouter.get("/", checkAuth, async (req, res) => {
  // now with res.session.accessToken
snow ridge