Too much comments or is this okay?

[Maximilian]

Heres a method I coded for a servlet, it deals with the user requesting to change their email address. I tend to put comments next to the if/else conditions like that quite often but ive read too much comments can be bad. If im honest these massively help me when I go back to refactor something like this and ive totally forgot whats going on in the code. What do you guys think?

 protected void processRequest(HttpServletRequest request, HttpServletResponse response)
            throws ServletException, IOException
    {
        ServletContext sc = this.getServletContext();
        HttpSession session = request.getSession();
        String id_user = (String) session.getAttribute("id_user");
        String password = request.getParameter("password");
        String newEmail = request.getParameter("newEmail");
        boolean success = false;

        //if user has entered the correct password
        if (Authorization.isCurrentUserAuthorized(password, request))
        {

            //if the users email does not already exist in the datbase
            if (!DatabaseAccess.userAlreadyExistsCheckEmail(newEmail))
            {
                success = DatabaseAccess.changeEmail(newEmail, id_user);
                
                //if email changed successfully
                if (success)
                {
                    session.setAttribute("email", newEmail);
                    RequestDispatcher rd = sc.getRequestDispatcher("/" + GlobalValues.getWebPagesDirectory() + "/" + GlobalValues.getSettingsPage());
                    PrintWriter out = response.getWriter();
                    out.println("<div class=\"alert alert-success\" role=\"alert\">Email changed successfully.</div>");
                    rd.include(request, response);
                } else //if email change failed for some reason
                {
                    RequestDispatcher rd = sc.getRequestDispatcher("/" + GlobalValues.getWebPagesDirectory() + "/" + GlobalValues.getSettingsPage());
                    PrintWriter out = response.getWriter();
                    out.println("<div class=\"alert alert-danger\" role=\"alert\">Email change request failed.</div>");
                    rd.include(request, response);
                }
            } else //if the users new email is already in the database
            {
                RequestDispatcher rd = sc.getRequestDispatcher("/" + GlobalValues.getWebPagesDirectory() + "/" + GlobalValues.getSettingsPage());
                PrintWriter out = response.getWriter();
                out.println("<div class=\"alert alert-danger\" role=\"alert\">Email already exists!</div>");
                rd.include(request, response);
            }

        } else //user entered incorrect password
        {
            RequestDispatcher rd = sc.getRequestDispatcher("/" + GlobalValues.getWebPagesDirectory() + "/" + GlobalValues.getSettingsPage());
            PrintWriter out = response.getWriter();
            out.println("<div class=\"alert alert-danger\" role=\"alert\">Incorrect password.</div>");
            rd.include(request, response);
        }
    }

 

[Bob Ghengis Khan]

I don't feel like any of these comments give any extra information

I also haven't declared all of my variables at the start of a method since college

 

[MagnusTheBrewer]

The comments are fine. The main purpose of comments isn't to provide more information, it's to improve readability and speed making changes to the code. My objection is using else if statements for this.

 

[Ken g6]

I'd say there's two schools of thought on this. On one hand, you should comment where it's not clear what's going on. Given the code that's there, these comments look extremely good. I'd even say they are something probably many of us () should aspire to.

Spoiler

In case it wasn't clear enough, I mean I should comment this well.

The other school of thought is that your code should be clear enough that you don't need (such extensive) comments. One argument for this is that the code might get changed, but the comment might not. For instance:

//if user has entered the correct password
if (Authorization.isCurrentUserAuthorized(password, request))

That call looks rather strange to me. Why are you including the whole request object, as well as the password from that object? This method makes me ask the question, "Is current user authorized for what?", and makes me think the request object might contain instructions to check user authorization for a specific area of functionality.

If the comment is correct, I would suggest naming the method something like:

Authorization.verifyCurrentUserPasswordMatches(password)

Or, if you really do need the request object:

Authorization.verifyCurrentUserAuthentication(request)

And then you wouldn't need a comment there. If the comment is not correct, then that's an argument against such extensive comments.

I will add that I really like how you put comments on the else's, and I don't see a good code alternative to that.

 

[nickbits]

I think it is obvious what is going on from your veryLongMethodNames, which is good.
I find comments like that make the code harder to maintain, not easier, because you have to be careful to keep them attached to the relevant code.

 

[Merad]

Well to be blunt I think the comments are perhaps compensating for messy code. Consider:

1. Inconsistent coding style, some variables are declared where needed, others are at the top of the function
2. Inconsistent coding style, some places use if success else failure, others use if failure else success
3. Lots of redundant code, the same block is repeated 4 different places with only one input changing
4. Big and nasty string constants mixed with code
5. String concatenation with "+" is always ugly

Take a look at this refactor:

private static final string badPassword = "<div class=\"alert alert-danger\" role=\"alert\">Incorrect password.</div>";
private static final string emailExists = "<div class=\"alert alert-danger\" role=\"alert\">Email already exists!</div>";
private static final string emailChanged = "<div class=\"alert alert-success\" role=\"alert\">Email changed successfully.</div>";
private static final string emailChangeFailed = "<div class=\"alert alert-danger\" role=\"alert\">Email change request failed.</div>";

protected void processRequest(HttpServletRequest request, HttpServletResponse response)
            throws ServletException, IOException
{
    String password = request.getParameter("password");
    string output;
    
    if (Authorization.isCurrentUserAuthorized(password, request))
    {
        HttpSession session = request.getSession();
        String id_user = (String) session.getAttribute("id_user");
        String newEmail = request.getParameter("newEmail");        
        
        if (DatabaseAccess.userAlreadyExistsCheckEmail(newEmail))
        {
            output = emailExists;
        }
        else if (DatabaseAccess.changeEmail(newEmail, id_user))
        {   
            session.setAttribute("email", newEmail);
            output = emailChanged;
        }
        else
        {
            output = emailChangeFailed;
        }
    }
    else
    {
        output = badPassword;
    }
    
    string url = string.Format("/%s/%s", GlobalValues.getWebPagesDirectory(), GlobalValues.getSettingsPage());
    RequestDispatcher rd = getServletContext().getRequestDispatcher(url);
    response.getWriter().println(output);
    rd.include(request, response);
}

Much more readable, don't you think? Of course, a lot of this depends on the context. If you're refactoring or writing new code you can do your best to make it clean. If you're stuck maintaining existing code that's nasty then you add comments or whatever you need to while you pray for eventual time or permission to refactor the nastiness away.

 

[Gryz]

Those comments are awesome. Don't let anyone discourage you to keep doing that. I have a lot more comments in my code myself. And nobody will be able to tell me that is bad.

The comment that a lot of code is repeated is of course true. What I sometimes do myself is, after I have written code, debugged it, and I am confident it works, I will reread my own code. And I'll see if I can optimize it. Once you got the logic working, it's just a matter of "massaging" the code to get it into another form. Make it shorter, or more readable.

I find comments like that make the code harder to maintain, not easier, because you have to be careful to keep them attached to the relevant code.

This of course, is bullocks.
The hard part about programming is read other people's code. Everybody can write "a program". Correctness is child's play. The real question is: "if you wrote it, can other people read it" ?

After you spent an hour or a day fixing, improving or modifing code, it only takes 5-10 minutes to check the comments, and maybe rewrite them. You do that once. But the code is probably gonna be read dozens (or hundreds) of times by dozens of other people. If you spend 10 minutes to document your code through a few comments and a few well-chosen function-names, you will probably save dozens of people a multitude of that lost time.

 

[Merad]

Another good guideline is to comment on why, not what. What you're doing should almost always be evident from the code itself when you're using good variable and method names. For example

//if user has entered the correct password
if (Authorization.isCurrentUserAuthorized(password, request))

This comment is IMO 100% redundant with the following line of code. All you're doing is taking the verb "authorized" and replacing it with the synonymous phrase "entered the correct password". It isn't much different from

// set foo to five
int foo = 5;

The why something is done isn't always evident, and that's when comments are best. This line for example (after the email change).

session.setAttribute("email", newEmail);

Probably justifies a comment, maybe something like.

// session state isn't refreshed until <condition> so we need to be manually
// update the new email
session.setAttribute("email", newEmail);

 

[purbeast0]

i think the comments are fine but a bit unnecessary just because your code is very readable. however i do find that having comments like yours to break up reading straight code is easy on the eyes, especially in an IDE where those comments would be a different color. so imo that is fine by me.

i'd add a block comment to the function though if you don't have one already. i have no clue what "process request" means.

if we're nitpicking here i'd nitpick your variable names. some are using under_score notation and some are using camelCase. i'm a fan of camelCase by far so i'd go with that.

also i dont' like the "ServletContext sc", i'd make that "ServletContext servletContext" because down the code, if I read "sc" i don't know if you're referring to a state or what.

