0

What would be considered an acceptable way of dealing with returning a record from a DB with the following 3 potential outcomes:

  1. Db connection works, finds a user and returns a populated user object
  2. Db connection works, doesn't find a user, returns a new user object
  3. Db connection/query fails...

I'm for the most part aiming for design by contract:

class Scratch {
        public User getUser(int id) {
            try {
                // Prepare SQL Query
                PreparedStatement s = this.connection.prepareStatement(
                        "select * from get_user(?)"
                );

                // Provide SQL Parameters
                s.setInt(1, id);

                // Run our SQL
                ResultSet rs = s.executeQuery();
                rs.next();

                // Extract data into Entity
                User user = User.createFromDatabase(rs);
                rs.close();

                return user;

            } catch(Exception e) {
                e.printStackTrace();
            }

            return new User();
        }
}

In the situation that the DB connection or query fails it's a little less obvious what I should do, I have a few options:

  • Return a new user object, because our method has agreed to return a user
    • Pro: This sticks to designing by contracts
    • Con: This makes it look like the user doesn't exist.
  • Return null since it didn't actually get a user.
    • Pro: This is totally clear that a user was not found
    • Con: Requires a null check
  • Throw an exception further up the chain.
    • Pro: Makes it explicitly clear the operation wasn't achieved
    • Con: Does not attempt to rectify issues where they happen

I'm leaning towards handling Exceptions as it avoids the billion-dollar mistake, and also maintains design by contract in normal situations.

However I'd like to know if there are any obvious pitfalls or if this is a familiar situation with a well established pattern to resolve it.

Royal Wares
  • 1,192
  • 10
  • 23
  • `getUser` should only do one thing, which is to get a user. If it is not able to, it should throw an exception. To me, returning null gives the impression that the user doesn't exist. Related: https://stackoverflow.com/questions/77127/when-to-throw-an-exception – Ivar Oct 25 '18 at 08:32
  • 1
    @Ivar thanks I've updated that now, I opened up a new scratch file in IntelliJ and pasted in some code. – Royal Wares Oct 25 '18 at 08:34

2 Answers2

1

I would throw an exception and let the user know that the connection failed. Returning NULL is something I would never do because you won't know what the problem was.

I don't know why you would return "New user object" if you don't have connection to the database. You won't be able to save the user.

My choice will be to Throw an exception

Ivar
  • 6,138
  • 12
  • 49
  • 61
Pieter Samoy
  • 80
  • 1
  • 8
  • I think this is the most suitable solution, from the additional reading from ivar, it's looking unanimous. I have also now refactored my application and this solution just fits the best. – Royal Wares Oct 25 '18 at 08:40
0

Use Optional (please). This make explicit that the user could not be found in the database and avoid NullPointerException's.

  • Optional(User) if the user is found in the database
  • Empty if user is not found
  • If there is an Error you have two options:
    • Notify the client so it can get feedback and react to this failure. In this case throw an exception and handle it in the proper layer. This should be the default behaviour for most of the applications.
    • Hide it to the client, this is less common but some times the client doesn't care or you don't want to publish your failures. In that case just return an Empty.
RoberMP
  • 1,306
  • 11
  • 22
  • I disagree with your last point. I'm not sure what you mean with "client", but if that is the end user, then you can always catch your exception somewhere else in your application. Just continuing as if nothing happend is prone to error/data corruption. – Ivar Oct 25 '18 at 08:41
  • @Ivar that's exactly the reason it wouldn't work in my use case, I'm batch processing users, so pretending everything is okay would cause the batch updater to further process the record instead of halting & retrying the current batch. – Royal Wares Oct 25 '18 at 08:47
  • 1
    I guess that I deserve the negative point, don't know what I was thinking -_-!. Btw, I really encourage you to use Optional to avoid return null's on empty results request. – RoberMP Oct 25 '18 at 11:53