VPF::нужны коменты к куску кода - Форум программистов

Alexandr87

Дата 24.11.2007, 09:23 (ссылка)

(нет голосов)

Загрузка ...

дыкий псых

Профиль
Группа: Завсегдатай
Сообщений: 1459
Регистрация: 27.11.2004
Где: Алматы, Казахстан

Репутация: 9
Всего: 39

Хочется услышать коменты. Суть простая - закрытие Connection и Statement, без засорения целевого кода try`ами.

Нормальный ли вариант? Если нет, то почему? Как лучше?

Код


    public void writeToDB()
    throws Exception {
        
        Connection conn = null;
        PreparedStatement pstmt = null;
        try {
            conn = DriverManager.getConnection(dbUrl, dbUserName, dbPassword);
            pstmt = conn.prepareStatement(sqlQuery);
            pstmt.executeUpdate();
        } finally {
            Utils.closeStatementAndConnection(pstmt, conn);
        }
    }

Код


public class Utils {
    
    public static void closeStatementAndConnection(PreparedStatement pstmt, 
            Connection conn) {
        
        try {
            pstmt.close();
        } catch (Exception ex) {}
        
        try {
            conn.close();
        } catch (Exception ex) {}
    }
}

ivg

Дата 24.11.2007, 11:32 (ссылка)

(нет голосов)

Загрузка ...

Autonomous R&D

Профиль
Группа: Участник
Сообщений: 686
Регистрация: 8.2.2006
Где: Екатеринбург

Репутация: 33
Всего: 81

ну вот такой вариант

Код


import java.sql.Connection;
import java.sql.SQLException;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

import com.mycompany.myapplication.app.Application;

public abstract class TransactionalUnit  {
    
    private static final Log log = LogFactory.getLog(TransactionalUnit.class);

    abstract protected void inTransaction(Connection connection) throws Exception;

    public void execute() {
        Application app = Application.getInstance();
        Connection con = null;
        try {
            con = app.getConnectionManager().getConnection();
            con.setAutoCommit(false);
            con.setTransactionIsolation(Connection.TRANSACTION_SERIALIZABLE);
            inTransaction(con);
            con.commit();
        } catch (Exception e) {
            if (con != null) {
                try {
                    con.rollback();
                } catch (SQLException se) {
                    log.error("Could not rollback connection");
                }
            }
            throw e;
        } finally {
            if (con != null) {
                try {
                    con.close();
                } catch (SQLException se) {
                    log.error("Could not close connection");
                }
            }
        }

    }

}

ну и гдето в целевом коде например:

Код


private User findUser(String login) {
// ... 
    final User[] res = new User[] { null };
    new TransactionalUnit() {
        protected void inTransaction(Connection con) throws Exception {
            res[0] = UsersDAO.findUserByLogin(con, login);
        }
    }.execute();
    return res[0];
}

ну а со Statement - аналогично - какой нибудь SqlCommand, где все try {...} catch{} finally{} расставлены, а в UserDAO:

Код


public User findUserByLogin(Connection con, String login) {
  return new SqlCommand<User>().exequteQuery(con, "select * from user_table where login = ?", login);
}

По вашему варианту: В общем случае Statement и Connection закрываются не в одном месте, поскольку в течении одного сеанса может быть выполнено несколько query, update.
В блоке finally{} должны быть подавлены все исключения, которые в нём возникают(я думаю понятно почему? Хм, интересно что будет если если этого не сделать, надо поэкспериментировать). То есть метод closeStatementAndConnection() класса Util должен соблюдать это соглашение. Обязательно это нужно указывать в коментарии. Иначе при развитии проекта завтра другим разработчиком это соглашение может быть нарушено.

Это сообщение отредактировал(а) ivg - 24.11.2007, 11:39

Alexandr87

Дата 24.11.2007, 14:45 (ссылка)

(нет голосов)

Загрузка ...

дыкий псых

Профиль
Группа: Завсегдатай
Сообщений: 1459
Регистрация: 27.11.2004
Где: Алматы, Казахстан

Репутация: 9
Всего: 39

Цитата(ivg @ 24.11.2007, 14:32

)

В блоке finally{} должны быть подавлены все исключения, которые в нём возникают(я думаю понятно почему? Хм, интересно что будет если если этого не сделать, надо поэкспериментировать). То есть метод closeStatementAndConnection() класса Util должен соблюдать это соглашение. Обязательно это нужно указывать в коментарии. Иначе при развитии проекта завтра другим разработчиком это соглашение может быть нарушено.

так они и так давятся. А развитие этой функции никакое не предвидется.

В-целом, очень ценный для меня комментарий. Большое спасибо.

w1nd

Дата 24.11.2007, 15:46 (ссылка)

(нет голосов)

Загрузка ...

Вертилятор

Профиль
Группа: Завсегдатай
Сообщений: 1077
Регистрация: 22.3.2006
Где: Москва

Репутация: 20
Всего: 54

Цитата(ivg @ 24.11.2007, 11:32

)

Мне не понятно. Может быть, разъясните?
Кроме того:

Код

// Представленный код
try {
    connection = ...
}
catch (Exception thrown) {
    ...
}
finally {
    ...
}

//Как следует делать
try {
    connection = ...

    try {
        ...
    }
    finally {
        // closing connection
    }
}
catch (Exception thrown) {
    ...
}

Это сообщение отредактировал(а) w1nd - 24.11.2007, 15:48

--------------------

ivg

Дата 24.11.2007, 16:26 (ссылка)

(нет голосов)

Загрузка ...

Autonomous R&D

Профиль
Группа: Участник
Сообщений: 686
Регистрация: 8.2.2006
Где: Екатеринбург

Репутация: 33
Всего: 81

Цитата(ivg @ 24.11.2007, 11:32

)

Хм, интересно что будет если если этого не сделать, надо поэкспериментировать

Поэкспериментировал. При возникновении исключений и в try{} и в finally{} ловится последнее, что вобщем то логично.

Цитата(Alexandr87 @ 24.11.2007, 14:45

)

так они и так давятся. А развитие этой функции никакое не предвидется.

А ведь даже в этом случае исключения в блоке finally{} возможны например если вставить в класс Utils:

Код


static {
    if (System.currentTimeMillis() > 0L) {
        throw new RuntimeException("StaticInitializerException");
    }
}

Притянуто за уши конечно, класс Utils может быть загружен раньше вызова writeToDB().

Цитата(w1nd @ 24.11.2007, 15:46

)

Мне не понятно. Может быть, разъясните?

Если в блоке finally{} возникнет исключение то при раскрутке стека вызовов наверх попадет оно. Исключение возникшее ранее в блоке try{} потеряется, а оно, как правило, более информативно, более важно, что ли. Хотя если например вот такая конструкция:

Код


try {
  //.....
} catch(Throwable t) {
  // всё что возникает в try {} ловится и обрабатывается здесь же без проталкивания наверх
} finally {
  // ...
}

то можно не беспокоится

w1nd, последнее не совсем понял вопрос, хотя может уже ответил?

Это сообщение отредактировал(а) ivg - 24.11.2007, 16:30

w1nd

Дата 24.11.2007, 16:41 (ссылка)

(нет голосов)

Загрузка ...

Вертилятор

Профиль
Группа: Завсегдатай
Сообщений: 1077
Регистрация: 22.3.2006
Где: Москва

Репутация: 20
Всего: 54

Цитата(ivg @ 24.11.2007, 16:26

)

Если в блоке finally{} возникнет исключение то при раскрутке стека вызовов наверх попадет оно. Исключение возникшее ранее в блоке try{} потеряется

Только в том случае, когда вы _потеряете_ его намеренно. Исключение, возникшее при закрытии соединения не менее важно, чем все прочие. Игнорировать исключение - суть обработать его. В утилитных классах вы не можете адекватно обработать эти исключения, а значит игнорировать их нельзя.

Цитата(ivg @ 24.11.2007, 16:26

)

w1nd, последнее не совсем понял вопрос, хотя может уже ответил?

Код

try {
    connection = ...
}
catch (...) {
    // Совершенно непонятно, что за ошибка обрабатывается здесь. 
    // Что именно не удалось сделать? Открыть соединение? Выполнить запрос? Закрыть транзакцию?
    // Что следует делать дальше?
}
finally {
    // Сюда мы попадём в любом случае, даже если это не требуется,
    // поэтому каким-то образом придётся определять состояние.
}

З. Ы. Кстати, для сигнализации об ошибках в статическом инициализаторе есть ExceptionInInitializerError.

Это сообщение отредактировал(а) w1nd - 24.11.2007, 16:44

--------------------

ivg

Дата 24.11.2007, 17:50 (ссылка)

(нет голосов)

Загрузка ...

Autonomous R&D

Профиль
Группа: Участник
Сообщений: 686
Регистрация: 8.2.2006
Где: Екатеринбург

Репутация: 33
Всего: 81

Цитата(w1nd @ 24.11.2007, 16:41

)

Только в том случае, когда вы _потеряете_ его намеренно

Речь идёт о простых конструкциях, без Классов исключений, которые хранят в себе информацию о нескольких исключениях и без оборачивания try{} в try{}?
Если так, вот простой код:

Код


public class Main {

    public static void main(String... args) {
        try {
            test();
        } catch (TestException te) {
                        System.out.println("Bingo!!!!");
            te.printStackTrace();
        } catch (OtherException oe) {
            oe.printStackTrace();
        } catch (Exception e) {
            e.printStackTrace();
        }
        
    }
    
    private static void test() throws Exception {
        try {
            genTestEx();
        } finally {
            genOtherEx();
        }
        
    }
    
    private static void genTestEx() throws TestException {
        throw new TestException("TestException"); // TestException extends Exception
    }
    
    private static void genOtherEx() throws OtherException {
        throw new OtherException("OtherException"); // OtherException extends Exception
    }
}

Вопрос: как в методе main() поймать TestException?

Цитата(w1nd @ 24.11.2007, 16:41

)

Исключение, возникшее при закрытии соединения не менее важно, чем все прочие. Игнорировать исключение - суть обработать его.

Я согласен, только "подавлено" не значит "проигнорировано", я имел ввиду не выпускать за пределы блока finally{}

Цитата(w1nd @ 24.11.2007, 16:41

)

try {
    connection = ...
}
catch (...) {
    // Совершенно непонятно, что за ошибка обрабатывается здесь.
    // Что именно не удалось сделать? Открыть соединение? Выполнить запрос? Закрыть транзакцию?
    // Что следует делать дальше?
}
finally {
    // Сюда мы попадём в любом случае, даже если это не требуется,
    // поэтому каким-то образом придётся определять состояние.
}

Это по тому коду что я привёл? Если так, то это просто демонстрация идеи, как избавиться от

Цитата(Alexandr87 @ 24.11.2007, 09:23

)

засорения целевого кода try`ами

