Update command does not work

Asked

Viewed 552 times

3

I have three tables in a database, and I have a method called alteraLinhaTabela where I try to update one of the three tables at a time by identifying it by the variable tabela. I also pass two more classes that contain the update content, but for each table a different class, one will be null while the other one doesn’t.

Ignored the if (tabela == 1) {...}, I got a second if where you choose the SQL statement to be executed for each table. I create the PreparedStatement and Seto the variables of one of the classes (this "setting" happens in the method dadosDasLinhas, which I also use in other methods), execute, and close the PreparedStatement.

public void alteraLinhaTabela(int tabela, Tabela1 t1, Tabela2 t2) {

    String query = "";

    try {

        if (tabela == 1) {

            if (t1.getPagamentoTotal() == 0) {

                deleteLinha(t1.getId(), 1);

                tabela = 3;

                insereLinhaTabela(tabela, t1, t2);

                return;

            }

        }

        if (tabela == 1 || tabela == 3)
            query = "update tabela" + tabela + " set nome=?, dataPagamento=?, atrazo=?, pagamentoTotal=? where id=?";
        else if (tabela == 2)
            query = "update tabela" + tabela + " set endereco=?, telefone=?, cpf=?, produto=?, valorProduto=? where id=?";

        PreparedStatement stmt = getConexao().prepareStatement(query);

        dadosDasLinhas(stmt, tabela, t1, t2);

        if (tabela == 1) 
            stmt.setInt(5, t1.getId());
         else if (tabela == 2) 
            stmt.setInt(6, t2.getId());

        stmt.execute();

        stmt.close();   

    } catch(SQLException e) {

        throw new RuntimeException(e);

    }

}

private void dadosDasLinhas(PreparedStatement stmt, int tabela, Tabela1 t1, Tabela2 t2) {

    try {

        if (tabela == 1) {

            stmt.setString(1, t1.getNome());
            stmt.setString(2, t1.getDataPagamento());
            stmt.setInt(3, t1.getAtrazo());
            stmt.setDouble(4, t1.getPagamentoTotal());

        } else if (tabela == 2) {

            stmt.setString(1, t2.getEndereco());
            stmt.setString(2, t2.getTelefone());
            stmt.setString(3, t2.getCpf());
            stmt.setString(4, t2.getProduto().toString().replace(" ", "")//retira espaços
                    .replace("[", "").replace("]", ""));//retira cochetes
            stmt.setString(5, t2.getValorProduto().toString().replace(" ", "")//retira espaços
                    .replace("[", "").replace("]", ""));//retira cochetes

        }

    } catch(SQLException e) {

        throw new RuntimeException(e);

    }

}

Whereas I just want to update the first table (which has the columns name, timePath, delay and pay), if I have a row with:

name = Test Subject,
dataPagamento = 2017-03-07,
delay = 5,
payment = 1450;

I only change the delay and execute the method alteraLinhaTabela.

But when I make one SELECT I see you didn’t change anything. If I see the console, it’s empty. If I try the command line, it works. If I’m looking for an error in the SQL statement, I can’t find it. If I see the connection to the database, it’s all right (and even inserts lines). If I debug, I see that it goes through all the lines and does not present any error or lack of data in any variable.

I have no idea what might be going on.

The database is Oracle Database 11g Express Edition. The IDE is Eclipse.

  • What’s in your method getConexao()? I have a half written answer, but I cannot finish it without knowing how this method is implemented.

1 answer

6


Control coupling

Before answering your question directly, allow me to talk a little bit about the control coupling.

If you have a method that has a parameter that denotes what it has to do and inside you have one if or a switch that based on this parameter decides the desired behavior, even more when a type of behavior has little or no relation to another type of behavior, is because they should be separate methods. This is an example of control coupling.

The control involvement gets even worse when some of the parameters serve only for some of the behaviors while the others serve other behaviors, but they are all in the same method signature. An example of a severe case of control coupling would be this:

public String desenharCirculoOuEscreverXML(int oQueFazer, Graphics g, Produto p) {
    if (oQueFazer == 1) {
        g.setColor(Color.RED);
        g.drawEllipse(20, 20, 50, 50);
    } else if (oQueFazer == 2) {
        return "<produto><nome>" + p.getNome() + "</nome><codigo>" + p.getCodigo() + "</codigo></produto>";
    }
    return null;
}

Note that this method houses and mixes two completely different functionalities that have no sense of being together, which results in confusing code, spaghetti, tied, gambiarrado, difficult to use and difficult to change. Everything would be so much easier if they were separated:

public void desenharCirculo(Graphics g) {
    g.setColor(Color.RED);
    g.drawEllipse(20, 20, 50, 50);
}

public String escreverXML(Produto p) {
    return "<produto><nome>" + p.getNome() + "</nome><codigo>" + p.getCodigo() + "</codigo></produto>";
}

