Coding Crimes

This document provides a few guidelines to programmers in the form of examples of what should not be done.

Never hide exceptions which may lead to system corruption


Never hide exceptions to the user for exceptions which may result in data inconsistency or system unstability. It is better to display an error to the user rather than finish a transaction and save corrupted data. At least, someone knows there is an error. Otherwise, errors happen, data is corrupted, and nobody knows. For example, make sure that SQLCatalog ZSQL methods generate exceptions and errors in the user interface in case of syntax error or database adapter connectivity problem.

Remember that hasattr hides all exceptions, including ZODB Conflict Errors, it's much safer to use getattr(obj, name, _marker) is not _marker in those situations.

Good Example

   1 try:
   2   ...
   3 except:
   4   LOG(...)
   5   raise

As long as you re-raise the same exception, you can catch all types of exceptions.

Never access properties directly in application code

Always use accessors (ex. self.getTitle(), self.setTitle()). Never access properties directly (ex. self.title). ERP5 design is method oriented. All accessors can be automatically overloaded to support different data access schemes or extend a static property into a dynamic property (ex. price). All methods can also act in ERP5 as events through interaction workflows. This allows for implementing complex interactions and constraints. Not using accessors would lead to non generic code and potential inconsistencies with the general ERP5 system. Only the core of ERP5 may in some case access properties directly.

Never use properties to store results of calculations

Always use properties to store document 'original' content (ie. the one which was entered by the user). If you need to process document original content and store the result somewhere, either this is a new document (and Simulation should be used if possible to track relations between calculated documents and original documents), or this is a workflow variable (with complete historisation), or this is an internal property to be handled within the class code.

Never modify more than 100 objects in a single transaction

If you need to modify many Zope objects (ex. 10000), please use activities. Modifying too many objects in a single transaction leads to conflicts which degrade the performance of Zope application server and eventually make applications unusable.

Never write to the ZODB if there is a way not to write

Each write transaction into the ZODB creates risks of conflicts and increases the size of the ZODB. Code which does too many writes can turn a 100 MB database into a 10 GB database in a few days.

Never use temp folder over multiple requests

Storing into temp folder is incompatible with ZEO and clustering, unless this is only for a single transaction. If you need to share data over multiple transactions, either user ERP5 preferences, an ERP5 selection object or any ad hoc storage. ERP5 selection object implementation is worth having a look at because it has been implemented to reduce ZODB conflicts by hashing selections per use. Make sure though that all changes to ZODB storage happen within the same mounting point of ZODB (ex. /erp5/portal_selections). This allows for selective compacting ZODB from time to time or for using different ZODB backends whenever complete history and traceability is not needed.

Never use sessions

In our experience, sessions in Zope are buggy and incompatible with clustering. They generate a lot of ZODB conflicts too. Right approaches are based on the same concepts as the ones desribed in the previous paragraph.

Never use cookies

ERP5 has been designed to be as much cookie technology independent as possible. Also, cookies have many hidden limitations. For example, big size cookies are sometimes "scrambled" by Apache or Squid front ends. This means that a system based on the exchange of big size cookies can not be reliable (rfc2109 specifies a limit of 4096 bytes for a cookie). The right solution is, sadly, to use standard forms to exchange data. This may involve using encrypted binhexed or uuencoded data in hidden form fields.

Never override python builtin names

Take care not to override builtin python names (as returned by dir(__builtins__) in the Python interpreter) as it makes debugging harder and hides errors. If you do so, the benefit is that you can use some python code checkers like PyFlakes, pylint or PyChecker.

Never name a base category with the same name as a property

Base category ids and property ids are meant to form a global vocabulary where every id is defined once and once only. This requires base categories to have a different id from properties. The same is true for base domains (which must be named with an id which is neither a category id or a property id).

Never use contentValues or objectValues if the number of documents could be more than 50

objectValues and contentValues are suitable for folders with a few documents (ex. less than 50). Many folders in ERP5 may include millions of documents. Use searchFolder is you need to list / sort documents in such folder.

