search

sirixdb / sirix

SirixDB is an an embeddable, bitemporal, append-only database system and event store, storing immutable lightweight snapshots. It keeps the full history of each resource. Every commit stores a space-efficient snapshot through structural sharing. It is log-structured and never overwrites data. SirixDB uses a novel page-level versioning approach.
https://sirix.io
BSD 3-Clause "New" or "Revised" License
1.13k stars 252 forks source link

Refactoring according to clean code principles #109

Open JohannesLichtenberger opened 5 years ago

JohannesLichtenberger commented 5 years ago

For instance splitting classes. Maybe also using a dependency injection framework if needed which works at compile time. For instance Dagger2...

JohannesLichtenberger commented 5 years ago

I think that's a never ending story, but for instance the code in the NodeReadOnlyTrx implementations or the abstract class... I guess we could for instance use the command pattern to internally have the commands Insert, Update, Delete... and split the class into manageable units.

BTW: I'm also thinking about using Kotlin together with Java in the bundles/modules. The REST-API for instance has been implemented with pure Kotlin and I love it. Basically almost all the items from Effective Java can be done in a much cleaner way :-) so maybe first adding Kotlin to the XQuery bundle and see how if it works... and then the core. It would also make some APIs cleaner.

JohannesLichtenberger commented 5 years ago

Yeah, I think you could have a look at the aforementioned interface and the implementing classes. I'm using my tablet right now, but I think the code shared amongst the JSON and XML transaction in an abstract class was huge with more than 1000 lines of code. I'd probably split this internally into Command classes. What do you think?

JohannesLichtenberger commented 5 years ago

So, for the #Hacktoberfest you don't have to refactor everything, that would be a bit crazy or at least too much work!? ;-)

JohannesLichtenberger commented 5 years ago

Oh and additionally, one of the sad things is that I wanted to start writing more unit tests. Most of the tests are integration tests (still stems from the university project years ago), so it's hard to tell in the first place what exactly broke when something breaks :-/

BTW: If you need dependency injection with a tool, you could probably use Dagger2 as a compile time dependency injection framework.

JohannesLichtenberger commented 5 years ago

Yup @JohannesLichtenberger. My intention is to not just contribute during hacktoberfest. So if stretches past October it's not an issue since I am looking to contribute for long term and make incremental changes periodically.

Wow, thanks :-)

JohannesLichtenberger commented 5 years ago

@fullstackguyhere guess you're not interested anymore?

Rishavgupta12345 commented 3 years ago

can i work on this ?

JohannesLichtenberger commented 3 years ago

@Rishavgupta12345 sure, give it a try :)

artwo commented 2 years ago

Hi @JohannesLichtenberger what is the scope of this refactor? Is there a list of classes, packages or specific areas? Or is the refactor in the entire code base? I would like to contribute to this issue, but its description is not very detailed and it could lead to an endless amount of work.

artwo commented 2 years ago

Oh and additionally, one of the sad things is that I wanted to start writing more unit tests. Most of the tests are integration tests (still stems from the university project years ago), so it's hard to tell in the first place what exactly broke when something breaks :-/

I could recommend to create different issues with specific goals in mind. For example, increase the amount of unit tests for a given flow. In this way there is a clear objective to follow and it also guides the contributor on what to do.

JohannesLichtenberger commented 2 years ago

I think the worst is that the document index trie is scattered over multiple classes (NodePageReadOnlyTrx, the different pages in the according layer which have methods as the CASPage::createCASIndexTree to create one initial layer index tree with an indirect page and the TreeModifierImpl. I think as with the other layers, the page read/write layer, the I/O layer... we should simply have a DocumentIndexTrieReader and a `DocumentIndexTrieWriter which gets a reader as a delegate.

Furthermore, I guess we should split the internals of the JsonNodeReadOnlyTrx and the JsonNodeTrx and delegate to other classes, to reduce to number of lines in the classes...

Other than that I'm now not that unhappy to have not that many real unit tests, because such refactoring can take place without changing a lot of tests usually. I've heared that one of the DuckDB guys in a talk also mentioned that this is a plus. But of course they have much more integration tests.

omkar-shitole commented 2 months ago

Hey, I have raised a PR related to this issue in which I have refactored and made the SirixVerticle class more readable and maintainable which will be ultimately helpful for easier future modifications and also will help in easily understanding request and response handling related to HTTP server.

You may find a more detailed explanation regarding the changes in the PR description here.

https://github.com/sirixdb/sirix/pull/740#issue-2504214438