{"id":392038,"date":"2024-06-29T10:11:02","date_gmt":"2024-06-29T10:11:02","guid":{"rendered":"http:\/\/savepearlharbor.com\/?p=392038"},"modified":"-0001-11-30T00:00:00","modified_gmt":"2024-06-29T10:11:02","slug":"","status":"publish","type":"post","link":"https:\/\/savepearlharbor.com\/?p=392038","title":{"rendered":"Talking About Errors in the QuantConnect Lean Code<\/span>"},"content":{"rendered":"
<\/div>\n
\n
\n
\n
\n
\"image1.png\"<\/div>\n

This article discusses errors found using a static analyzer in an open source project. There are some simple things that can help you avoid them. For example, the usage of language syntactic constructs starting from C# 8.0. We hope it will be exciting. Have fun reading!
<\/a>
QuantConnect Lean<\/a> is an open source algorithmic trading engine designed for easy strategy research, backtesting, and live trading. Compatible with Windows, Linux, and macOS. Integrates with prevalent data providers and brokerage companies to quickly deploy algorithmic trading strategies. <\/p>\n

The check was implemented by using the PVS-Studio static analyzer<\/a>. PVS-Studio is a tool designed to detect errors and potential vulnerabilities in the source code of programs, written in C, C++, C#, and Java. <\/p>\n

Accidents are not accidental.<\/h2>\n

<\/p>\n

public virtual DateTime NextDate(....) {   ....   \/\/ both are valid dates, so chose one randomly   if (   IsWithinRange(nextDayOfWeek, minDateTime, maxDateTime)        && IsWithinRange(previousDayOfWeek, minDateTime, maxDateTime))   {     return _random.Next(0, 1) == 0  \/\/ <=            ? previousDayOfWeek            : nextDayOfWeek;   }   .... }<\/code><\/pre>\n
  V3022<\/a> Expression ‘_random.Next(0, 1) == 0’ is always true. RandomValueGenerator.cs 142<\/p>\n
  The bottom line was that either one or the other value was selected with a 50% probability. However, in this case, the Next<\/i> method will always return 0. <\/p>\n
  This happens because the second argument is not included in the range of values. That is, the value that the method can return will be in the range [0,1). Let’s fix that:<\/p>\n
public virtual DateTime NextDate(....) {   ....   \/\/ both are valid dates, so chose one randomly   if (   IsWithinRange(nextDayOfWeek, minDateTime, maxDateTime)        && IsWithinRange(previousDayOfWeek, minDateTime, maxDateTime))   {     return _random.Next(0, 2) == 0            ? previousDayOfWeek            : nextDayOfWeek;   }   .... }<\/code><\/pre>\n
  <\/p>\n
Passing reference-type parameters<\/h2>\n
  Example<\/b><\/p>\n
\/\/\/ <summary> \/\/\/ Copy contents of the portfolio collection to a new destination. \/\/\/ <\/summary> \/\/\/ <remarks> \/\/\/ IDictionary implementation calling the underlying Securities collection \/\/\/ <\/remarks> \/\/\/ <param name=\"array\">Destination array<\/param> \/\/\/ <param name=\"index\">Position in array to start copying<\/param> public void CopyTo(KeyValuePair<Symbol, SecurityHolding>[] array, int index) {   array = new KeyValuePair<Symbol, SecurityHolding>[Securities.Count];   var i = 0;   foreach (var asset in Securities)   {     if (i >= index)     {       array[i] = new KeyValuePair<Symbol,SecurityHolding>(asset.Key,                                                           asset.Value.Holdings);     }     i++;   } }<\/code><\/pre>\n
  V3061<\/a> Parameter ‘array’ is always rewritten in method body before being used. SecurityPortfolioManager.cs 192<\/p>\n
  The method accepts the collection and immediately overwrites its value. Accept that this looks rather suspiciously. So, let’s try to understand what this method must do.<\/p>\n
  According to the comment and the method’s name, it becomes clear that another array must be copied to the array passed. However, this will not happen, and the value of the array<\/i> outside the current method will remain unchanged.<\/p>\n
  It happens because the array<\/i> argument will be passed to the method by value, not by reference. Thus, when the assign operation is done, the array<\/i> variable accessible inside the method will store a reference to the new object. The argument value passed to the method will remain unchanged. To fix this, the reference type argument must be passed by reference:<\/p>\n
public void CopyTo(out KeyValuePair<Symbol, SecurityHolding>[] array, \/\/ <=                    int index) {   array = new KeyValuePair<Symbol, SecurityHolding>[Securities.Count];   .... }<\/code><\/pre>\n
  Since we are certainly creating a new array in the method, the out <\/i>modifier must be used instead of ref<\/i>. This immediately indicates that the variable inside will be assigned a value.<\/p>\n
  By the way, this case enlarges the collection of my colleague, Andrey Karpov, you can learn about it from the article «Getting started collecting errors in copy functions<\/a>«.<\/p>\n
Unbinding the resources<\/h2>\n
  <\/p>\n
public static string ToSHA256(this string data) {   var crypt = new SHA256Managed();   var hash = new StringBuilder();   var crypto = crypt.ComputeHash(Encoding.UTF8.GetBytes(data),                                   0,                                   Encoding.UTF8.GetByteCount(data));   foreach (var theByte in crypto)   {     hash.Append(theByte.ToStringInvariant(\"x2\"));   }   return hash.ToString(); }<\/code><\/pre>\n
  V3114<\/a> IDisposable object ‘crypt’ is not disposed before method returns. Extensions.cs 510<\/p>\n
  To understand the meaning of this diagnostic, let’s first recall a little bit of theory. If you don’t mind, I will take information from the documentation for this diagnostic: <\/p>\n
  «The garbage collector automatically unbinds the memory associated with a controlled object if it is no longer in use and there are no visible references to it. However, we can’t say for sure when exactly the garbage collection will occur (unless you call it manually). In addition, the garbage collector does not have information about unmanaged resources such as handles, windows, or open files and threads. The Dispose<\/i> method is usually used to unbind such unmanaged resources». <\/p>\n
  In other words, we have created a crypt<\/i> variable of the SHA256Managed<\/i> type, which implements the IDisposable<\/i> interface. As a result, when we exit the method, the potentially captured resources will not be released. <\/p>\n
  To prevent this, I recommend choosing using<\/i>. The Dispose<\/i> method is activated automatically upon reaching the closing curly bracket associated with the using<\/i> instruction. Let’s take a look at this:<\/p>\n
public static string ToSHA256(this string data) {   using (var crypt = new SHA256Managed())   {     var hash = new StringBuilder();     ....   } }<\/code><\/pre>\n
  But if you don’t like curly brackets, then in C# 8.0 you can write like this:<\/p>\n
public static string ToSHA256(this string data) {   using var crypt = new SHA256Managed();   var hash = new StringBuilder();   .... }<\/code><\/pre>\n
  The difference with the previous version is that the Dispose<\/i> method is activated when the closing curly bracket of the method is reached. This is the end of the piece where crypt<\/i> is declared.<\/p>\n
Real numbers<\/h2>\n
  <\/p>\n
public bool ShouldPlot {   get   {     ....     if (Time.TimeOfDay.Hours < 10.25) return true;     ....   } }  public struct TimeSpan : IComparable,                           IComparable<TimeSpan>,                           IEquatable<TimeSpan>,                           IFormattable {   ....   public double TotalHours { get; }   public int Hours { get; }   .... }<\/code><\/pre>\n
  V3040<\/a> The ‘10.25’ literal of the ‘double’ type is compared to a value of the ‘int’ type. OpeningBreakoutAlgorithm.cs 426<\/p>\n
  It looks strange that in the condition the value of an int<\/i> variable is compared with a double-type<\/i> literal. This looks odd and another variable is clearly supposed to be here. And, indeed, if you check out which similarly named fields TimeOfDay<\/i> has, we will find:<\/p>\n
public double TotalHours { get; }<\/code><\/pre>\n
  The code probably should look like this:<\/p>\n
public bool ShouldPlot {   get   {     ....     if (Time.TimeOfDay.TotalHours < 10.25) return true;     ....   } }<\/code><\/pre>\n
  Also keep in mind that you mustn’t check<\/a> floating-point numbers for direct equality («==», «!=»). And don’t forget about typecasting<\/a>.<\/p>\n
Switch statement<\/h2>\n
  Tip 1<\/b><\/p>\n
public IEnumerable<TradingDay> GetDaysByType(TradingDayType type,                                              DateTime start,                                               DateTime end) {   Func<TradingDay, bool> typeFilter = day =>   {     switch (type)        \/\/ <=     {       case TradingDayType.BusinessDay:         return day.BusinessDay;       case TradingDayType.PublicHoliday:         return day.PublicHoliday;       case TradingDayType.Weekend:         return day.Weekend;       case TradingDayType.OptionExpiration:         return day.OptionExpirations.Any();       case TradingDayType.FutureExpiration:         return day.FutureExpirations.Any();       case TradingDayType.FutureRoll:         return day.FutureRolls.Any();       case TradingDayType.SymbolDelisting:         return day.SymbolDelistings.Any();       case TradingDayType.EquityDividends:         return day.EquityDividends.Any();     };     return false;   };   return GetTradingDays(start, end).Where(typeFilter); }<\/code><\/pre>\n
  V3002<\/a> The switch statement does not cover all values of the ‘TradingDayType’ enum: EconomicEvent. TradingCalendar.cs 79<\/p>\n
  The type of the variable type<\/i> is TradingDayType<\/i>, and it is enum<\/i>:<\/p>\n
public enum TradingDayType {   BusinessDay,   PublicHoliday,   Weekend,   OptionExpiration,   FutureExpiration,   FutureRoll,   SymbolDelisting,   EquityDividends,   EconomicEvent }<\/code><\/pre>\n
  If you count, you will see that there are 9 elements in the enumeration, but only 8 are checked in switch<\/i>. Such situation could happen due to code extension. To prevent this, I always recommend explicitly using default<\/i>:<\/p>\n
public IEnumerable<TradingDay> GetDaysByType(TradingDayType type,                                              DateTime start,                                               DateTime end) {   Func<TradingDay, bool> typeFilter = day =>   {     switch (type)     {       ....       default:         return false;     };   };   return GetTradingDays(start, end).Where(typeFilter); }<\/code><\/pre>\n
  As you may have noticed, the return<\/i> statement that stood after the switch<\/i> moved to the default<\/i> section. In this case, the program’s logic has not changed, but I still recommend writing it this way.<\/p>\n
  Code extensibility is the reason for it. In the case of the original, it’s possible to safely add some logic before return false<\/i>, without suspecting that this is the default<\/i> of the switch <\/i>statement. Now everything is evident and clear.<\/p>\n
  However, if you think that only part of the enumeration elements should always be processed in your case, you can throw an exception:<\/p>\n
default:   throw new CustomExeption(\"Invalid enumeration element\");<\/code><\/pre>\n
  Personally, I got hooked on this syntactic sugar C# 8.0:<\/p>\n
Func<TradingDay, bool> typeFilter = day => {   return type switch   {     TradingDayType.BusinessDay      => day.BusinessDay,     TradingDayType.PublicHoliday    => day.PublicHoliday,     TradingDayType.Weekend          => day.Weekend,     TradingDayType.OptionExpiration => day.OptionExpirations.Any(),     TradingDayType.FutureExpiration => day.FutureExpirations.Any(),     TradingDayType.FutureRoll       => day.FutureRolls.Any(),     TradingDayType.SymbolDelisting  => day.SymbolDelistings.Any(),     TradingDayType.EquityDividends  => day.EquityDividends.Any(),     _ => false   }; };<\/code><\/pre>\n
  Tip 2<\/b><\/p>\n
public string[] GetPropertiesBy(SecuritySeedData type) {   switch (type)   {     case SecuritySeedData.None:       return new string[0];      case SecuritySeedData.OpenInterest:       return new[] { \"OpenInterest\" };  \/\/ <=      case SecuritySeedData.OpenInterestTick:       return new[] { \"OpenInterest\" };  \/\/ <=      case SecuritySeedData.TradeTick:       return new[] {\"Price\", \"Volume\"};      ....      case SecuritySeedData.Fundamentals:       return new string[0];      default:       throw new ArgumentOutOfRangeException(nameof(type), type, null);   } }<\/code><\/pre>\n
  V3139<\/a> Two or more case-branches perform the same actions. SecurityCacheTests.cs 510<\/p>\n
  In two different cases<\/i>, the same value is returned. It looks very suspicious in such a form. It feels like someone copied, pasted and forgot to change the code. Therefore, here’s my advice: if the same logic must be performed for different values, then combine the case<\/i> that way:<\/p>\n
public string[] GetPropertiesBy(SecuritySeedData type) {   switch (type)   {     case SecuritySeedData.None:       return new string[0];      case SecuritySeedData.OpenInterest:     case SecuritySeedData.OpenInterestTick:       return new[] { \"OpenInterest\" };      ....   } }<\/code><\/pre>\n
  This clearly indicates what we want and removes an extra line as well. \ud83d\ude42<\/p>\n
If statement<\/h2>\n
  Example 1<\/b><\/p>\n
[TestCaseSource(nameof(DataTypeTestCases))] public void HandlesAllTypes<T>(....) where T : BaseData, new() {   ....   if (   symbol.SecurityType != SecurityType.Equity       || resolution != Resolution.Daily       || resolution != Resolution.Hour)   {     actualPricePointsEnqueued++;     dataPoints.Add(dataPoint);   }   .... }<\/code><\/pre>\n
  V3022<\/a> Expression ‘symbol.SecurityType != SecurityType.Equity || resolution != Resolution.Daily || resolution != Resolution.Hour’ is always true. LiveTradingDataFeedTests.cs 1431<\/p>\n
  The condition is always true. After all, in order to have the condition failed, the variable resolution<\/i> must have the Resolution.Daily<\/i> value and Resolution.Hour<\/i> at a time. A possible fixed variant:<\/p>\n
[TestCaseSource(nameof(DataTypeTestCases))] public void HandlesAllTypes<T>(....) where T : BaseData, new() {   ....   if (   symbol.SecurityType != SecurityType.Equity       || (   resolution != Resolution.Daily            && resolution != Resolution.Hour))   {     actualPricePointsEnqueued++;     dataPoints.Add(dataPoint);   }   .... }<\/code><\/pre>\n
  Some tips for the if<\/i> statement. When there is a condition that consists entirely of «||» operators, then after writing, verify whether the same variable is checked for inequality<\/b> to something else several times in a row. <\/p>\n
  The situation is similar in the condition with the «&&» operator. If a variable is checked for equality<\/b> to something repeatedly, it is most likely to be a logical error.<\/p>\n
  Also, if you write a compound condition, and it contains «&&»and «||», do not hesitate to put parentheses. This can help you either to see an error or to avoid it.<\/p>\n
  Example 2<\/b><\/p>\n
public static string SafeSubstring(this string value,                                     int startIndex,                                    int length) {   if (string.IsNullOrEmpty(value))   {     return value;   }    if (startIndex > value.Length - 1)   {     return string.Empty;   }    if (startIndex < -1)   {     startIndex = 0;   }    return value.Substring(startIndex,                           Math.Min(length, value.Length - startIndex)); }<\/code><\/pre>\n
  V3057<\/a> The ‘Substring’ function could receive the ‘-1’ value while non-negative value is expected. Inspect the first argument. StringExtensions.cs 311<\/p>\n
  According to the analyzer the value -1 can be passed as the first argument of the Substring<\/i> method. This will cause an exception of the type System.ArgumentOutOfRangeException<\/i>. Let’s see why this value can be obtained. In this example, we take no interest in the first two conditions, so they will be omitted in the reasoning.<\/p>\n
  The startIndex<\/i> parameter has the int<\/i> type, so its values are in the [-2147483648, 2147483647] range. Therefore, to prevent bounds violation, the developer wrote the following condition:<\/p>\n
if (startIndex < -1) {   startIndex = 0; }<\/code><\/pre>\n
  That is, it was assumed that if a negative value was received, we would simply change it to 0. But instead of «<=» they wrote «<«, and now the range’s lower limit of the startIndex<\/i> variable (from the analyzer’s point of view) is -1. <\/p>\n
  In these situations, I suggest using a construction like this:<\/p>\n
if (variable < value) {   variable = value; }<\/code><\/pre>\n
  This combination is much easier to read, since it involves one less value. So, I suggest you fix the problem like this:<\/p>\n
public static string SafeSubstring(....) {   ....   if (startIndex < 0)   {     startIndex = 0;   }    return value.Substring(startIndex,                           Math.Min(length, value.Length - startIndex)); }<\/code><\/pre>\n
  You can say that we could have just changed the sign in the condition in the initial example:<\/p>\n
if (startIndex <= -1) {   startIndex = 0; }<\/code><\/pre>\n
  The error also disappears. However, the logic will look like this:<\/p>\n
if (variable <= value - 1) {   variable = value; }<\/code><\/pre>\n
  Agree that it looks overloaded.<\/p>\n
  Example 3<\/b><\/p>\n
public override void OnEndOfAlgorithm() {   var buyingPowerModel = Securities[_contractSymbol].BuyingPowerModel;   var futureMarginModel = buyingPowerModel as FutureMarginModel;   if (buyingPowerModel == null)   {     throw new Exception($\"Invalid buying power model. \" +                         $\"Found: {buyingPowerModel.GetType().Name}. \" +                         $\"Expected: {nameof(FutureMarginModel)}\");     }   .... }<\/code><\/pre>\n
  V3080<\/a> Possible null dereference. Consider inspecting ‘buyingPowerModel’. BasicTemplateFuturesAlgorithm.cs 107<\/p>\n
  V3019<\/a> Possibly an incorrect variable is compared to null after type conversion using ‘as’ keyword. Check variables ‘buyingPowerModel’, ‘futureMarginModel’. BasicTemplateFuturesAlgorithm.cs 105<\/p>\n
  A very interesting fragment. The analyzer issues two warnings at once. And in fact, they contain the problem and its cause. Firstly, let’s see what happens if the condition is met. Since buyingPowerModel<\/i> will be strictly null<\/i> inside, dereferencing will occur:<\/p>\n
$\"Found: {buyingPowerModel.GetType().Name}. \"<\/code><\/pre>\n
  The reason is that the condition has a wrong variable compared to null<\/i>. It’s obvious that futureMarginModel<\/i> should be written instead of buyingPowerModel<\/i>. A fixed version:<\/p>\n
public override void OnEndOfAlgorithm() {   var buyingPowerModel = Securities[_contractSymbol].BuyingPowerModel;   var futureMarginModel = buyingPowerModel as FutureMarginModel;   if (futureMarginModel == null)   {     throw new Exception($\"Invalid buying power model. \" +                         $\"Found: {buyingPowerModel.GetType().Name}. \" +                         $\"Expected: {nameof(FutureMarginModel)}\");     }   .... }<\/code><\/pre>\n
  However, there is still a problem with dereferencing buyingPowerModel<\/i> inside the condition. After all, futureMarginModel<\/i> will be null<\/i> not only when it is not FutureMarginModel<\/i>, but also when buyingPowerModel<\/i> is null<\/i>. So I suggest this version:<\/p>\n
public override void OnEndOfAlgorithm() {   var buyingPowerModel = Securities[_contractSymbol].BuyingPowerModel;   var futureMarginModel = buyingPowerModel as FutureMarginModel;   if (futureMarginModel == null)   {     string foundType =    buyingPowerModel?.GetType().Name                         ?? \"the type was not found because the variable is null\";     throw new Exception($\"Invalid buying power model. \" +                         $\"Found: {foundType}. \" +                         $\"Expected: {nameof(FutureMarginModel)}\");      }   .... }<\/code><\/pre>\n
  Personally, I have recently got to like writing such constructions using is<\/i>. So the code becomes shorter and it is more difficult to make a mistake. This example is completely similar to the example above:<\/p>\n
public override void OnEndOfAlgorithm() {   var buyingPowerModel = Securities[_contractSymbol].BuyingPowerModel;   if (!(buyingPowerModel is FutureMarginModel futureMarginModel))   {     ....   }   .... }<\/code><\/pre>\n
  Moreover, in C# 9.0 we will be able to write the keyword not<\/i>:<\/p>\n
public override void OnEndOfAlgorithm() {   var buyingPowerModel = Securities[_contractSymbol].BuyingPowerModel;   if (buyingPowerModel is not FutureMarginModel futureMarginModel)   {     ....   }   .... }<\/code><\/pre>\n
  Example 4<\/b><\/p>\n
public static readonly Dictionary<....>    FuturesExpiryDictionary = new Dictionary<....>() {   ....   if (twoMonthsPriorToContractMonth.Month == 2)   {     lastBusinessDay = FuturesExpiryUtilityFunctions                       .NthLastBusinessDay(twoMonthsPriorToContractMonth, 1);   }   else   {     lastBusinessDay = FuturesExpiryUtilityFunctions                       .NthLastBusinessDay(twoMonthsPriorToContractMonth, 1);   }   .... }<\/code><\/pre>\n
  V3004<\/a> The ‘then’ statement is equivalent to the ‘else’ statement. FuturesExpiryFunctions.cs 1561<\/p>\n
  The same logic runs under different conditions. Since one of the arguments is a numeric literal, it is likely that another value may be passed. For example:<\/p>\n
public static readonly Dictionary<....>    FuturesExpiryDictionary = new Dictionary<....>() {   ....   if (twoMonthsPriorToContractMonth.Month == 2)   {     lastBusinessDay = FuturesExpiryUtilityFunctions                       .NthLastBusinessDay(twoMonthsPriorToContractMonth, 2);   }   else   {     lastBusinessDay = FuturesExpiryUtilityFunctions                       .NthLastBusinessDay(twoMonthsPriorToContractMonth, 1);   }   .... }<\/code><\/pre>\n
  But this is nothing more than an assumption. Here I would like to draw attention to the fact that the error occurs in the container’s initialization. The size of this initialization is almost 2000 lines:<\/p>\n
\"image2.png\"<\/div>\n
  Also, the code fragments inside are similar in appearance, which is logical, because here the collection is simply filled in. Therefore, be very careful when copying something in large and similar sections. Make changes at once, because then your eyes will get tired and you won’t see the problem.<\/p>\n
  Example 5<\/b><\/p>\n
public AuthenticationToken GetAuthenticationToken(IRestRequest request) {   ....   if (request.Method == Method.GET && request.Parameters.Count > 0)   {     var parameters = request.Parameters.Count > 0                      ? string.Join(....)                      : string.Empty;     url = $\"{request.Resource}?{parameters}\";   } }<\/code><\/pre>\n
  V3022<\/a> Expression ‘request.Parameters.Count > 0’ is always true. GDAXBrokerage.Utility.cs 63<\/p>\n
  The condition in the ternary operator is always true, because this check was already performed above. Now this is either a redundant check, or the «&&» and «||» operators are mixed up in the condition above. <\/p>\n
  To avoid this, when you are in a condition, always keep in mind at what values you will enter it.<\/p>\n
  A possible fixed variant:<\/p>\n
public AuthenticationToken GetAuthenticationToken(IRestRequest request) {   ....   if (request.Method == Method.GET && request.Parameters.Count > 0)   {     var parameters = string.Join(....);     url = $\"{request.Resource}?{parameters}\";   } }<\/code><\/pre>\n
  Example 6<\/b><\/p>\n
public bool Setup(SetupHandlerParameters parameters) {   ....   if (job.UserPlan == UserPlan.Free)   {     MaxOrders = 10000;   }   else   {     MaxOrders = int.MaxValue;     MaximumRuntime += MaximumRuntime;   }    MaxOrders = job.Controls.BacktestingMaxOrders; \/\/ <=   .... }<\/code><\/pre>\n
  V3008<\/a> The ‘MaxOrders’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 244, 240. BacktestingSetupHandler.cs 244<\/p>\n
  Here, the MaxOrders<\/i> variable is assigned a value twice in a row. In other words, logic with conditions is unnecessary.<\/p>\n
  To fix this, we have 2 options. We either remove the assignments in the then-else branches, or the assignment after the condition. Most likely, the code is covered by tests, and the program works correctly. Therefore, we will leave only the last assignment. A possible fixed variant:<\/p>\n
public bool Setup(SetupHandlerParameters parameters) {   ....   if (job.UserPlan != UserPlan.Free)   {     MaximumRuntime += MaximumRuntime;   }    MaxOrders = job.Controls.BacktestingMaxOrders;   .... }<\/code><\/pre>\n
  <\/p>\n
Typical human failings<\/h2>\n
  This section will cover copy-paste errors, accidentally pressed keys, and so on. Basically, the most common problems of human imperfection. We are not robots, so these situations are typical. <\/p>\n
  General recommendations on them: <\/p>\n