Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
}
share|improve this question
5  
Welcome to Code Review! To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. –  Simon André Forsberg Mar 29 at 10:13
    
Edit: Functionality that overwrite standard error msg with trace to user friendly msg with error number stored in DB –  Speak with system Mar 29 at 10:18

3 Answers 3

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.

share|improve this answer
    
Thanks for answer i will finish source in next week and put it here. –  Speak with system Mar 29 at 11:01
    
@Speakwithsystem / Uri - about the blank line... I disagree. Code inside a method can be put in 'paragraphs' of logical blocks. Adding a blank line before the first paragraph is fine, and I believe it is even necessary if the method declaration is split on multiple lines (e.g. has many parameters or throws exceptions) –  rolfl Mar 29 at 12:40
    
@rolfl, I guess it is a matter for debate. I'm not familiar with any argument against the empty line, I just know that it made the code more difficult to read for me. I copied it aside, and removed the lines, and could read it better. Maybe for long method declarations it might be better, but we are talking about getInfo()... –  Uri Agassi Mar 29 at 13:07
  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.

share|improve this answer
    
Thanks for the tips ! –  Speak with system Mar 29 at 11:06
1  
I am pl/sql programmer so thank you very much for tips in java! –  Speak with system Mar 29 at 11:08
    
@Speakwithsystem: You're welcome! –  palacsint Mar 29 at 11:11

@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.

share|improve this answer
    
Hi! rolfl I am a bit of suspicion! I rewrite code like like posted palacsint if (component == null) DefaultErrorHandler.doDefault(event)@ Begining. This funcionality need for Vaadin based backend App with many of components and many DB transactions. + the whole look of glamor with many components see priview for default error indicator @ [link] (postimg.org/image/oxvjrmflj). All other error if component not found will go around StoredErrorWriter by DefaultErrorHandler.So system/server critical and fatal error will processed. –  Speak with system Mar 29 at 13:54
    
I tested in some situations looks legally. I am wrong ? Some info I founded in Vaadin 7 book and in the this post link –  Speak with system Mar 29 at 13:55
    
I agree with you i remake Writer with my locale Exceptions and some Sql, Hibernate default exceptions. It is like you said Error handling in java is complicated ;) –  Speak with system Mar 29 at 14:51
1  
@Speakwithsystem - Error handling is complicated, Exception handling is not complicated, it just takes discipline –  rolfl Mar 29 at 14:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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