Difference between revisions of "Coding Conventions"

Latest revision as of 12:13, 24 January 2017

Multithreading

Critical sections should be small and predictable

To avoid dead-locks critical sections should be small enough to prove that dead lock is not possible. Critical section should not cover any complicated business task with access to database (for example business action).

Please AVOID below construction:

lock(typeof(TranslateBO))
{
    //Anti-pattern do not put complicated business functions (like get data by key) inside critical section
    transBO.getDataByKey(phraseKey);

    result = transBO.detailData.syPhrase.Find(phraseKey);
    [...]
}

In above example getDataByKey function is used inside critical section. It is NOT possible to prove that this function placed in critical section will never fall into dead-lock. Above example should be fixed with:

transBO.getDataByKey(phraseKey);
lock(typeof(TranslateBO))
{
    result = transBO.detailData.syPhrase.Find(phraseKey);
    [...]
}

Ensure using thread-safe data containers

DataSets are not thread safe. If dataset is shared between threads and there is action which update dataset than any access to such dataset must be synchronized.

static TranslateDataSet translateCache = new TranslateDataSet()

function void updateCache(DataKey phraseKey, string phrase)
{
    lock(typeof(TranslateBO))
    {
        var phraseRow = translateCache.syPhrase.Find(phraseKey);
        if(phraseRow != null)
        {
             phraseRow.Phrase = phrase;
        }
    }

}

If data set is rarely updated than it make sense to use ReaderWriterLockSlim instead standard lock. For example ReaderWriterLockSlim allows to create sections winch allow parallel reading, but when it came to update than parallel reading will be blocked until section with write access is finished. Please follow ReaderWriteLockSlim Reference for details.

FastNET framework rules

Use translate only for messages intended for end user

Initially we had rule that any string literal should be translated, but it is possible that occasionally translation of internal exception can be more problematic than exception itself. Starting from v8.0 system is more resistant to fail due to translate, but applying above rules will improve system reliability still.

Translation rules:

1. Translate only messages which are intended to display on user screen
2. There is no need to translate exceptions which are written only to system event log
3. Translate should not be used in core functionalities which initialise whole system. At the moment forbidden is use translate in MetaData.cs

Configuration options (from xml configuration)

Currently we don't have any upgrade process which allow to add new parameters in client configuration file. When new option is introduced in configuration file then it should be optional. Without new option system should assume some default values. Logical consequence of above assumptions is that missing parameter should not be treated as exception. Missing parameter should not raise exception but default value should be used (it is valid only for new parameters, some sections of configuration file are mandatory from it's beginning). In other hand when parameter is added to configuration file but it's value type is wrong (for example expected is integer but literal is set) then exception should be thrown and reported in event log.

In version v8.0 and above use function getTypedValue<T>(string section, string key, T defaultValue) which was created to fulfil above observations:

try
{
    int retryCount = Configuration.getTypedValue<int>("interface/GeneralImport", "DataChangedByAnotherUserRetryCount", 5)
}
catch(ConfigParameterValueConvertException ex)
{
    //At this point exception is already written to windows event log, decide what to do: stop system? assume default value?
}
catch(Exception ex)
{
    //Other exception not related with value type
}

There is another version of function getTypedValue<T>(string section, string key) this function do not have defaultValue parameter. Without default value function assume that such parameter is mandatory and it will throw additional exception when parameter is missing in config file. This version should not be used when introducing new parameters which are likely not mandatory and some default value must be assumed.