Programming best practices

[zigouras]

After fixing a number of bugs in the Product Opener server code, I have been exposed to much of the Perl code base. I have enjoyed the opportunity to get to know the team and to write code to contribute to the project. However, I see a number of issues with the code as it exists now.

Here is a list of the some the things I have noticed:

• Functions are too large and complex. Methods are often hundreds of lines of code in length and have large cyclomatic complexity with deeply nested conditional statements. This makes code difficult to read and understand, increases the risk of bugs, lessens code reuse, limits testing and makes debugging difficult. A good rule of thumb for method size is 15 lines.
• Object Oriented programming is not used. Although there is a library of Perl .pm ’modules’ in the ProductOpener namespace, they only contain procedural functions. There are a great many advantages to OO code, I won’t go into them all here.
  A good book to get your feet wet on OO would be:
  Head First Object-Oriented Analysis and Design
  The classic text on OO which is always worth reading is:
  Design Patterns: Elements of Reusable Object-Oriented Software

Perl 5 has had OO support for a while, but it is a fairly weak implementation. It looks like the Moose extension is the current standard for object oriented Perl, see more below.

• HTML and Javascript are directly generated in Perl code. This makes maintenance difficult for both server and client code. There are a number of ways to address this, e.g. templating as in HTML::Template

• As APIs become more popular with patterns like micro services, consider building business logic first as JSON REST services. This will automatically provide APIs for users and then you can further decouple your front end and business logic by using a pure JavaScript front end like Angular, React, TypeScript to consume those same services.

• Code documentation is lacking, both exportable at the class and method level and inline within the code. See issue Document ProductOpener Perl modules. #2203

• The advantages to implementing the above:

  • Reduced risk of occurrence of new or regression bugs.
  • Easier debugging of problems.
  • Quicker development time
  • Improved testability
  • Code reusability
  • Improved performance
  • Lower cylomatic complexity

Ways to implement these changes:

• Perl code should be written as object oriented with the Moose project. This will immediately realize all of the advantages listed above. Moose improves upon standard Perl OO by introducing type checking, custom types, and many other features. While I have not used Moose myself it looks like it is the latest preferred approach to OO Perl.
• Document code, e.g. with Perl POD as you go along while coding. Perl POD can be automatically converted to HTML and put online.
• Write tests for all code. The test coverage in OFF is decent ( better than many projects I have worked on) but it should be a priority.
• Consider a new architecture: micro-service with pure JS front ends, etc.
• Consider a more modern language. Java, Node, Python etc. would be options if we started this project today.

I understand the OFF is a large legacy project that was started long ago when many of these technologies did not exist and Perl was the best choice for web applications. With the upcoming development of the self-service platform by Stephane, now might be a good time to consider implementing some of these ideas to secure a healthy future for OFF.

[aleene]

I guess a big holdback in adopting and improving code is the (lack of) manpower.

[teolemon]

Thank you very much for the deep-dive @zigouras 👍

• I'm not the expert, but templatization of the web frontend would be a very useful first step so that we can modernize the design, and prepare for a proper decoupling of the Front and the Back on the web site.
• For testing, the focus has been so far on testing for regressions in ingredient parsing and normalization. We should try to list where testing will have the most impact

[stephanegigandet]

Hi @zigouras, thanks a lot for your observations, comments and suggestions. I definitely agree with what you said. The big question I have is: how can we do all the refactoring incrementally, so that we can continue the development of new features and limit risks introduced by big changes.

For some issues like "functions too large and complex", the good thing is that we can address them one by one, break the big functions into smaller ones, add tests for them etc.

OO programming: maybe we can find a way to do it gradually too. e.g. creating oo objects and methods for some small things (e.g. user management) first.

[CharlesNepote]

Thanks a lot @zigouras, I agree with many of your suggestions but not all. Disclaimer: I'm not an engineer and have learnt computer science by myself.

Following @teolemon, I think templatization would be the very first step, to allow contributions from developers that don't play well with Perl.

About OO code, I know it has advantages but I think development is more complex: you have to understand well OO programming -- and the learning curve of Moose seems quite long.

Also, I can be wrong but I don't like pure JavaScript front end which introduce many drawbacks:

• SEO and indexing contents
• makes development more complex; you have to learn a bunch of (heavy) new technologies which change often (today Angular, Vue, Ract, tomorrow NextJS, and so on) -- one thing I love so much in Perl is that my old scripts from 1990' are still working! while not my PHP ones
• network requests makes things slower

[zigouras]

