Refactors database code #987
Conversation
simonharrer
added
cleanup-ops
status: ready-for-review
| output(Localization.lang("No entries or multiple entries selected.")); | ||
| return; | ||
| } | ||
| JabRefExecutorService.INSTANCE.execute((Runnable) () -> { |
There was a problem hiding this comment.
You are using IntelliJ(?), right? I'm wondering if the settings there and Eclipse's are the same? I think I formatted that complete block when I introduced the lambda... But not fully sure.
There was a problem hiding this comment.
Yes, I am an IntelliJ addict. Formatting seems to differ. I reverted it.
|
Looks good! |
| this.database = database; | ||
| } | ||
|
|
||
| public String getServerType() { |
There was a problem hiding this comment.
I think it is better to change the return type to DatabaseType instead of the string value.
| public String getCreateTableSQL(Table table) { | ||
| switch (table) { | ||
|
|
||
| case JABREF_DATABASE: |
There was a problem hiding this comment.
At this point I would suggest better creating a method for each case
e.g. createhabRefDatabase
That would better follow the principle of a single purpose method and additionally reduces the length of the method. Furthermore better readability
There was a problem hiding this comment.
Hm, to me, the method is still quite simple despite being long in lines as it comes with almost no complexity. Hence, I would not gain that much by splitting it up. From my perspective, I would have the hassle of maintaining 9 methods, which have to be put in the interface, and I cannot iterate over these 9 methods in the caller (which is currently done as I can iterate over the Table enum to call the method for each table).
|
In principle LGTM |
# Conflicts: # src/main/java/net/sf/jabref/exporter/MySQLExport.java # src/main/java/net/sf/jabref/exporter/PostgreSQLExport.java # src/main/java/net/sf/jabref/sql/exporter/DatabaseExporter.java
| Connection conn = connect(url, dbStrings.getDbPreferences().getUsername(), dbStrings.getPassword()); | ||
|
|
||
| SQLUtil.processQuery(conn, "CREATE DATABASE IF NOT EXISTS `" + dbStrings.getDbPreferences().getDatabase() + '`'); | ||
| conn.setCatalog(dbStrings.getDbPreferences().getDatabase()); |
There was a problem hiding this comment.
Just one thing I got in mind while looking through an old PR I created a while ago:
There is a potential resource leak in here:
If the Create Database query fails and throws an SQLException, the connection does not get closed and remains open.
That was one thing I fixed in my old PR #910 because it was discovered from coverity amalysis.
My solution was to catch the exception and close the conn. Then re throw the ex.
https://github.com/Siedlerchr/jabref/blob/reworkedCoverityFixes/src/main/java/net/sf/jabref/sql/exporter/MySQLExporter.java
There was a problem hiding this comment.
Hm, you are talking about managing the connection itself. I am not sure if we do handle the connection resource at all, mostly only the statements and result sets.
|
Can you merge this in? |
|
Hm, I am not completely sure this introduces some bugs. Should I wait for directly after the release? |
…xpected sql file is used.
Refactoring of the database SQL stuff.
MySQLand thePostgreSQLclass and can be accessed, both implement theDatabaseinterface which defines all DB specific code.There is still a lot to do in that package, but this should be a good start, I think.