Refactor shared package into the architecture #3523
Conversation
| .putAllDBMSConnectionProperties(properties); | ||
| DatabaseSynchronizer synchronizer = context.getDBMSSynchronizer(); | ||
| if (synchronizer instanceof DBMSSynchronizer) { | ||
| DatabaseConnectionProperties properties = ((DBMSSynchronizer) synchronizer).getDBProcessor() |
There was a problem hiding this comment.
If you add getDBMSConnectionProperties to the Synchronizer interface, then you probably don't need the if statement and the explicit cast here.
There was a problem hiding this comment.
Thanks, nicely spotted! So far, I was focused on the refactoring only, but the code certainly has a lot of potential for improvements. Implementing your suggestion was fairly easy and I fixed this at a second position as well.
|
This refs #110. I am still planning to work on that as I want to provide our logic package as standalone library. |
|
@koppor Ah, I was looking for that issue :) Maybe we should re-open it, since we now have a tangible use case (the new Java module system). Once the model package is modularized, the logic package will not be far off. |
|
@JabRef/developers With another review, this could be merged. |
|
|
||
| import org.apache.commons.logging.Log; | ||
| import org.apache.commons.logging.LogFactory; | ||
|
|
||
| /** | ||
| * Keeps all essential data for establishing a new connection to a DBMS using {@link DBMSConnection}. | ||
| */ | ||
| public class DBMSConnectionProperties { | ||
| public class DBMSConnectionProperties implements DatabaseConnectionProperties { |
There was a problem hiding this comment.
I only don't get this, isn't this supposed to be same or are DataBaseConnectionProperties more general?
There was a problem hiding this comment.
DBMSConnectionProperties are the only implementation of DatabaseConnectionProperties, they're essentially the same. The sole reason for the existence of the interface is that it breaks the dependency from model to elsewhere.
|
|
||
| void registerListener(Object listener); | ||
|
|
||
| void openSharedDatabase(DatabaseConnection connection) throws DatabaseNotSupportedException, SQLException; |
There was a problem hiding this comment.
I would let this rather be more general, by not having the concrete SQLException here. I could implement this interface for my Sharelatex Connection.
There was a problem hiding this comment.
Ok, I've inlined the SQLException in favor of an IllegalStateException.
* upstream/master: Refactor shared package into the architecture (#3523)
The new Java 9 module system brings us the possibility to actually modularize inside Java, i.e. to distribute our source code into multiple modules and specify dependencies and constraints among them, much in the way that we do it now with tests. But to extract modules, there can be no cyclic dependencies among them.
The
modelpackage is our best candidate for extracting a new module. Here the last dependency that coupled it to transitively to everything else was thesharedpackage. This PR refactors the shared package and puts most of it intologic. I broke the dependency frommodelby introducing a number of interface that thelogiclayer implements. These interfaces will have just one implementation now, but at least they break the dependency cycle.This is just a refactoring, so I think it should be merged after the release of 4.1
gradle localizationUpdate?