StoredErrorWriter

Question

Functionality that overwrite standard error message with trace to user friendly message with error number stored in DB.

Not yet completed some stuff with trx...

Please review this.

DB table:

CREATE TABLE stored_error_msg
 (
id number PRIMARY KEY 
,msg VARCHAR2(2000)
,trace CLOB
,browser VARCHAR2(2000)
,date date 
,user VARCHAR2(100)
,ses_id number
 );

@ UI

this.getCurrent().setErrorHandler(new StoredErrorWriter());

StoredErrorWriter

package components;

import java.io.PrintWriter;
import java.io.StringWriter;

import com.vaadin.server.DefaultErrorHandler;
import com.vaadin.server.ErrorEvent;
import com.vaadin.server.ErrorHandler;
import com.vaadin.server.ErrorMessage;
import com.vaadin.server.UserError;
import com.vaadin.server.WebBrowser;
import com.vaadin.ui.AbstractComponent;
import com.vaadin.ui.UI;

import java.util.logging.Level;
import java.util.logging.Logger;

/*
 * @author: Kristaps Zukovskis
 * Write info about: user, date, err_msg, backtrace, browser info, ses_id   
 * @ DB table stored_error_msg 
 */
public class StoredErrorWriter implements ErrorHandler {

    private static final long serialVersionUID = 5117130906083582416L;
    public static String ERROR_MSG = "System error! Error number in journal :";
    private Logger logger = Logger.getAnonymousLogger();

    @Override
    public void error(ErrorEvent event) {

        Throwable t = event.getThrowable();
        if (t != null) {

            String trace = getTrace(t);
            final String msg = t.getMessage();

            AbstractComponent component = DefaultErrorHandler
                    .findAbstractComponent(event);

            if (component != null) {

                final Integer errorNumber = InsertTrace(msg,trace);
                ErrorMessage errorMessage = new UserError(ERROR_MSG
                        + errorNumber);

                if (errorMessage != null) {
                    component.setComponentError(errorMessage);
                    logger.log(Level.SEVERE, trace);
                    return;
                }
            }
        }

        DefaultErrorHandler.doDefault(event);

    }

    private static String getTrace(Throwable t) {

        StringWriter sw = new StringWriter();
        PrintWriter pw = new PrintWriter(sw);
        t.printStackTrace(pw);

        return sw.toString();
    }

    public static String getInfo() {

        @SuppressWarnings("deprecation")
        WebBrowser browser = UI.getCurrent().getSession().getBrowser();
        String browserName = "unknown";

        if (browser.isOpera())
            browserName = "Opera";
        else if (browser.isSafari())
            browserName = "Apple Safari";
        else if (browser.isChrome())
            browserName = "Google Chrome";
        else if (browser.isIE())
            browserName = "Internet Explorer";
        else if (browser.isFirefox())
            browserName = "Mozilla Firefox";

        String screenSize = " Screen Size: " + browser.getScreenWidth() + "x"
                + browser.getScreenHeight();
        String version = String.valueOf(browser.getBrowserMajorVersion());

        return browserName + " " + version + screenSize;
    }

    public static Integer InsertTrace(String msg, String trace) {

        final String browser = getInfo();
        final String user = null;// TODO

        Integer id = getSeq();

        java.util.Date utilDate = new java.util.Date();
        java.sql.Date date = new java.sql.Date(utilDate.getTime());

        // TODO ses_id 
        // TODO trx
        System.out.println("==id == " + id + " msg " + msg + " trace "+ trace +" browser"
                + browser + " Date " + date);

        return id;
    }

    public static Integer getSeq() {

        Integer id =1;
        String v_sql = "SELECT err_seq.NEXTVAL FROM DUAL";
        // TODO trx

        return id;
    }
}

---

// Edit

package components;

import java.io.PrintWriter;
import java.io.StringWriter;

import com.vaadin.server.DefaultErrorHandler;
import com.vaadin.server.ErrorEvent;
import com.vaadin.server.ErrorHandler;
import com.vaadin.server.ErrorMessage;
import com.vaadin.server.UserError;
import com.vaadin.server.WebBrowser;
import com.vaadin.ui.AbstractComponent;
import com.vaadin.ui.UI;

