On the upside of your code, you kept it as simple as possible.
But there are a few flaws - and the upside doesn't come without consequences.
What's also good is that you already use PreparedStatement
.
Security Risk
Hard-coding the username and password is not a good idea for another reason.
A misconfigured webserver could easily expose these critical credentials by accident.
Layered Architecture
We say that often, an architecture consists of 3+1 layers: UI, Business Logic / App, Persistence, plus Main.
In your case this is all mixed up.
The upside is that in your case it keeps things seemingly simple.
But this is a trap.
Someone needs to change the source code of the application in order to connect with a different password, different username, or to use a different SQL RDBMS than MySQL, or to use a different type of DBMS altogether.
ID Collisions likely
The way how you get your ID means that ID collisions are quite likely.
There are better ways to generate a unique ID.
In case of MySQL, and most other RDBMS for that matter, you can actually let the DBMS generate an ID for you.
In MySQL, the id column would need to be defined like this: id INT PRIMARY KEY AUTO_INCREMENT
.
Usually, we say that using auto-incremental IDs is a security issue.
However, if pastes are public anyway, that's a different topic.
The pastes are not access-protected anyway.
So incremental IDs are fine in this case.
Otherwise, universally unique ids or hashes with unlikely collisions are far better user ids. One might still want to go for id INT PRIMARY KEY AUTO_INCREMENT
hidden in the DBMS for relations and use a uuid VARCHAR(...) UNIQUE
for the id visible outside the DBMS.
Error handling
You want to close your pstmt
even in case of error, and you want to close your con
in any case.
You can use Java 7 try
-with-resources for that:
try (final Connection con = DriverManager.getConnection(dbUrl, dbUsername, dbPassword)) {
try (final PreparedStatement pstmt = con.prepareStatement("INSERT INTO paste_tool(gen_key, title, source_codes) VALUES(?, ?, ?)")) {
// ...
}
}
This will close your PreparedStatement
and your Connection
automatically, regardless of whether they were successful or threw exceptions.
Code Conventions
In Java, we only use _
in names in these rare cases:
- The name implements an API defined elsewhere and there the name already contains
_
.
- The name is a compile time constant, as in
public static final short MAX = 32767;
- The
_
replaces a comma in natural language, as in givenConnectedChatClient_whenSendingMessage_thenReceivesMessage()
.
In your case, you should rename things:
gen_key
-> genKey
submit_code1
-> submitCode1
title_code
-> titleCode
Why is it named submit_code1
anyway? Is there also non-submitted code? Is there also a submit_code0
? A submit_code2
? And why is it named submit_code1
, when in the form it is named paste_code
?
I think it that submitCode1
should actually be named pasteCode
.
And submitCode2
should be renamed to htmlEscapedPasteCode
- or removed, see next point.
When to replace the special characters
You replace the special characters in the paste code before storing the paste in the DBMS.
While this gives you a little bit of performance - you have to replace only once, upon storage, not many times upon each retrieval, I think it's not the best way how to handle it.
Consider you want to extend this with different interfaces.
Maybe you want to implement raw retrieval, or PDF export, or the facility that a user can fetch all his pastes as one big zip file?
Then you'd have to reverse the replacement of the special characters.
From an architecture perspective, the special characters are a presentation topic - it's because you use HTML as presentation.
Therefore it should be in the presentation layer, not in the persistence layer.
If you then add more presentation modules, they will not have to reverse presentations of other modules that are otherwise unrelated to them.
It would look awkward in a raw module or PDF module to first have to reverse the HTML special characters from what they get from the database.
So, I'd put the StringEscapeUtils.escapeHtml4()
call in viewCode.jsp
instead.
Error Handling 2
What happens in case of an ID collision (gen_key
already used)?
Error Handling 3
What happens in case the form is submitted empty?
ISO-8859-1 vs. UTF-8
I'd really use UTF-8. Always. Everywhere. Unless there's a really good reason not to. And I don't think that here there is.
ASP-style JSPs vs. XML-style JSPs
I'd rather go for XML-style JSPs than ASP-style JSPs.
For many it may just be a matter of taste.
But to me there's the advantage that I can XSLT process the source of the JSP.