6
\$\begingroup\$

I'm working on a spigot plugin, that deals with messages to from players on a server to the admin's. It's the same plugin this question relates to: SQL structure to save coordinates for spigot plugin . I've got a command, which allowes access to the inbox, and does all kinds of operations on the data. One of these operations is deleting an entry by it's ID.

I've got a simple SQL query to delete it, and want to display a message, depending on if the message could be deleted or not. Therefore I wrote this method in my Command-Class:

private void deleteMessage(CommandSender sender, int messageId) {
    int deleted = DatabaseMethods.deleteMessage(messageId);
        
    if (deleted == 0) {
        sender.sendMessage(MessageManager.couldNotBeFound(messageId));
    } else {
        sender.sendMessage(MessageManager.deletedMessage(messageId));
    }
}

and this is the method that accesses the database:

public static int deleteMessage(int messageId) {
    try (final PreparedStatement ps = BasePluginAPI.getDatabaseConnection()
            .prepareStatement("delete from MELDUNG where ID = ?")) {
        ps.setInt(1, messageId);
        return ps.executeUpdate();
    } catch (SQLException e) {
        e.printStackTrace();
        return 0;
    }
}

I don't know if this is the best way to approach it. I have two main questions:

  1. Is an early return better for the first method? Or is it just a question of personal taste?
  2. Can the SQL-handling be improved? Should maybe the method return a boolean instead of an int?

Besides these specific questions, of course general feedback is very welcome as well! :)

\$\endgroup\$
5
  • \$\begingroup\$ What do you mean with an earlier return? You can't return earlier there as far as I can see. \$\endgroup\$
    – Bobby
    Oct 28 '20 at 17:06
  • \$\begingroup\$ Can the delete affect multiple messages? If yes, int, if not, boolean. \$\endgroup\$
    – Bobby
    Oct 28 '20 at 17:07
  • \$\begingroup\$ Instead of using an else clause, just insert a return; after sending the couldNotBeFound-Message, and putting the second message after the if \$\endgroup\$
    – monamona
    Oct 28 '20 at 17:08
  • \$\begingroup\$ @Bobby No it cannot. ID, or messageId is the primary key of my database table \$\endgroup\$
    – monamona
    Oct 28 '20 at 17:09
  • 1
    \$\begingroup\$ Personal taste, I'm afraid. I like if/else constructs for these situations, as they provide immediate meaning how the two statements depend on then condition. Performance-wise it doesn't matter at all. \$\endgroup\$
    – Bobby
    Oct 28 '20 at 17:10
3
\$\begingroup\$

Regarding your questions:

  1. Is an early return better for the first method? Or is it just a question of personal taste?

The alternative you suggest would be:

private void deleteMessage(CommandSender sender, int messageId) {
    int deleted = DatabaseMethods.deleteMessage(messageId);
    if (deleted == 0) {
        sender.sendMessage(MessageManager.couldNotBeFound(messageId));
        return;
    } 
    sender.sendMessage(MessageManager.deletedMessage(messageId));
}

I generally don't recommend it. A developer might delete the empty return thinking it's unnecessary causing a bug. The if-else is more readable in my opinion.

  1. Can the SQL-handling be improved? Should maybe the method return a boolean instead of an int?

If the delete operation can return only 0 or 1, then it should be a boolean. However, there are more than two outcomes: the database is down, the database call times out, etc. "Message not found" is just one of the reasons for deleteMessage to return 0. So instead of MessageManager.couldNotBeFound(messageId)), would be more appropriate to say "message cannot be deleted".

To tell the user "message not found", you can let the second method throw the exception and catch it in the first method.

One more thing, from the method signature is not clear why deleteMessage(CommandSender sender, int messageId) needs a CommandSender, a better name can be deleteMessageAndNotify.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.