Never use the expression "if obj" whenever obj is not a simple type

It is impossible to know in advance what is the value of an object. For example "if obj" returns false if the object is an empty folder, and "if obj" returns true if it is a folder with sub objects. Remember that python uses __len__ if __nonzero__ method doesn't exist on object. So "if obj" MUST NOT be used to check if the object is None. Here is a good example :

   1 if obj is not None:
   2   xxx

Never use the expression "if obj is not None and obj or another_object" whenever obj is not a simple type

For the same reason as before, you can have an object which is not None but returns false (for example an empty folder), and you will get the another_object. This is not what you expect. So don't use that kind of expression. You must use this instead :

   1 if obj is None:
   2   obj = another_object

Never use immediateReindexObject or recursiveImmediateReindexObject

Sometimes, it is possible that you feel that it will be easier if you can use immediateReindexObject or recursiveImmediateReindexObject. Theses two methods must be used only for debugging. So never use it in any script, never use it in the ERP5 Code.

Each time we use immediateReindexObject, it will work for development, but it will fails on production if you have more than 2 users.

If you need immediateReindex, this probably means that you are doing the wrong way.

Never use getPath to get a user URL

Use absolute_url_path instead, or absolute_url. getPath only returns the path of an object in the OFS. This means the physical path. Whereas absolute_url_path takes into account all virtual hosting configuration. absolute_url includes the host part of the URL (http://hostname/url/to/object) while absolute_url_path only returns the user URL (/url/to/object).

Never use aq_parent to get the physical container of an ERP5 document

Use instead getParentValue() method which is defined on Base class. aq_parent is only useful if you wish to get the acquisition context of an object, which may be different from its physical container. For example, aq_parent is useful to calculate a breadcrumb whenever the acquisition path results from the combination of multiple physical path (ex. web_site_module/a/b/web_page_module/c). Refer to the Zope Developer Guide for more information on acquisition.

Never directly access attributes of state_change object in workflow scripts

Even though it's likely to work use instead state_change['attribute_name'] as default access method.

Never use doActionFor to implement logic

ERP5 generates transition methods for all workflow transitions implemented as workflow methods. If you need to programmaticaly trigger a transition on a workflow, use transition methods. Transition methods are intended to represent the workflow logic, unlike workflow actions which should be used for user interface only.

Bad Example:

context.getPortalObject().portal_workflow.doActionFor(object, 'submit_action')

Good Example:


Never hardcode interactions (which may require extensibility)

In short, use interaction workflows each time you have to implement something such as whenever X is called on A, invoke Y on B, even it may be a bit harder to debug at first.

Now, here is the long explanation. For example, suppose that you want to call a method (eg. 'expand') each time an order or an order line is reindexed so that the simulation takes into account changes in that order. A simple (but wrong) approach consists in invoking expand from the overloaded reindex method defined on the Order or Order Line classes. The problem with this approach are multiple. First of all, there is no single point where this interaction pattern is explicitly defined. If there are a few interactions in the system, this is fine. But in an ERP system with many interactions, it leads to spaghetti code and no way to understand what happens.

Second, if one wants to extend the Order model and allow the user, let us say, to add a Resource instance inside certain orders, and at the same time a Rule which changes the invoicing based on parameters defined on that Resource instance, it is necessary to call 'expand' each time the Resource instance is modified. If we follow the simple approach, we must overload also the 'reindex' method of the Resource class (by creating for example a local Resource class with portal_classes). In the end, implementing interactions in a complex system may lead to overload most classes.

With interaction workflows, extending an interaction becomes much more simple. Adding an interaction definition for the Resource class each time the resource is part of an Order can provide a straightforward solution without having to overload any method.

Do not however overuse interaction workflows or create too many interaction workflows. It is better to have a couple of well defined interactions than dozens of small interactions. Also, if an interaction really does not need to be extended, it is acceptable to use interaction classes, decorators and / or method wrapper classes to achieve at the level of the python source code the same result as interaction workflows.

Never use harcoded state names in class code

Instead use state groups

Bad Example:

if self.getSimulationState() == "draft":

Good Example:

if self.getSimulationState() in self.getPortalDraftOrderStateList():

Never use failUnless to test identity

In unit test, use assertEqual, thus we see the difference in traceback

Bad Example:

self.failUnless(self.getSimulationState() == "draft")
self.failUnless(self.getSimulationState() =! "delivered")

Good Example:

self.assertEquel(self.getSimulationState(), "draft")
self.assertNotEqual(self.getSimulationState(), "delivered")

Configuration Crimes

Never render a report directly

Every list view in ERP5 should contain a default report button. Clicking on this button should never display directly a report. Instead, it must first display a standard dialogue with a few parameters (or no parameters). The report is only created once the dialogue button has been pressed.

Never render a print result directly

Unless there is a single print action, all print actions should behave as reports and Lead the user to a dialogue.

Never forget edit_workflow on Documents

edit_workflow is required to track the creation and modification history on a Document. Always chain a document to edit_workflow. This does not apply though to modules (which contain documents). Sub objects of a Document should either be chained to edit_workflow or to an appropriate interaction workflow which triggers the edit workflow of a related document (ex. the parent).

Never copy/paste the Person and Organisation modules to create a new module unless you really know why

The Person and Organisation modules are intended to store objects which represent people and organisations of many different kinds: customers, suppliers, staff, etc. It is a mistake to create multiple modules and types to represent the different kinds of people and organisations for two reasons: it breaks the unification principle in ERP5 and it requires to update so many relation fields everywhere. Instead, use the role category to achieve the same result. Make also sure that workflows on Person and Organisation as well as security are designed based on the role category value(s). With this approach, a Person who used to be a staff and then becomes a customer can be tracked from the same object. Do not hesitate to implement 'big' workflows which consist in the union of all possible cases. It may sound complex or heavy, but it is the only way to achieve the unification of people and organisations. In the consulting phase, the workflow analysis does not need obviously to focus on the unified complex workflow but rather on simple workflows on per use case base (ex. customer validation, supplier validation, etc.). It is the responsibility of the implementer to find a way to unify all these workflows in a single workflow. The only exceptions to this rule which we know are some cases in which one wants to use a kind of buffer module to import people and organisations before copying them into the real module, or some cases in which one wants to create a new kind of node (ex. animal).

Never hardcode categories in scripts or classes

Each time a category was hardcoded in a script or a class, it was found later on that it was a mistake because it prevented flexibility in configuration from one ERP5 site to another. Also, it makes the code and configuration quite unreadable. The solution is quite simple: either select category values in a ListField in the dialogue which is presented before running the script or define those categories as preferences. Use preferences if this choice of categories is really general allover the application. Last word: ERP5 was designed based on the principle that for maximum flexibility, all categories should be defined (or definable) for each ERP5 site.

In unit test, never makes test methods dependants between them

When tests are dependant, it is hard to identify which unit is really broken. Indeed, if test_Y needs test_X, then when test_X fails, test_Y will also fails, even if the unit tested by test_Y might still work.. Also, if you do this way you can't run a single test at once and you will thus loose so much time either to run all tests or to find which tests must be run before this one.

Bad Example :

   1 def test_X(self):
   2   foo = self.getFooModule().newContent(id='foo')
   3   # Here start some assertions for functionnality A
   5 def test_Y(self):
   6   foo = self.getFooModule().getObject('foo')
   7   # Here start some assertions for functionnality B

There is many way of avoid such practice, here a good example with some shared code that will create needed objects for you :

   1 def createFoo(self):
   2   return self.getFooModule().newContent()
   4 def test_X(self):
   5   foo = self.createFoo()
   6   # Here start some assertions for functionnality A
   8 def test_Y(self):
   9   foo = self.createFoo()
  10   # Here start some assertions for functionnality B

GuidelinesForCodingCrimes (last edited 2012-01-30 07:51:55 by ArnaudFontaine)