How can I improve error handling on a pool

Question

I am have created a class User that will hold the logic for inserting a new user into a postresql database. My code works perfectly but i think it is poorly written and would like some views on how to improve it, especially error handling.

const pool = require('../config/config.js');
// user constructor
class User {
  constructor(user) {
    this.username = user.username;
    this.email = user.email;
    this.password = user.password;
    this.role = user.role;
  }

  // save new user in databes
  createUser(res) {
    pool.connect((err, client, done) => {
      done();
      if (err) return res.status(400).json({ err });
      client.query('INSERT INTO users (username, email, password, role) VALUES($1, $2, $3, $4)', [this.username, this.email, this.password, this.role], (error) => {
        if (error) return res.json({ error });
        return res.json({ message: 'created successfully' });
      });
    });
  }
}

module.exports = User;

app.post('/', (req, res) => {
  const user = new User({
    username: 'Femoz',
    password: '1234',
    role: 'admin',
    email: '[email protected]',
  });
  user.createUser(res);
  // res.json('user created successfully');
});

That done() call looks wrong.

Bergi
– Bergi

2019-11-13 21:01:55 +00:00
Commented Nov 13, 2019 at 21:01 — Bergi
– Bergi, Commented Nov 13, 2019 at 21:01

Aikon Mogwai · Accepted Answer · 2019-11-13 17:27:55Z

0

const pool = require('../config/config.js');

class User {
  constructor(user) {
    this.username = user.username;
    this.email = user.email;
    this.password = user.password;
    this.role = user.role;
  }

  // save new user in databes
  save(cb) {
    const user = this;

    // Perhaps, you should to do some checks of props e.g. a name is not empty
    if (!user.name)
      return cb(Error('Empty name'));

    // I think that you should implement some wrapper 
    // e.g. `db.run` to avoid call the pool directly each time. 
    pool.connect((err, client, done) => {
      // done(); here is an error. You released the db-connection too early.
      if (err) 
         return cb(err);

      // I assumed that the result of done() is undefined so cb will be called.
      // Overwise use `&&` instead `||`.
      client.query(
        'INSERT INTO users (username, email, password, role) VALUES($1, $2, $3, $4) RETURNING id', 
        [user.username, user.email, user.password, user.role], 
        (err, res) => done() || cb(err, id: res && res.rows[0].id)
      );
    });
  }
}

module.exports = User;

app.post('/', (req, res, next) => {
  const user = new User({
    username: 'Femoz',
    password: '1234',
    role: 'admin',
    email: '[email protected]',
  });

  // Return new id is better than a static text :)
  user.save((err, id) => err ? res.status(400).json({error: err.message}) : res.json({id}));

  // OR

  // For Express you can pass error to an error handler
  user.save((err, id) => err ? next(err) : res.json({id}));
});

edited Nov 13, 2019 at 17:27

answered Nov 13, 2019 at 17:20

Aikon Mogwai

5,2732 gold badges23 silver badges36 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Bergi Over a year ago

"I assumed that the result of done() is undefined" - better don't guess, and just write (err, res) => { done(); cb(err, id: res && res.rows[0].id); }

Aikon Mogwai Over a year ago

Yes, I agree. It's better :)

Collectives™ on Stack Overflow

How can I improve error handling on a pool

1 Answer 1

2 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

2 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related