Ясно, что код не идеальный и для всех случаев в жизни не подойдёт.

w1nd

Дата 24.11.2007, 17:54 (ссылка)

(нет голосов)

Загрузка ...

Вертилятор

Профиль
Группа: Завсегдатай
Сообщений: 1077
Регистрация: 22.3.2006
Где: Москва

Репутация: 20
Всего: 54

Цитата(ivg @ 24.11.2007, 17:50

)

Речь идёт о простых конструкциях, без Классов исключений, которые хранят в себе информацию о нескольких исключениях и без оборачивания try{} в try{}?

Нет конечно. Речь о создании свои типов исключений (более высокоуровневые), об обработке всех исключений.

Это сообщение отредактировал(а) w1nd - 24.11.2007, 17:54

--------------------

ivg

Дата 25.11.2007, 00:26 (ссылка)

(нет голосов)

Загрузка ...

Autonomous R&D

Профиль
Группа: Участник
Сообщений: 686
Регистрация: 8.2.2006
Где: Екатеринбург

Репутация: 33
Всего: 81

Поразмыслил тут на досуге smile

.w1nd, надо признать критика справедлива. Пытаюсь исправиться:

Код


import java.sql.Connection;
import java.sql.SQLException;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

import com.mycompany.myapplication.app.*;

