SMS relay --> MySQL database

Question

The main purpose of my program is to extract GPS information from text messages sent to several GSM modems send from various GPS tracking devices. The main flow of my program loops in three steps:

• Receive SMS message (with the help of SMSlib)
• Extract data with regex
• Push data to MySQL database

I've only been exposed to Java for half a year and I would be honored to receive some feedback on my code - it is working at the moment. Should I split my code in seperate classes - nested class? Are there any smarter/more elegant ways of doing the things I do? Any general feedback on my code and my style?

(My massive abuse of print statements is only for debugging purposes doing development.)

Main "driver" class (some kind of GUI in the future):

public class GUI {
 public static void main(String[] args) {
 SMSRelay relay = new SMSRelay();
 try {
 System.out.println("Configuring gateway...");
 relay.addGateway("modem1", "COM4", "3716");
 System.out.println("Starting service...");
 relay.start();
 System.out.println("Listening... - Hit <enter> to stop service.");
 System.in.read();
 } catch (Exception e) {
 System.out.println("Something went wrong ...");
 } finally {
 try {
 relay.stop();
 System.out.println("SMSRelay stopped");
 } catch (Exception e) {
 System.out.println("Failed to stop the service...");
 }
 }
 }
}

SMSRelay class:

public class SMSRelay {
 private Service instance;
 public SMSRelay() {
 instance = Service.getInstance();
 instance.setInboundMessageNotification(new InboundHandling()); 
 }
 public void addGateway(String name, String port, String pin) throws GatewayException {
 SerialModemGateway g = new SerialModemGateway(name, port, 115200, "Huawei", "E220");
 g.setProtocol(Protocols.PDU);
 g.setInbound(true);
 g.setOutbound(true);
 g.setSimPin(pin);
 g.getATHandler().setStorageLocations("SMME"); //To prevent duplicate texts
 instance.addGateway(g);
 }
 public void removeGateway(String name) throws GatewayException {
 instance.removeGateway(instance.getGateway(name));
 }
 public void start() throws Exception {
 instance.startService();
 }
 public void stop() throws Exception {
 instance.stopService();
 }
 public class InboundHandling implements IInboundMessageNotification {
 private Pattern p = Pattern.compile("lat: (?<lat>\\d+\\.\\d+) long: (?<lng>\\d+\\.\\d+).*imei:(?<imei>\\d+)");
 public void process(AGateway gateway, MessageTypes msgType, InboundMessage msg) {
 java.text.SimpleDateFormat formatter = new java.text.SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
 String date = formatter.format(msg.getDate());
 String originator = msg.getOriginator();
 String text = msg.getText();
 //Delete message from SIM
 try {
 instance.deleteMessage(msg); 
 } catch (Exception e) {
 System.out.println("Failed to delete message...");
 }
 //Look for GPS data 
 Matcher m = p.matcher(text);
 if (m.matches()) {
 System.out.println("Parsing valid info to database...");
 toDatabase(m.group("lat"), m.group("lng"), m.group("imei"), date, originator);
 } else {
 System.out.println("Invalid SMS");
 System.out.println(text);
 }
 }
 private void toDatabase(String lat, String lng, String imei, String date, String originator) { 
 Connection con = null;
 PreparedStatement pst = null;
 try {
 //Get MySQL info from properties file
 Properties props = new Properties();
 props.load(new FileInputStream("properties/database.properties"));
 String url = props.getProperty("db.url");
 String user = props.getProperty("db.user");
 String password = props.getProperty("db.passwd");
 System.out.println("Connecting to database...");
 //Connect to database 
 con = DriverManager.getConnection(url,user,password);
 System.out.println("Writing...");
 //Write data
 pst = con.prepareStatement("INSERT INTO positions VALUES (default, ?, ?, ?, ?, ?)");
 pst.setString(1, date);
 pst.setString(2, lat);
 pst.setString(3, lng);
 pst.setString(4, originator);
 pst.setString(5, imei);
 pst.executeUpdate();
 System.out.println("Data has been written!");
 } catch (IOException e) {
 System.out.println("Can't access database.properties!");
 } catch (SQLException e) {
 System.out.println("Something went wrong while interacting with database...");
 while (e != null) {
 System.out.println ("SQLState: " + e.getSQLState () + "");
 System.out.println ("Message: " + e.getMessage() + "");
 System.out.println ("Vendor ErrorCode: " + e.getErrorCode() + "");
 e = e.getNextException();
 System.out.println("");
 }
 } finally {
 try {
 if (pst != null) pst.close();
 if (con != null) con.close(); 
 } catch (SQLException e) {
 System.out.println("Something went wrong closing the connections...");
 } 
 } 
 }
 }
}

Answer 1

It's good to make fields final when possible, for example:

private final Service instance;

and also in the InboundHandling class:

private final Pattern p = ...

---

Avoid declaring methods with throws Exception and catching exceptions with catch (Exception s). Throw the most specific kind of exception possible, and do likewise when catching. For one thing, it tells readers of the code a lot about what can go wrong inside the method, or block of code, which is very useful. But even more importantly, it avoids masking exceptions that are actually unexpected and could be the result of bugs or oversight.

---

The nested InboundHandling class should be static, as it doesn't need anything from the enclosing class. This improves performance, and clarity of the design.

• user63646: Thank you very much for your words! Would you specify all the different exception a method can throw? E.g. my start() method (because of SMSlib) is able to throw five different exceptions (TimeoutException, GatewayException, SMSLibException, IOException, InterruptedException) - would you specify all of them explicitly after the throws clause as I don't want to handle the exception in the method?
• janos: A method that can throw so many different kinds of exceptions is a sign of bad design. Does the method follow the single responsibility principle? It seems unlikely.
• user63646: I actually don't know - it's the startService() method from SMSlib that throws the exceptions.
• janos: You should try to catch each and handle gracefully if possible. If not possible, then you could rethrow all of them and let the caller handle. If that doesn't really make sense, then you could catch and throw a custom exception that wraps the original exception. I think any of these options will be better than throwing a generic Exception. Or maybe yours is a truly exception case where throwing Exception is justified. I suggest to at least try other options, because throwing and catching Exception is a known bad practice.