import java.sql.SQLException;
import java.util.logging.Logger;

/*
 * @author: Kristaps Zukovskis
 * Write info about: user, date, err_msg, backtrace, browser info, ses_id   
 * @ DB table stored_error_msg 
 */

public class StoredErrorWriter implements ErrorHandler {

private static final long serialVersionUID = 5117130906083582416L;
public static String ERROR_MSG = "System error! Error number in journal :";
private Logger logger = Logger.getAnonymousLogger();

@Override
public void error(ErrorEvent event) {
    Throwable throwable = event.getThrowable();
    if (throwable == null) {
        DefaultErrorHandler.doDefault(event);
        return;
    }
    if (StoredErrorWriter.getException(throwable)) {
        DefaultErrorHandler.doDefault(event);
        return;
    }
    AbstractComponent component = DefaultErrorHandler
            .findAbstractComponent(event);
    if (component == null) {
        DefaultErrorHandler.doDefault(event);
        return;
    }
    final String trace = StoredErrorWriter.getTrace(throwable);
    final String msg = throwable.getMessage();
    final Integer errorNumber = insertTrace(msg, trace);
    ErrorMessage errorMessage = new UserError(ERROR_MSG + errorNumber);
    component.setComponentError(errorMessage);
    logger.severe(trace);
}

private static String getTrace(Throwable t) {
    StringWriter sw = new StringWriter();
    PrintWriter pw = new PrintWriter(sw);
    t.printStackTrace(pw);
    return sw.toString();
}

public static String getInfo() {
    @SuppressWarnings("deprecation")
    WebBrowser browser = UI.getCurrent().getSession().getBrowser();
    String browserName = "unknown";

    if (browser.isOpera())
        browserName = "Opera";
    else if (browser.isSafari())
        browserName = "Apple Safari";
    else if (browser.isChrome())
        browserName = "Google Chrome";
    else if (browser.isIE())
        browserName = "Internet Explorer";
    else if (browser.isFirefox())
        browserName = "Mozilla Firefox";

    String screenSize = " Screen Size: " + browser.getScreenWidth() + "x"
            + browser.getScreenHeight();
    String version = String.valueOf(browser.getBrowserMajorVersion());

    return browserName + " " + version + screenSize;
}

public static Integer insertTrace(String msg, String trace) {
    final String browser = getInfo();
    final String user = null;// TODO
    final Integer id = getSeq();

    java.util.Date utilDate = new java.util.Date();
    java.sql.Date date = new java.sql.Date(utilDate.getTime());

    // TODO ses_id
    // TODO trx
    System.out.println("==id == " + id + " msg " + msg + " trace " + trace
            + " browser" + browser + " Date " + date);

    return id;
}

public static Integer getSeq() {
    final Integer id = 1;
    String v_sql = "SELECT SETTINGS.err_seq.NEXTVAL FROM DUAL";
    // TODO trx
    return id;
}

private static Boolean getException(Throwable t) {
    Boolean error = true;
    IllegalArgumentException exception = getCauseOfType(t,
            IllegalArgumentException.class);
    if (exception != null) {
        error = false;
    }
    SQLException sqlException = getCauseOfType(t, SQLException.class);
    if (sqlException != null) {
        error = false;
    }
    //...
    return error;
}

@SuppressWarnings("unchecked")
private static <T extends Throwable> T getCauseOfType(Throwable th,
        Class<T> type) {
    while (th != null) {
        if (type.isAssignableFrom(th.getClass())) {
            return (T) th;
        } else {
            th = th.getCause();
        }
    }
    return null;
}
}

Answer 1

Proprietary indentation convention
For some reason the first line in every method in your code is blank. This might be consistent, but since I haven't seen it anywhere else, I find it breaks the read flow, and makes your code less readable.

Use of deprecated code
Deprecation warnings are there for a reason - what they actually say is that "this may not work in next versions of this library - be warned, change this as soon as you can".
Deprecation warning also generally come with an alternative API you should use, in the case of vaadin - it suggests you use Page.getWebBrowser() instead.

