... thank you for this post Arnold... I see that my favorite class (Loan)
grew from 6K LOC the last time I checked (which was not that long ago)...
;-)
I would even add another rule to what you said: if it's longer than 500 LOC
then something is wrong... ok, we know that real life is sometimes not that
binary, but as a rule of thumb I find that number helpful... anything that
is longer than that is really hard to digest. I am pretty sure if we look
closely at the details we'll see certain patterns emerging... there are
certainly a lot of if-then-elses... and each of those would be for me
something worth looking into for similarities and move them to separate
classes (e. g. loan schedule "strategies" in multiple classes instead of if
then elses in one service function). But this kind of refactoring comes
maybe last...
There's another code pattern that I think is responsible for a big chunk of
repetitive code: I think at one point the idea was to have immutable
classes/objects (e. g. data classes where attributes can only be set when
passing constructor values)... now, I don't want to dismiss this
entirely... I can see what the motivation for this is... but the rest of
the code requires us more often than not to copy the whole objects to
change a value; let's ignore the memory implications of that for a
moment... the "copy strategies" in Fineract are various... sometimes it's a
utility function inside the class itself, sometimes in a service, sometimes
the function is static... and so on... this kind of code alone doesn't make
it very easy to read the code; you are constantly side-tracked by this and
sent down some rabbit hole that in the could be a simple
"something.setValue(...)". I would let go of this and instead put Lombok in
place with access to all with proper getters and setters. This is very
simple and will help later (see next paragraph).
Mapping between classes is another topic here that is - my bet -
responsible for at least 20% of avoidable code. There are 2 kinds of
mappings in Fineract that are currently happening:
- JSON mapping: there are some historic reasons why it's done the way we
do it currently, mainly because of some irregular JSON structures that were
hard to map automatically when Mifos/Fineract started. There are certain
repeating patterns with helper classes (which is good), but occasionally we
circumvented those and just did "open heart surgery" where we needed it.
- The first issue I found when I found when I was investigating the
dependencies between "modules" (well, it's more like packages at the
moment) was that in the business logic classes you have
references e. g. to
"org.apache.fineract.infrastructure.core.api.JsonQuery" which indicates
that "belongs" to the REST API layer; down the trenches (read: in service
classes) we should have all our ducks in a row, means everything properly
mapped to domain classes.
- We do a lot (if not all the mapping) with GSON; let's say, it's not
the fastest kid in town... and not the most convenient. We were also not
really consistent with its usage... somewhere we define quite a bunch of
modules that help GSON de-/serializing certain non-standard structures
(like Joda Time etc.); we should have done that too for our data
structures
(data classes) instead of the JSON helpers we created. It's just too
tempting to use the JSON helpers outside of their proper scope (which is
the REST API layer).
- Fineract in the end is a Spring/-Boot application which means we
should replace GSON with Jackson (which is also faster and better
integrated with Spring... again, less code). Anything that Jackson is not
capable of to digest immediately (generic types come to mind) can be
"fixed" with proper (Jackson) APIs (e. g.
"com.fasterxml.jackson.databind.JsonDeserializer" and
"com.fasterxml.jackson.databind.JsonSerializer"); these things are
automatically managed by Spring, no manual anything. And we have exactly
one place where the magic happens instead of interfering with it
constantly
in the business logic.
- data/domain mapping: those structures are often very similar, but
because of the immutability thing I mentioned earlier it is very hard to
translate back and forth. Let's imagine for a moment we had Lombok in
place... then we could use Mapstruct (https://mapstruct.org) to generate
all the code for getting and setting values for us; there would be still
code, BUT:
- it's generated (read: you don't have to maintain it)
- structured 100% the same way (no code style differences)
- debug friendly, because "real" code is generated (you can actually
place breakpoints in Mapstruct's mappers)
The beauty of this approach is that we'll have immediately 2-way mapping;
in our current approach (I am paraphrasing) we have one helper function to
map from data to domain and another helper function from domain to data...
and there's hardly any reusable code in that area... so everyone who
introduces something new has to do this over and over again. With Mapstruct
you define in the best case an annotated simple Java interface and have 2
annotated method definitions. Even in more complex scenarios (that we will
have) it's a very structured approach and most importantly: things stay in
their place and are not moving everywhere.
Again... my bet... mapping will make 20% of the code base disappear.
These would be really worth attacking... I'm not saying easy, but it would
make life so much easier. And I am convinced we could do it in a way to
make it REST API backwards compatible (obviously internally Java APIs need
to change for this to happen).
Cheers,
Aleks
On Wed, Apr 6, 2022 at 10:20 AM Arnold Galovics <arn...@apache.org> wrote:
> Dear Fineract community,
>
> I've been thinking a lot about incoming PRs lately and how a lot of
> functionality is implemented around Loan management.
>
> One of the hardest things about Fineract's maintainability is the amount
> of code it includes. I previously proposed Lombok to reduce the boilerplate
> code therefore reducing the size of baggage we're carrying around; but
> that's just one step.
>
> Another thing we have to be really careful with is how to increase (or at
> least keep) the maintainability of the system while still adding new
> functionality to it.
> As I said in the beginning, Loan management. It's vital to Fineract and a
> lot of code is there to support all the use-cases.
> Just to give you some rough idea around LOC for some classes:
> - Loan class, ~7000 lines
> - LoanWritePlatformServiceJpaRepositoryImpl, ~3500 lines
> - LoanApplicationWritePlatformServiceJpaRepositoryImpl, ~1800 lines
> And this was just the tip of the iceberg.
>
> There's a lot more functionality to come. I feel like if those go into
> these classes, especially into the Loan class, what's already difficult to
> maintain will be impossible to maintain.
>
> I propose to restrict ourselves from adding more functionality to the Loan
> class (and to other classes which are just too big) and extracting those
> new functionalities into new, small classes which will be easy to deal
> with.
> And in the meantime, we can start breaking down these huge classes as well
> into smaller pieces without the fear that it'll grow faster than we could
> handle.
>
> Slowly but surely we can achieve an easily maintained, (mostly) bug free
> and most importantly extensible system that will bring joy not only into
> the life of financial institutions, but to the developers. ;-)
>
> Let me know your thoughts.
>
> Best,
> Arnold
>