and since i'm already nitpicking, i am not a fan of having the "else" on the same line as the closing bracket, but that is just 100% personal opinion.

EDIT:

i also agree that you should refactor. as someone else mentioned, a lot of the time when i do work i'll do it to get it working kind of like how you did, then i will go back and refactor parts that are using redundant code. in general i try to make functions no longer than like 5-10 lines, but obviously they can't all be that way. but if i have a large block of code that is like 50 lines, i will try to figure out how i can logically break that up into chunks so i can call 5-10 functions in that long method to do the job instead of having a large block in there. but again, it simply isn't ALWAYS the case but kind of something i go for.

 

[Markbnj]

I'm not going to dive into the java because I don't write it, but as a general rule I like your intent in your approach to commenting. You're documenting the points of decision and what's being considered there, and that's a good thing.

 

[Maximilian]

Thanks for the feedback guys, some great points being made here!

So in general the comment strategy is decent but that particular method is a bit overly convoluted.

@Merad that is a really nice refactor :thumbsup: It definitely dosent look like it needs comments at all. Rather than just copy/paste what you did I looked at your strategy and tried it myself and came up with the code below. You're right about the inconsistent if success else failure aspect. I looked at this but then I realized the reason it was like that. I coded it using the if clause for any success scenario and else for a failure scenario, regardless of the actual boolean value involved. Like if its true that the new email the user wants already exists then this is a failure scenario because they cant change their email to that since its already taken. Im not sure if its best to do that or to do it your way.

Also I wasent sure what the string.Format method at the end was for.

        private static final String emailChanged = "<div class=\"alert alert-success\" role=\"alert\">Email changed successfully.</div>";
    private static final String emailChangeFailed = "<div class=\"alert alert-danger\" role=\"alert\">Email change request failed.</div>";
    private static final String emailAlreadyExists = "<div class=\"alert alert-danger\" role=\"alert\">Email already exists!</div>";
    private static final String incorrectPassword = "<div class=\"alert alert-danger\" role=\"alert\">Incorrect password.</div>";

    protected void processRequest(HttpServletRequest request, HttpServletResponse response)
            throws ServletException, IOException
    {
        HttpSession session = request.getSession();
        String id_user = (String) session.getAttribute("id_user");
        String password = request.getParameter("password");
        String output;

        if (Authorization.isCurrentUserAuthorized(password, id_user))
        {
            String newEmail = request.getParameter("newEmail");
            
            if (!DatabaseAccess.userAlreadyExistsCheckEmail(newEmail))
            {
                boolean success = DatabaseAccess.changeEmail(newEmail, id_user);

                if (success)
                {
                    output = emailChanged;
                    
                    //current session must be updated with the new email address
                    //otherwise it will contain the wrong address until next login
                    session.setAttribute("email", newEmail);
                } else
                {
                    output = emailChangeFailed;
                }
            } else
            {
                output = emailAlreadyExists;
            }

        } else
        {
            output = incorrectPassword;
        }
        
        ServletContext servletContext = this.getServletContext();
        RequestDispatcher rd = servletContext.getRequestDispatcher("/" + GlobalValues.getWebPagesDirectory() + "/" + GlobalValues.getSettingsPage());
        PrintWriter out = response.getWriter();
        out.println(output);
        rd.include(request, response);
    }

@keng6 Yeah the Authorized.isCurrentUserAuthorized() method was poorly thought out. It needed the request to get the session object to get the id_user value which is stored in the session. Most calling servlets need to get the session for something anyway so the servlet can simply get the id_user value itself. I changed it so it looks like Authorized.isCurrentUserAuthorized(String password, String id_user)

@purbeast Honestly im not a fan of where the else goes either but its whats netbeans does when I use the autoformat alt+shift+f command. I quite like that feature so I just tend to let it do its thing. Also yeah the variable names are a bit of a mess in places they could defo use a refactor to bring some consistency. It all stemmed from my uncertainty of what to name database columns.

 

[Merad]

You're right about the inconsistent if success else failure aspect. I looked at this but then I realized the reason it was like that. I coded it using the if clause for any success scenario and else for a failure scenario, regardless of the actual boolean value involved. Like if its true that the new email the user wants already exists then this is a failure scenario because they cant change their email to that since its already taken. Im not sure if its best to do that or to do it your way.