@stephanegigandet I understand the risk of touching old code. We might therefore consider these changes first for new code only, e.g. if we are adding new functionality, try to implement some of these ideas.
Thanks for all the feedback guys. I just wanted to throw some ideas out there to think about. I realize all of the work that went into the code base. The code does work fine as is, it is just that we want to improve it if it is feasible at this time.

[hangy]

First of all, I want to say that what Stephane has done for OFF is amazing. Please don't take anything in this discussion as an offense or personal attack. At least from my point of view, it's just some constructive criticism with the goal of making OFF even better than it already is. 🙂

• As APIs become more popular with patterns like micro services, consider building business logic first as JSON REST services. This will automatically provide APIs for users and then you can further decouple your front end and business logic by using a pure JavaScript front end like Angular, React, TypeScript to consume those same services.

I'm not sure if we really need microservices. At least, I'm not sure how I'd cut responsibility of the microservices right now, because there's not that much "business logic" in OFF (no shipping, billing, ... services). What might be a good candidate for a separate API could actually be stuff like user management and authentication.

In general, I do like the idea of API-first/API as a product. If the API was the first-class product which the off.org website was built on, there could be very a good feature parity for different platforms, since everything would be available in the API(s).

I understand the OFF is a large legacy project that was started long ago when many of these technologies did not exist and Perl was the best choice for web applications.

I believe OFF was launched in 2012, so other popular technologies did exist. As it was launched as a one-man-show by @stephanegigandet, he used what he knew best. The choice might not be ideal for scalability (in terms of getting other coders to contribute) nowadays, but the decision to know the tools that you know (and base the project on something you already have and know to work with) is completely valid and understandable.

I guess a big holdback in adopting and improving code is the (lack of) manpower.

That's very true. Unfortunately, since Perl isn't the most "hip" language and isn't part of some cool software stack, the current situation also somewhat limits the possible gain of manpower.

• I'm not the expert, but templatization of the web frontend would be a very useful first step so that we can modernize the design, and prepare for a proper decoupling of the Front and the Back on the web site.

ACK, even though I think that implementing #80 and separating content from presentation could lead to a better developer experience in general (see below).

Following @teolemon, I think templatization would be the very first step, to allow contributions from developers that don't play well with Perl.

Also with people that do work well with Perl, because a templating system that know that it will render HTML can help avoid common pitfalls (ie. incorrect encoding of entities). 🙂

About OO code, I know it has advantages but I think development is more complex: you have to understand well OO programming -- and the learning curve of Moose seems quite long.

I have no experience with Moose, but it's definitely true that good OOP can be difficult. Having classes and objects in a project doesn't make it OOP. However, well-made OOP (ex. separation of concerns) can help developer productivity (in the long run).

Also, I can be wrong but I don't like pure JavaScript front end which introduce many drawbacks:

• SEO and indexing contents

True, there might be a downside, but apparently, there are options to work around that. Also, search engines quite often prefer prose over lists of links, anyways. Some well placed-links inside of editorial content are advantageous, but websites that have a high link-to-text ratio tend to not do very well regarding SEO. (I'm mainly thinking about Freebase and Wikidata.) Point in case: When I search site:openfoodfacts.org, android.off.org ranks before fr.blog.openfoodfacts.org and be.openfoodfacts.org (in that order). I guess, I simply wonder if SEO needs to be a priority of OFF right now. 🙂

• makes development more complex; you have to learn a bunch of (heavy) new technologies which change often (today Angular, Vue, Ract, tomorrow NextJS, and so on)

Really depends on your point of view. Yes: For people that haven't done SPAs with JS, yet, there's a learning curve. (Me, too! [at least for production use]) For others, however, those technologies are more popular than server-side rendered HTML - especially with Perl. That means, with a working SPA, it might be easier to find web developers who might like to improve the design or usability of the website, since they would not have to learn Perl (which people rarely do today). With a good PWA, separate development of Android/iOS apps might some day be made redundant, which could be another advantage.

• network requests makes things slower

JS/CSS can be bundled quite nicely nowadays, so that the upfront HTTP requests for SPAs should not be too huge. That content can be cached very well. Apart from that, there might just be some REST requests for the products or product lists, which don't differ much from the current website. If anything, it might result in smaller and faster web requests, because the REST API would only need to return some JSON for the requested content, and wouldn't have to produce some (sometimes redundant for the responsive web) HTML for display purposes.

[zigouras]

There are many string literals everywhere in the code, e.g. tag names. If we could formalize the management of those in a data structure we could eliminate the chance of bugs from typos. e.g.

line 10 $product->{categories}->{unhealthy} = 1;
...
line 20 if ( defined $product->{categories}->{unheelthy} ) { # TYPO! }

[github-actions]

Stale issue message