Removing the control involvement

Well, your code has a serious control coupling problem, both in the method alteraLinhaTabela how much in the method dadosDasLinhas. To do so, let’s first separate the features that are mixed:

public void alteraLinhaTabela1(Tabela1 t1) {
    if (t1.getPagamentoTotal() == 0) {
        deleteLinha(t1.getId(), 1);
        insereLinhaTabela3(t1);
        return;    
    }

    try {
        String query = "update tabela1 set nome=?, dataPagamento=?, atrazo=?, pagamentoTotal=? where id=?";

        PreparedStatement stmt = getConexao().prepareStatement(query);

        dadosDasLinhas1(stmt, t1);

        stmt.setInt(5, t1.getId());
        stmt.execute();
        stmt.close();   
    } catch(SQLException e) {
        throw new RuntimeException(e);
    }
}

public void alteraLinhaTabela2(Tabela2 t2) {
    try {
        String query = "update tabela2 set endereco=?, telefone=?, cpf=?, produto=?, valorProduto=? where id=?";

        PreparedStatement stmt = getConexao().prepareStatement(query);

        dadosDasLinhas2(stmt, t2);

        stmt.setInt(6, t2.getId());
        stmt.execute();
        stmt.close();   
    } catch(SQLException e) {
        throw new RuntimeException(e);
    }
}

private void dadosDasLinhas1(PreparedStatement stmt, Tabela1 t1) {
    try {
        stmt.setString(1, t1.getNome());
        stmt.setString(2, t1.getDataPagamento());
        stmt.setInt(3, t1.getAtrazo());
        stmt.setDouble(4, t1.getPagamentoTotal());
    } catch(SQLException e) {
        throw new RuntimeException(e);
    }
}

private void dadosDasLinhas2(PreparedStatement stmt, Tabela2 t2) {
    try {
        stmt.setString(1, t2.getEndereco());
        stmt.setString(2, t2.getTelefone());
        stmt.setString(3, t2.getCpf());
        stmt.setString(4, t2.getProduto().toString().replace(" ", "")//retira espaços
                .replace("[", "").replace("]", ""));//retira cochetes
        stmt.setString(5, t2.getValorProduto().toString().replace(" ", "")//retira espaços
                .replace("[", "").replace("]", ""));//retira cochetes
    } catch(SQLException e) {
        throw new RuntimeException(e);
    }
}

Class responsibility

I think it’s somewhat obvious that the functionality that provides the name of a Produto should be in class Produto. It is also obvious that this name should, if possible, be properly formatted so that other classes do not need to keep fixing it. However, that’s not what your code shows:

        stmt.setString(4, t2.getProduto().toString().replace(" ", "")//retira espaços
                .replace("[", "").replace("]", ""));//retira cochetes
        stmt.setString(5, t2.getValorProduto().toString().replace(" ", "")//retira espaços
                .replace("[", "").replace("]", ""));//retira cochetes

This indicates that the class Produto does not provide its name in a correct format, which forces other classes to have to fix it. Now, this means that the class Produto is not doing its part right and that therefore other classes end up having to fix/fix/correct what this class is not doing right but should know, what is a gambiarra. Therefore, the ideal is that the class Produto provide a method that provides the name in the correct format. The same can be said about the value of the product in the class Tabela2.

In fact, this has everything to do with the principle of inversion of dependency - Your code is depending on the details of the behavior of the methods, not just on calling the methods. From this principle can also be inferred the rule don’t talk to strangers - that is, the method should not call other methods than directly related objects. The best alternative then would be to put this in the class Produto:

public String getNomeLimpo() {
    return toString().replace(" ", "").replace("[", "").replace("]", "");
}

And that in class Tabela2:

public String getNomeLimpoProduto() {
    return getProduto().getNomeLimpo();
}

public String getValorLimpoProduto() {
    return getValorProduto().toString().replace("[", "").replace("]", "");
}

And with that, in the method dadosDasLinhas2 we have:

        stmt.setString(4, t2.getNomeLimpoProduto());
        stmt.setString(5, t2.getValorLimpoProduto());

Security when using the PreparedStatement

Your code does not deal with the PreparedStatement adequately. In particular, if the code makes an exception, the PreparedStatement can remain open in a zombie state as well as the connection. The ideal way to deal with this problem is to use the Try-with-Resources.

In fact, not only the PreparedStatement, as the Connection must also be in a zombie state, since you do not finish it and do not execute the commit. The connection must be in auto-commit mode (and therefore the commit explicit is unnecessary), but even so, it must be properly closed. This must be the flaw you’re reporting which causes nothing to appear in the database.

We can also move Sqls to static fields.