Yeah, it's hard to keep things clean when you have that kind of situation. A lot of it is really just person preference. Another alternative would be to stick the output code in a helper method, so that you can call it to perform output then return immediately after a failure condition. This is actually the style I usually prefer because I think it tends to make the control flow more obvious than a comparable setup with nested if's.

private static final String emailChanged = "<div class=\"alert alert-success\" role=\"alert\">Email changed successfully.</div>";
private static final String emailChangeFailed = "<div class=\"alert alert-danger\" role=\"alert\">Email change request failed.</div>";
private static final String emailAlreadyExists = "<div class=\"alert alert-danger\" role=\"alert\">Email already exists!</div>";
private static final String incorrectPassword = "<div class=\"alert alert-danger\" role=\"alert\">Incorrect password.</div>";

protected void processRequest(HttpServletRequest request, HttpServletResponse response)
            throws ServletException, IOException
{
    HttpSession session = request.getSession();
    String id_user = (String) session.getAttribute("id_user");
    String password = request.getParameter("password");
    
    if (!Authorization.isCurrentUserAuthorized(password, id_user))
    {
        writeOutput(request, response, incorrectPassword);
        return;
    }
    
    String newEmail = request.getParameter("newEmail");
    if (DatabaseAccess.userAlreadyExistsCheckEmail(newEmail))
    {
        writeOutput(request, response, emailAlreadyExists);
        return;
    }
    
    if (DatabaseAccess.changeEmail(newEmail, id_user))
    {
        //current session must be updated with the new email address
        //otherwise it will contain the wrong address until next login
        session.setAttribute("email", newEmail);
        writeOutput(request, response, emailChanged;
    }
    else
    {
        writeOutput(request, response, emailChangeFailed);
    }
}

private void writeOutput(HttpServletRequest request, HttpServletResponse response, string output)
{
    ServletContext servletContext = this.getServletContext();
    RequestDispatcher rd = servletContext.getRequestDispatcher("/" + GlobalValues.getWebPagesDirectory() + "/" + GlobalValues.getSettingsPage());
    PrintWriter out = response.getWriter();
    out.println(output);
    rd.include(request, response);
}

Also I wasent sure what the string.Format method at the end was for.

I always prefer string.format style methods for building strings. I find it much easier to grok the format of the output and the inputs used to build it.

 

[slugg]

I think the comments are bad. They seem like an excuse to get away with messy code. Three nested if statements seems like a maintenance and testing disaster. Yea, this is simple now, but wait until you write the rest of the software in the same style. The biggest problem here is that you've violated the most important rule of clean code: single responsibility. Your method should do one thing and do it well. This one method validates the user's authorization, validates the user's input, writes the user's input to the database, handles all negative cases for each one, and formats the output. That is way too much in one method.

I think you'll find that if you simplify your method into multiple, smaller methods, you won't need a single comment. There are multitudes of other advantages, too, but they're a bit off topic for now.

 

[IHateMyJob2004]

I usually put any comments for an else block at the top of the else block:

// condition: bla
if {
} else {
// condition: bla
}

It's not in books this way but it is easy to follow.


Always declare variables when they are needed, not at the top. All at the top can only lead to misuse of variables during maintenance. The only reason for having them at the top was back many many many years ago when compilers required it.

YOur variable names are good.

 

[brianmanahan]

extracting some of that code into descriptively-named methods would be more beneficial than comments

 

[beginner99]

I don't feel like any of these comments give any extra information

I also haven't declared all of my variables at the start of a method since college

this.

Comments are needed when

a) what you are doing is complex and not easy to understand

b) the reason you are doing something isn't obvious
This can be due to weird business rules, uncommon or wrong file input formats, errors in 3rd party libs, etc

Simple if-statements like in OP case do not match this. Plus I would try to avoid if statements like

if (!true) ...

way better and easier to read if you always use if (true)...

Yeah, this is not always possible but still a good rule of thumb.

 

[Maximilian]

Ah cool I really like that alternative to the if else nest! Ive always thought they could get a bit complicated, hence my initial comments for them.

Yeah I think you guys are onto something with the multiple smaller methods as well. The method names themselves are a hint of sorts as to what is going on. I remember being taught about this at uni but getting this all working was priority 1 when I wrote it.

I took what you guys have said and refactored the AuthenticationServlet as well. Its used for initial login.

Old code:

