Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract database module #1371

Merged
merged 9 commits into from
May 15, 2023
Merged

Conversation

GianpaMX
Copy link
Contributor

@GianpaMX GianpaMX commented Jul 28, 2022

Resolves #1370 by extracting the database code into a separate gradle module

                       ┌─────┐
                       │ app │
                       └──┬──┘
                          │
        ┌─────────────────┼──────────────────────────┐
        │                 │                          │
        ▼                 ▼                          ▼
  ┌──────────┐   ┌───────────────────┐   ┌────────────────────┐
  │ database │   │ icon-pack-classic │   │ icon-pack-material │
  └─────┬────┘   └───────────────────┘   └────────────────────┘
        │
        ▼
   ┌────────┐
   │ crypto │
   └────────┘

There few tricks I had to do to extract the code:
IconPackChooser. I had to extract an interface (InterfaceIconPackChooser open to a better name suggestion) so the database module can depend on the interface but not on the implementation that requires values from the app BuildConfig

Exceptions. Instead of passing a string id, I just identified the exception class and linked it manually

Templates. Default name, fields and title are passed as strings instead of res ids

In a different PR

  • publish crypto module in maven repo
  • publish database module in maven repo

KeePassDX development can continue with the proposed structure and it should be familiar to any other developer

@GianpaMX GianpaMX marked this pull request as ready for review August 3, 2022 18:39
@J-Jamet J-Jamet added this to In progress in 5.0.0 via automation Aug 5, 2022
@J-Jamet J-Jamet removed this from In progress in 5.0.0 Aug 8, 2022
@J-Jamet J-Jamet added this to In progress in 4.0.0 via automation Aug 8, 2022
@GianpaMX GianpaMX changed the base branch from master to develop October 27, 2022 16:29
@J-Jamet
Copy link
Member

J-Jamet commented May 8, 2023

OK, I have done a first check pass, there are things to improve:

  • The database module must be self-sufficient
  • It is sufficient to import only the database module into the app module. The crypto module is only known from database.
  • The database module must not have access to the PreferenceManager
  • The database module should be seen as a minimalist library, no unused imports
  • Utility classes should not be in multiple locations
  • App.Database package contains room db to manage URI so nothing to do with database module
    I am improving the code in the feature/Module_Refactoring

@J-Jamet
Copy link
Member

J-Jamet commented May 14, 2023

I am refactoring and studying your code, there is one rule: never duplicate the code. You did it several times for the utilities. There is always a way to encapsulate without duplicating.

@J-Jamet
Copy link
Member

J-Jamet commented May 14, 2023

I made only one enum for CompressionAlgorithm to avoid mapping

@J-Jamet J-Jamet merged commit 48cc8b3 into Kunzisoft:develop May 15, 2023
4.0.0 automation moved this from In progress to Done May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
4.0.0
Done
Development

Successfully merging this pull request may close these issues.

Extract database code into a separate library
2 participants