Is it possible to enhance this code further?

The code below is a part of a java file for authentication. I want to know how can i write optimized code for this(especially when i am using database with java what I need to consider). Comment on my code how can I improve it.

Is it up to Standard?

Is it possible to optimize my coder further?

Thanks In advance

Some suggestions:

1. Rather than bringing back all rows of tablename, you can add a where clause that brings back the row(s) that match(es) the username and password. Look at PreparedStatement to add variables to your SQL statement. You can also bring back fewer columns. That is, don't "select *". Instead specify, for example "select user_id", where user_id is a column name. If no rows are returned then you know the login attempt failed.

2. I notice that you only print "Fail" when an exception occurs. What happens when no rows are returned or no rows match your user name and password?

3. You should use variables that are better named. For example, instead of the variable str1, use userName.

4, Don't hardcode column indexes (rs.getString(8)) You have multiple choices here. Look at using constants (private static final) or enums. You can also use column names instead of column numbers.

>> You also need to close the Connection( if you arent using it further) or close the Statement. Put if in a finally block so that it is executed even if there is an exception.
>> Its better to print/log the stacktrace instead of some string say- "Fail", because a Stacktrace can provide more information and will be easier to debug or you can log some useful message. Also its better not to provide a Catch all block-

>> You can create a User defined exception for Authentication Failed and can throw that If the Authentication is not successful.

>> Its better to declare the connection related Strings at one place. Or better you can have a Factory class to give you a connection object.

You can further enhance code following way.

Close the system resource in reverse order of its creation. its is good practice to close .. resultset then Statement and Then connection.
put resource release code always inside finally block.

what i suggest you to use a FindBugs tool and work on the suggestions made by the tool , try to work on the performance Category issues .

Try to run for every piece of code because it might be a lot when ran for the entire Application .

Thanks .

if(rs != null)
{
rs.close();
}
if(stmt != null)
{
stmt.close();
}

Note that closing the Statement closes the ResultSet automatically, so there's no need to do it explicitly.

And I agree with mohamed - DB driver names, URLs, username and password should not be hardcoded; they should be stored elsewhere (maybe in a property file) and read at runtime.

This way of writing code looks bad , because you need to write the same things again and again en every DAO layer class of your Application so i suggest you write a helper class with containing static methods , and pass these Connection , Statement , ResultSet to it and close them in that Method , then you can simply call this static methods of that class from myour finally block .

Instead writing Helper class on your own you can use industry proven solution.

Do not re-invent the wheel.

Apache DBUtil API to handle database related things.

Link to DBUtil http://commons.apache.org/dbutils/
DBUtil class org.apache.commons.dbutils.DbUtils

tahnks Alpesh , this link looks good

http://www.devdaily.com/java/jwarehouse/commons-dbutils-1.0/src/java/org/apache/commons/dbutils/DbUtils.java.shtml

after reading all the informations in this thread,I modified my code and below is my new code,

Please comment on this whether i can enhance this further,

package javaapplication1;

import java.sql.*;

/**
 *
 * @author m.senthil kumar
 */
public class Main {
    public static void main(String[] yahoo) {
Connection con=null;
PreparedStatement pstmt=null;
ResultSet rs=null;
        try {
            Class.forName("sun.jdbc.odbc.JdbcOdbcDriver");
                    con = DriverManager.getConnection("jdbc:odbc:tecch", "uuu", "ppp");
            String userName = "jav1a";//username & assume that getting input from user
            String password = "java";//password & assume that getting input from user
             pstmt = con.prepareStatement("select username from test where username=? and password=?");
            pstmt.setString(1, userName);
            pstmt.setString(2, password);
             rs = pstmt.executeQuery();
            boolean b = false;

while (rs.next()) {
                b = true;
                break;
            }
            if (b) {
                System.out.print("Success");
            } else {
                throw new unauthorized();
            }
        } catch (Exception e) {
            System.out.print(e);
        } finally {
            try{
            if(rs!=null)
                rs.close();
                if (pstmt != null) {
                pstmt.close();
            }
            
            if (con != null) {
                con.close();
            }
            }
            catch(Exception e)
            {
             System.out.print(e);   
            }
        }
    }
}

class unauthorized extends Exception {

public String toString() {
        return "You are not authorized person";
    }
}

This construct is somewhat inelegant.

Consider this which does the same thing

I like the second one better because it doesn't clutter things up.

In general, I would also move the close logic to a helper class. With an API like close(Connection conn, PreparedStatement pstmt, ResultSet rs). It's just boilerplate code so doesn't add to the meaning of what you are doing. Similarly for getting a connection. For learning purposes, this doesn't matter.

I dont understand what is Helper Class? How can i apply that to my program?

Can somebdoy give me a hint about what is helper class , so that I can learn about that

A helper class is another class that your current class calls. It has a narrower focus. For example, maybe it just deals with getting/closing a connection.

I used the findbugs,tool as you suggested , even it is showing some errors though I corrected those things ,the screen shot is below,

Why these errors appeared? Am I using the tools wrongly ?

The Code is Below

package javaapplication1;

import java.sql.*;

/**
 *
 * @author m.senthil kumar
 */
public class Main {
    public static void main(String[] yahoo) {
Connection con=null;
PreparedStatement pstmt=null;
ResultSet rs=null;
        try {
            Class.forName("sun.jdbc.odbc.JdbcOdbcDriver");
                    con = DriverManager.getConnection("jdbc:odbc:MNM", "dfd", "dfd");
            String userName = "java";//username & assume that getting input from user
            String password = "java";//password & assume that getting input from user
             pstmt = con.prepareStatement("select username from test where username=? and password=?");
            pstmt.setString(1, userName);
            pstmt.setString(2, password);
             rs = pstmt.executeQuery();
            boolean b = false;
            if(! rs.next()) {
                throw new unauthorized();              
            }
            System.out.print("Success");

} catch (Exception e) {
            System.out.print(e);
        } finally {
            try{
            if(rs!=null)
                rs.close();
                if (pstmt != null) {
                pstmt.close();
            }
            
            if (con != null) {
                con.close();
            }
            }
            catch(Exception e)
            {
             System.out.print(e);   
            }
        }
    }
}

class unauthorized extends Exception {

public String toString() {
        return "You are not authorized person";
    }
}

Thanks for your time.

Thinking of extensibility and reuse, why don't you move our connection establishment to some other utility class and use it. Also, for this particular application, assuming if you want to use this in some concurrent request mode, you are creating the connection for every request. May be you can use a pool of pre-created connections and upon every single request, try to get a connection from pool.