protected void processRequest(HttpServletRequest request, HttpServletResponse response)
            throws ServletException, IOException 
    {
        ServletContext sc = this.getServletContext();
        System.out.println("AuthenticationServlet: executing");
        String loginAttemptEmail = request.getParameter("email");
        String loginAttemptPassword = request.getParameter("password");
        Map<String,String> userCredentials;
                
        //obtain user details from database
        userCredentials = (HashMap) DatabaseAccess.getUserCredentialsFromEmail(loginAttemptEmail);
        System.out.println("USER CREDENTIALS: "+userCredentials);
            //if account exists
            if(userCredentials != null)
            {
                System.out.println("AuthenticationServlet: account exists");

                    //if password is correct                   
                    String storedHashedPassword = userCredentials.get("hashedPassword");
                    if(Security.passwordMatch(loginAttemptPassword, storedHashedPassword))
                    {
                        System.out.println("AuthenticationServlet: password correct");
                        
                        //create new session and add email/user_id values to it
                        HttpSession session = request.getSession(true);
                        session.setAttribute(ClientAPI.getClientRequestIdentifier(), loginAttemptEmail);
                        session.setAttribute("id_user", userCredentials.get("id_user"));
                        session.setMaxInactiveInterval(GlobalValues.getMaxInactiveInterval());
                        //String encodedURL = response.encodeRedirectURL(sc.getContextPath() +"/"+ GlobalValues.getWebPagesDirectory() +"/"+ GlobalValues.getMainPage());
                        //response.sendRedirect(sc.getContextPath() +"/"+ GlobalValues.getWebPagesDirectory() +"/"+ GlobalValues.getLoginSuccessReferrer());

                        RequestDispatcher rd;
                        rd = request.getRequestDispatcher("/MainPageServlet");
                        rd.forward(request, response);
                    }
                    else
                    {
                        System.out.println("AuthenticationServlet: wrong password");
                        //if wrong password, back to login page
                        //response.sendRedirect(sc.getContextPath() +"/"+ GlobalValues.getFirstLoginServlet());

                        RequestDispatcher rd = sc.getRequestDispatcher("/"+GlobalValues.getWebPagesDirectory()+ "/" + GlobalValues.getLoginPage());
                        PrintWriter out= response.getWriter();
                        out.println("<div class=\"alert alert-danger\" role=\"alert\">Invalid email or password</div>");
                        rd.include(request, response);
                    }
                

            }
            else
            {
                System.out.println("AuthenticationServlet: account dosent exist");
                //if account dosent exist, back to login page
                //response.sendRedirect(sc.getContextPath() +"/"+ GlobalValues.getFirstLoginServlet());
                
                RequestDispatcher rd = sc.getRequestDispatcher("/"+GlobalValues.getWebPagesDirectory()+ "/" + GlobalValues.getLoginPage());
                PrintWriter out= response.getWriter();
                out.println("<div class=\"alert alert-danger\" role=\"alert\">Invalid email or password</div>");
                rd.include(request, response);
            }

        }

New code:

    private static final String accountDosentExist = "<div class=\"alert alert-danger\" role=\"alert\">Invalid email or password</div>";
    private static final String wrongPassword = "<div class=\"alert alert-danger\" role=\"alert\">Invalid email or password</div>";
    private static final String forwardRequestTo = "/MainPageServlet";

    protected void processRequest(HttpServletRequest request, HttpServletResponse response)
            throws ServletException, IOException
    {
        System.out.println("AuthenticationServlet: executing");
        String loginAttemptEmail = request.getParameter("email");
        Map<String, String> userCredentials = (HashMap) DatabaseAccess.getUserCredentialsFromEmail(loginAttemptEmail);

        if (userCredentials == null)
        {
            System.out.println("AuthenticationServlet: account dosent exist");
            writeOutput(request, response, accountDosentExist);
            return;
        }

        String storedHashedPassword = userCredentials.get("hashedPassword");
        String loginAttemptPassword = request.getParameter("password");
        if (!Security.passwordMatch(loginAttemptPassword, storedHashedPassword))
        {
            System.out.println("AuthenticationServlet: password incorrect");
            writeOutput(request, response, wrongPassword);

        } else
        {
            System.out.println("AuthenticationServlet: password correct");
            createNewSession(request, userCredentials);
            forwardRequest(request, response);
        }
    }

    private void writeOutput(HttpServletRequest request, HttpServletResponse response, String output) throws IOException, ServletException
    {
        ServletContext servletContext = this.getServletContext();
        RequestDispatcher rd = servletContext.getRequestDispatcher("/" + GlobalValues.getWebPagesDirectory() + "/" + GlobalValues.getLoginPage());
        PrintWriter out = response.getWriter();
        out.println(output);
        rd.include(request, response);
    }

    private void createNewSession(HttpServletRequest request, Map userCredentials) throws ServletException, IOException
    {
        HttpSession session = request.getSession(true);
        session.setAttribute(ClientAPI.getClientRequestIdentifier(), userCredentials.get("email"));
        session.setAttribute("id_user", userCredentials.get("id_user"));
        session.setMaxInactiveInterval(GlobalValues.getMaxInactiveInterval());
    }

    private void forwardRequest(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
    {
        RequestDispatcher requestDispatcher = request.getRequestDispatcher(forwardRequestTo);
        requestDispatcher.forward(request, response);
    }

}

 

