A path to improve code quality

[alexgarel]

What

Following up on #2361 I propose a path to improve code quality on at least a first side, quite systematic.

We may discuss on this issue and the create sub-tickets

Why

See #2361. Main point is to make code testability and transformation more easy, and allow new developer to more easily contribute.

Proposed methodology

1. in .pm files: identify every call to param and replace by a field in request ref (this avoid having apache reference deep inside the code, which make thinks harder to identify and to refactor). Also avoid using directly add headers or that kind of http related stuff and put it in response ref instead, then process at the end.

2. initialization of request_ref should do all HTTP related stuff and put them in meaningful structure (or a series of specific sub for specific cases)

3. incrementally replace a lot of hashmaps structuring the code with objects:

   • I'm not sure if we should use blessed references or use an object framework
   • this will help document structure used (currently it really obfuscate the code)
   • it should use typed parameters, eventually with checks (if it does not hinder performance too much)
   • use composition when possible (object containing object)
   • it first keep current hashmap structure (when possible), but in the future can be refactored
   • for all stuff that we want to keep flexible, isolate them in a extended field containing a free hashmap (but use it only for temporary / experimental stuff or some edge cases)
   • this should also apply to product hashmap, but we shall keep it for the last point, with a compatibility layer

4. refactor the existing code in smaller chunks, trying to respect single responsibility principle and testability. To do it progressively:

   • add a comment #TODO: refactor to MODULE::function_name on group of lines. Function name should really be choosen with care, and module also (it maybe a new module). We can do it progressively
   • discuss it with other developers as we get enough to discuss
   • see if code already covered by tests in some way, if not add tests to the parent method (before refactoring)
   • refactor, maybe with the help of a LLM

[alexgarel]

@hangy @john-gom @stephanegigandet @zigouras @yuktea what do you think ?

[john-gom]

Hi @alexgarel . I would personally like to see a more long-term vision of how we want the code to be structured in the future, e.g. how we might partition the code into multiple, smaller services. I know we can't achieve this quickly, and activities like the ones you suggest are likely to be part of the path to getting there, but it is helpful to know the ultimate goal to be sure that the work we are doing is heading in the right direction and not down a side path.

[stephanegigandet]

@alexgarel Thanks for writing this down. It makes a lot of sense. Regarding the objects, I'm curious to see how well it can work with things like products (if we want to decompose them in smaller objects like ingredients lists, ingredients, nutrition facts, scores etc.), and then their recomposition/decomposition when we want to save/load them to/from MongoDB, JSON, disk etc.
Maybe we could try using objects first with simpler objects like users and organizations.

@john-gom I'm a bit scared of the multiple smaller services approach. Today's monolithic approach has the benefit that all products updates / analysis (e.g. analyzing ingredients, computing scores etc.) is synchronous and fast. There's only one state for products: the analysis is always complete. We can update the 3M products relatively easily (e.g. there's an ingredient taxonomy change, we recompute ingredient analysis, ecoscore, nutriscore etc. on all products).

If we have separate micro services, and we still run them in a synchronous way with a defined order with a reasonnable performance cost (e.g. re-analyising 3M products does not take twice the time), then it's fine I think. But I would be very afraid of going in a direction where products can be partially updated, with for instance updated ingredients and nutrition facts but a score or analysis that would still be pending.

[john-gom]

Hi @stephanegigandet , I recognise your concerns, but I think we do need to do something to address the size of the codebase in the server repository. Everything is taking much too long, which significantly impacts on productivity / willingness to contribute and makes it very difficult to adopt modern practices like TDD.

It is exactly because of those concerns that you have that I think it is important that we have a discussion about which way we should go in terms of a longer-term strategy, as otherwise people might make assumptions...

[john-gom]

Hi @stephanegigandet , @alexgarel I've started dumping my thoughts on future architecture here: https://docs.google.com/document/d/1I0ld47BwRaLg7ThNMrLSbKbgqygMWB0OdYfCI49Vp3g/edit?usp=sharing

Please feel free to add your own thoughts, pros, cons, etc.

[stephanegigandet]

I've started dumping my thoughts on future architecture here: https://docs.google.com/document/d/1I0ld47BwRaLg7ThNMrLSbKbgqygMWB0OdYfCI49Vp3g/edit?usp=sharing

Wonderful, thank you @john-gom !

[alexgarel]

@stephanegigandet about objects, I think I did not understand object in perl indeed. Because what I was seeking is enforcing / documenting the attributes names. And apart from using accessor (which is may slow down quite a lot and is a bit verbose in the code) there is no way to enforce attributes in blessed perl hash. Maybe we should use Class::Struct ? but what is the perf penalty ?

[github-actions[bot]]

This issue has been open 90 days with no activity. Can you give it a little love by linking it to a parent issue, adding relevant labels and projets, creating a mockup if applicable, adding code pointers from https://github.com/openfoodfacts/openfoodfacts-server/blob/main/.github/labeler.yml, giving it a priority, editing the original issue to have a more comprehensive description… Thank you very much for your contribution to 🍊 Open Food Facts