private static final String ALTERA_LINHA_TABELA1_SQL =
    "UPDATE tabela1 SET nome = ?, dataPagamento = ?, atrazo = ?, pagamentoTotal = ? WHERE id = ?";

private static final String ALTERA_LINHA_TABELA2_SQL =
    "UPDATE tabela2 SET endereco = ?, telefone = ?, cpf = ?, produto = ?, valorProduto = ? where id = ?";

public void alteraLinhaTabela1(Tabela1 t1) {
    if (t1.getPagamentoTotal() == 0) {
        deleteLinha(t1.getId(), 1);
        insereLinhaTabela3(t1);
        return;    
    }

    try (
        Connection c = getConexao();
        PreparedStatement stmt = c.prepareStatement(ALTERA_LINHA_TABELA1_SQL);
    ) {
        dadosDasLinhas1(stmt, t1);
        stmt.setInt(5, t1.getId());
        stmt.execute();
    } catch (SQLException e) {
        throw new RuntimeException(e);
    }
}

public void alteraLinhaTabela2(Tabela2 t2) {
    try (
        Connection c = getConexao();
        PreparedStatement stmt = c.prepareStatement(ALTERA_LINHA_TABELA1_SQL);
    ) {
        dadosDasLinhas2(stmt, t2);
        stmt.setInt(6, t2.getId());
        stmt.execute();
    } catch (SQLException e) {
        throw new RuntimeException(e);
    }
}

private void dadosDasLinhas1(PreparedStatement stmt, Tabela1 t1) throws SQLException {
    stmt.setString(1, t1.getNome());
    stmt.setString(2, t1.getDataPagamento());
    stmt.setInt(3, t1.getAtrazo());
    stmt.setDouble(4, t1.getPagamentoTotal());
}

private void dadosDasLinhas2(PreparedStatement stmt, Tabela2 t2) throws SQLException {
    stmt.setString(1, t2.getEndereco());
    stmt.setString(2, t2.getTelefone());
    stmt.setString(3, t2.getCpf());
    stmt.setString(4, t2.getNomeLimpoProduto());
    stmt.setString(5, t2.getValorLimpoProduto());
}

Connection management

I assumed your method getConexao() return a newly opened connection in autocommit mode (which is the default) and should be closed after being used (connect, do something, disconnect). I strongly recommend you do this if you’re not doing it.

If this is not the case and you want to insist on reusing a connection, then the beginning of the blocks try would be so:

try (PreparedStatement stmt = getConexao().prepareStatement(ALTERA_LINHA_TABELA1_SQL)) {

However, dealing properly with previously open connections is not a very easy thing to do. The only thing that’s easy in this case is the possibility of screwing up. Seeing the maturity level of the rest of your code, I doubt you have reusable connections implemented correctly, because if you were not having the problem you are having (sorry for the sincerity).

If you were to use previously opened connections in your method getConexao(), you will probably want to have well delimited transactions, and with that would have auto-commit disabled and the commit() and the rollback() explicit somewhere in your code. In addition, you would have to worry about delimiting the scope of the transaction, which usually corresponds to a request in the case of a web system or a action in the case of a desktop system. With this, to deal correctly with these concepts, you have to know how to deal with ThreadLocals and clear the connection if the thread in question can be reused (typical case of web applications). Another and more complicated problem would be dealing with pool connections.

Therefore, I recommend that your method getConexao() return connections that have just been opened, with the autocommit enabled and that are always used with the Try-with-Resources to ensure proper closure. I suggest trying more complex connection management only when you are already a Jedi Knight in Java, because this is not something for Padawans.

Other possible improvements

There are probably more improvements that would be possible, but that depends on more information about your project that is not in your question. In particular I believe that improvements still fit in the following areas:

  • Delay is spelled "s", not "z"!

  • The value of the product should be something coming from within the product and not from Tabela2.

  • The fact of Produto and ValorProduto have methods toString() that produce results that need to be repaired later is somewhat suspicious.

  • I don’t think the if (t1.getPagamentoTotal() == 0) { ... } is in the most suitable place.

  • Utilise catch (SQLException e) { throw new RuntimeException(e); } probably not a good way to treat mistakes.

  • That was the reason why it didn’t work, I kept modifying several classes that had this coupling until I got to the class where I do the editing, and then I realized I was doing this, this class I use as a controller. fxml(javafx) that serves to insert data, so I thought I’d put it to change data too (since I was going to practically duplicate the class and .fxml if I did it separately), but I forgot to change the id that looked like -1. I thought that if I put to change, insert, etc. all tables in the same method would look better, but it only made the code worse.

  • His answer also showed me that I still have much to improve, I thought I was already a good programmer. But this mistake of mine in Portuguese is unjustifiable(How I missed it?).

Browser other questions tagged

You are not signed in. Login or sign up in order to post.