[Cogman]

Just a note, it looks a bit like you are doing things in "the hard way".

Any reason you are not using JAX-RS?

This

        protected void processRequest(HttpServletRequest request, HttpServletResponse response)
            throws ServletException, IOException 
    {
        ServletContext sc = this.getServletContext();
        System.out.println("AuthenticationServlet: executing");
        String loginAttemptEmail = request.getParameter("email");
        String loginAttemptPassword = request.getParameter("password");
...
                RequestDispatcher rd = sc.getRequestDispatcher("/"+GlobalValues.getWebPagesDirectory()+ "/" + GlobalValues.getLoginPage());
                PrintWriter out= response.getWriter();

becomes this

	@POST
	@Path("/")
	public Response authenticate(@FormParam("email") String email, @FormParam("password") String password)
	{
		System.out.println("AuthenticationServlet: executing");
...
		return Response.temporaryRedirect("/"+GlobalValues.getWebPagesDirectory()+ "/" + GlobalValues.getLoginPage()).build();

Or something similar.

Also, System.out.println is somewhat discouraged, You should instead be using something like Log4J2, SLF4J, or even the java utils logger instead.

I also spy a `DatabaseAccess` class. I can only guess what is going on inside is either some synchronized datasource or the datasource is being recreated with every request.

If it is the first, then that lead to problems if you want to have other DatabaseAccess type modules (Maybe a UserDatabaseAccess vs an AccountDatabaseAccess object, for example). Ideally, your application will have one DataSource behind some sort of connection pool (We are currently testing HikariCP, however, BoneCP is also popular).

If it is the second, I would suggest trying to move away from it. Datasources are expensive to create and making new connections aren't cheap. 200 shared connections can often move though data faster than 2000 single use connections.

Just my two bits.

 

[Maximilian]

@Cogman

Cheers for the advice, I actually tried using JAX-RS initially but I didn't really understand what was going on or anything. I asked about frameworks on here probably ~10-11 months ago and I remember it was fuzzybabybunny that suggested I try coding without a framework. So I did that and I made more progress in a day than I did with a week of trying to figure out JAX-RS. Having made the website without a framework and looking back I do remember some bits of JAX-RS that could have come in handy. I remember JAX-RS didn't make me use servlets directly I could have one class with varied but related functionality and the syntax for it was quite straightforward. I think barebone servlets can do the same thing with the @WebServlet urlpattern annotation but I never toyed with that much.

Ill look into the system.out.println alternatives :thumbsup: I didn't know it was discouraged.

Every DatabaseAccess method gets a connection object by calling DatabaseUtils.getDatabaseConnection() which is the method below. The DatabaseAccess method then closes the Connection, ResultSet and PreparedStatement objects when its finished by calling DatabaseUtils.closeConnections(); There is a connection pool so im not sure if the datasource is recreated with each request or not. From what I remember reading about it months ago the Connection.close() just returns that connection to the pool to be reused its never actually closed completely.

I haven't refactored getDatabaseConnection() or anything I just got it working months ago and left it alone :awe:

public static Connection getDatabaseConnection()
    {       
        InitialContext initialContext = null;
        try
        {
            initialContext = new InitialContext();
        } catch (NamingException ex)
        {
            Logger.getLogger(DatabaseAccess.class.getName()).log(Level.SEVERE, null, ex);
        }
        
        DataSource source = null;
        
        try
        {
            //source = (DataSource) initialContext.lookup("java:/comp/env/jdbc/mydb");
            source = (DataSource) initialContext.lookup("java:/comp/env/jdbc/fitnesstrackerdatabaseJNDI");
        } catch (NamingException ex)
        {
            Logger.getLogger(DatabaseAccess.class.getName()).log(Level.SEVERE, null, ex);
        }
        
        Connection aConnection = null;

        try
        {
            aConnection = source.getConnection();
        } catch (SQLException ex)
        {
            Logger.getLogger(DatabaseAccess.class.getName()).log(Level.SEVERE, null, ex);
        }
        
         return aConnection;
    }

 

[Apathetic]

Another good guideline is to comment on why, not what. What you're doing should almost always be evident from the code itself when you're using good variable and method names. For example

//if user has entered the correct password
if (Authorization.isCurrentUserAuthorized(password, request))

This comment is IMO 100% redundant with the following line of code. All you're doing is taking the verb "authorized" and replacing it with the synonymous phrase "entered the correct password". It isn't much different from

// set foo to five
int foo = 5;

The why something is done isn't always evident, and that's when comments are best. This line for example (after the email change).

session.setAttribute("email", newEmail);

Probably justifies a comment, maybe something like.

// session state isn't refreshed until <condition> so we need to be manually
// update the new email
session.setAttribute("email", newEmail);

More than anything else, the "why" gets lost over time. I can look at the code to figure out WHAT it is doing, but the WHY is usually impossible without some additional comments (especially if you're doing something odd).

Dave

 

[Cogman]

@Cogman

Cheers for the advice, I actually tried using JAX-RS initially but I didn't really understand what was going on or anything. I asked about frameworks on here probably ~10-11 months ago and I remember it was fuzzybabybunny that suggested I try coding without a framework. So I did that and I made more progress in a day than I did with a week of trying to figure out JAX-RS. Having made the website without a framework and looking back I do remember some bits of JAX-RS that could have come in handy. I remember JAX-RS didn't make me use servlets directly I could have one class with varied but related functionality and the syntax for it was quite straightforward. I think barebone servlets can do the same thing with the @WebServlet urlpattern annotation but I never toyed with that much.

Ill look into the system.out.println alternatives :thumbsup: I didn't know it was discouraged.

Every DatabaseAccess method gets a connection object by calling DatabaseUtils.getDatabaseConnection() which is the method below. The DatabaseAccess method then closes the Connection, ResultSet and PreparedStatement objects when its finished by calling DatabaseUtils.closeConnections(); There is a connection pool so im not sure if the datasource is recreated with each request or not. From what I remember reading about it months ago the Connection.close() just returns that connection to the pool to be reused its never actually closed completely.

I haven't refactored getDatabaseConnection() or anything I just got it working months ago and left it alone :awe:

public static Connection getDatabaseConnection()
    {       
        InitialContext initialContext = null;
        try
        {
            initialContext = new InitialContext();
        } catch (NamingException ex)
        {
            Logger.getLogger(DatabaseAccess.class.getName()).log(Level.SEVERE, null, ex);
        }
        
        DataSource source = null;
        
        try
        {
            //source = (DataSource) initialContext.lookup("java:/comp/env/jdbc/mydb");
            source = (DataSource) initialContext.lookup("java:/comp/env/jdbc/fitnesstrackerdatabaseJNDI");
        } catch (NamingException ex)
        {
            Logger.getLogger(DatabaseAccess.class.getName()).log(Level.SEVERE, null, ex);
        }
        
        Connection aConnection = null;

        try
        {
            aConnection = source.getConnection();
        } catch (SQLException ex)
        {
            Logger.getLogger(DatabaseAccess.class.getName()).log(Level.SEVERE, null, ex);
        }
        
         return aConnection;
    }

You are at sort of a level below barebones at this point . JAX-RS is pretty minimal in what it provides. There are a few popular choices, Jersey and RestEASY I believe are the most common options. Beyond that there is stuff like DropWizard and Spring Boot which make getting a JAX-RS environment setup easy.

For the database connection stuff, I would suggest always doing something like this

		try(Connection connection = dataSource.getConnection();
			PreparedStatement statement = connection.prepareStatement("Select * from Thingy");
			ResultSet rs = statement.executeQuery())
		{
			while (rs.next())
			{
				Thingy thing = new Thingy(rs.getInt("id"), rs.getString("name"));
			}
		}
		catch (SQLException ex)
		{

Where dataSource is some already calculated and loaded datasource.

You want to be extra careful with making sure you close out things correctly. Try with resources was introduced in java 7 explicitly to solve that problem (and it is somewhat nice in that fashion).

That being said. Something like Spring's JdbcTemplate or MyBatis is also a good route to go here. But if you want to do it raw, I would strongly suggest using the above pattern instead of trying to do anything else. You will want to pull the connection off of the datasource at the sql call site instead of in some resource to ensure that you properly close the connection after using it.

And just as a general rule of thumb, anything that must be "closed" should be either done in a try with resources block if it is closable or in a finally statement. Doing it anywhere else will almost assuredly result in some sort of connection leak (and those are dreadfully painful to fix, trust me on this).

Also, if you are doing database connections raw, make sure you read and understand the Jdbc javadocs for each method you call. Some drivers follow the standard better than other, some will try to "help" you by doing more for you than they should. I'm currently fighting this in work as we are trying to change from jtds to Microsoft's SQL driver. Jtds was really permissive and programmer fault tolerant. Microsoft's driver demands that you follow the spec to the letter. You can't go wrong if you follow the spec.

If you like. I could create for you a "barebones" java servlet project. I've been doing that a lot recently so I'm pretty familiar with the "right" and wrong ways to do things.

 

[Maximilian]

@Cogman That looks great actually. I can scrub the overloaded closeConnections() method in my DatabaseUtils class and use the try with resources like you've done.

Yeah if you've got time it would be cool to have a look a barebones servlet project :thumbsup:

 

[beginner99]

Cheers for the advice, I actually tried using JAX-RS initially but I didn't really understand what was going on or anything. I asked about frameworks on here probably ~10-11 months ago and I remember it was fuzzybabybunny that suggested I try coding without a framework. So I did that and I made more progress in a day than I did with a week of trying to figure out JAX-RS. Having made the website without a framework and looking back I do remember some bits of JAX-RS that could have come in handy. I remember JAX-RS didn't make me use servlets directly I could have one class with varied but related functionality and the syntax for it was quite straightforward. I think barebone servlets can do the same thing with the @WebServlet urlpattern annotation but I never toyed with that much.

Of course it takes longer if you have to learn the framework first. But you have to do that once and for any next project you already know the framework and profit from it. Going barebones is IMHO just masochistic.

 

[purbeast0]

Of course it takes longer if you have to learn the framework first. But you have to do that once and for any next project you already know the framework and profit from it. Going barebones is IMHO just masochistic.

if there's one thing i've learned in my professional career as a dev, there is absolutely no need to re-invent the wheel. libraries and frameworks exist for a reason and many of them are tried and true.

i'm working on a mobile app right now that uses to many frameworks/libraries/api's that if i were to write all of that stuff myself, it would take years to get things done. but i am using Parse as the backend, Layer as the text messaging service, Quikblox for video service, multiple CocoaPods, and facebook api to login to it all.

 

[Crusty]

if there's one thing i've learned in my professional career as a dev, there is absolutely no need to re-invent the wheel. libraries and frameworks exist for a reason and many of them are tried and true.

i'm working on a mobile app right now that uses to many frameworks/libraries/api's that if i were to write all of that stuff myself, it would take years to get things done. but i am using Parse as the backend, Layer as the text messaging service, Quikblox for video service, multiple CocoaPods, and facebook api to login to it all.

That kind of stuff is a huge double-edged sword IMHO. It's too easy for people to say 'vendor all the things' and forget that they are still indeed trying to solve their businesses direct needs. In the end you could end up with a bunch of ill-fitting libraries and an application that breaks in completely non-intuitive ways if you're not diligent in choosing what code to vendor.

Of course this highly depends on what you are doing and what tools you decided to use.

For example, in the Rails ecosystem, some of the most popular gems for building an application have been around since before Rails 2.0. They have carried around with them all this code to maintain compatibility between Rails 2.0, 3.x, 4.x, 5.x and not to mention the transition to Ruby > 1.9.

A couple of things I do before vendoring any code

• Read the source, at least the interesting parts and test code
• Check out the activity level of the maintainers, responsiveness to bugs/issues
• Compare the complexity of the architecture of the code to the complexity of the problem being solved. See someone rolling their own LinkedList class inside a library? RUN FOR THE HILLS.
• Look for way to subscribe to a release/changelog, how else are you going to know when there's a security bug you need to update for?
• License compatibility, usually a non-issue since I'm not selling software, but always worth checking into