public abstract class TransactionalUnit {

    private static final Log log = LogFactory.getLog(TransactionalUnit.class);

    abstract protected void inTransaction(Connection connection) throws Exception;

    public void execute() {
        Application app = Application.getInstance();
        Connection con = app.getConnectionManager().getConnection();
        ExceptionList<RuntimeException> elist = null; // extends RuntimeException implements List<RuntimeException>
        try {
            inTransaction(con);
            con.commit();
        } catch (DataAccessException dae) { // Needs rollback
            elist = new ExceptionList<RuntimeException>();
            elist.add(dae);
            try {
                con.rollback();
            } catch (SQLException se) {
                elist = elist == null ? new ExceptionList<RuntimeException>() : elist;
                elist.add(se);
                log.error("Could not rollback connection");
            }
        } catch (Exception e) { // No need rollback
            elist = new ExceptionList<RuntimeException>();
            elist.add(new RuntimeException(e)); // wrap to unchecked
        } finally {
            try {
                con.close();
            } catch (SQLException se) {
                elist = elist == null ? new ExceptionList<RuntimeException>() : elist;
                elist.add(se);
                log.error("Could not close connection");
            }
        }
        if (elist != null && elist.size() > 0) {
            if (elist.size() == 1) {
                throw elist.get(0);
            } else {
                throw elist;
            }
        }
    }

}

Стандартных классов для работы с несколькими исключениями как я понимаю нет?

Alexandr87

Дата 25.11.2007, 06:07 (ссылка)

(нет голосов)

Загрузка ...

дыкий псых

Профиль
Группа: Завсегдатай
Сообщений: 1459
Регистрация: 27.11.2004
Где: Алматы, Казахстан

Репутация: 9
Всего: 39

надо будет найти время перечитать всю тему. Всем большое спасибо за обсуждение.

0 Пользователей читают эту тему (0 Гостей и 0 Скрытых Пользователей)
0 Пользователей:
« Предыдущая тема \| Java: Общие вопросы \| Следующая тема »