Naming conventions
Generally, your method names are OK, but, for some reason, you've decided to uppercase the first letter in InsertTrace... it should start with lowercase, like the other methods.

Use static methods explicitly
To make sure your reader understand where your code lies, it is preferable that you call static methods in a fully qualified way - StoredWriterError.getTrace(t). This way it is obvious that the method is static, and it not part of the current instance.

Missing code
Since your code is missing all database-related parts, it is hard to tell whether this design is sound or not... perhaps you would like to revisit us after you complete that part.

Answer 2

1. I'd consider storing raw values of screen size, browser name, major and minor versions in the database as separate attributes too. It might help creating statistics later.

2. Instead of an anonymous logger:

   private Logger logger = Logger.getAnonymousLogger();
   

   it would be easier for operations to use a named one:

   private static final Logger logger = Logger.getLogger(StoredErrorWriter.class);
   

   It makes possible to configure its level according to to their needs.

3. errorMessage is never null here:

   ErrorMessage errorMessage = new UserError(ERROR_MSG
           + errorNumber);
   
   if (errorMessage != null) {
       component.setComponentError(errorMessage);
       logger.log(Level.SEVERE, trace);
       return;
   }
   

   Two possible cases: new UserError throw an exception or returns a new object. Therefore, the comparison is unnecessary.

4. Anyway, I'd use guard clauses here to make the code flatten, even if it requires a little bit duplication (DefaultErrorHandler.doDefault(event)).

   @Override
   public void error(ErrorEvent event) {
       Throwable throwable = event.getThrowable();
       if (throwable == null) {
           DefaultErrorHandler.doDefault(event);
           return;
       } 
       String trace = getTrace(throwable);
       final String message = throwable.getMessage();
       AbstractComponent component = DefaultErrorHandler
               .findAbstractComponent(event);
       if (component == null) {
           DefaultErrorHandler.doDefault(event);
           return;
       }
       final Integer errorNumber = insertTrace(message, trace);
       ErrorMessage errorMessage = new UserError(ERROR_MSG + errorNumber);
       component.setComponentError(errorMessage);
       logger.log(Level.SEVERE, trace);
   }
   

   Note the longer variable name for the Throwable instance and the message. Short variable names are not too readable. I suppose you have autocomplete, so using longer names does not mean more typing but it would help readers and maintainers a lot since they don't have to remember the purpose of each variable - the name would express the programmers intent. (Clean Code by Robert C. Martin, Avoid Mental Mapping, p25)

5. Instead of

   logger.log(Level.SEVERE, trace);
   

   I guess you could use

   logger.severe(trace);
   

6. This constant is used only once:

   public static String ERROR_MSG = "System error! Error number in journal :";
   

   I'd inline it for the principle of proximity

7. I like final variables, they make the code easier to follow (readers know immediately that a value can't be changed), but the code doesn't use it consistently. Some variables are final, some aren't.

Answer 3

@palacsint and @UriAgassi have both given good answers, and there is not much to add, except one thing: Error

In Java you have Exceptions, and Errors... java.lang.Exception and java.lang.Error. They both extend java.lang.Throwable. Exceptions are thrown when the program or it's input/output has problems that can normally be resolved from inside the program. Errors are thrown when 'bigger things' happen, like a security problem, a physical resource problem (memory), or a class-path problem (Jars with classes incompatible with the current Java version), etc. It is very unusual for Java programs to trap and process Errors.

Often, when an actual Error does occur, handling the Error in a way like you are doing it, is going to cause more problems than it solves (thinking out-of-memory errors here..).

I strongly recommend that you only trap 'Exceptions' (not all Throwables). If there are specific Errors that you want to trap, then I recommend that you catch them explicitly, handle them appropriately, and then re-throw the situation (and log it) using the Error wrapped inside a normal Exception... Then replace all mention of Error in your logger with 'Exception', and replace java.langl.Throwable with java.lang.Exception

Error handling in Java is complicated... I generally recommend that you steer clear of them.