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!

QuantConnect Lean 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.

The check was implemented by using the PVS-Studio static analyzer. 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.

Accidents are not accidental.

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;
  }
  ....
}

V3022 Expression '_random.Next(0, 1) == 0' is always true. RandomValueGenerator.cs 142

The bottom line was that either one or the other value was selected with a 50% probability. However, in this case, the Next method will always return 0.

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:

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;
  }
  ....
}

Passing reference-type parameters

Example

/// <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++;
  }
}

V3061 Parameter 'array' is always rewritten in method body before being used. SecurityPortfolioManager.cs 192

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.

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 outside the current method will remain unchanged.

It happens because the array argument will be passed to the method by value, not by reference. Thus, when the assign operation is done, the array 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:

public void CopyTo(out KeyValuePair<Symbol, SecurityHolding>[] array, // <=
                   int index)
{
  array = new KeyValuePair<Symbol, SecurityHolding>[Securities.Count];
  ....
}

Since we are certainly creating a new array in the method, the out modifier must be used instead of ref. This immediately indicates that the variable inside will be assigned a value.

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".

Unbinding the resources

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();
}

V3114 IDisposable object 'crypt' is not disposed before method returns. Extensions.cs 510

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:

"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 method is usually used to unbind such unmanaged resources".

In other words, we have created a crypt variable of the SHA256Managed type, which implements the IDisposable interface. As a result, when we exit the method, the potentially captured resources will not be released.

To prevent this, I recommend choosing using. The Dispose method is activated automatically upon reaching the closing curly bracket associated with the using instruction. Let's take a look at this:

public static string ToSHA256(this string data)
{
  using (var crypt = new SHA256Managed())
  {
    var hash = new StringBuilder();
    ....
  }
}

But if you don't like curly brackets, then in C# 8.0 you can write like this:

public static string ToSHA256(this string data)
{
  using var crypt = new SHA256Managed();
  var hash = new StringBuilder();
  ....
}

The difference with the previous version is that the Dispose method is activated when the closing curly bracket of the method is reached. This is the end of the piece where crypt is declared.

Real numbers

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; }
  ....
}

V3040 The '10.25' literal of the 'double' type is compared to a value of the 'int' type. OpeningBreakoutAlgorithm.cs 426

It looks strange that in the condition the value of an int variable is compared with a double-type literal. This looks odd and another variable is clearly supposed to be here. And, indeed, if you check out which similarly named fields TimeOfDay has, we will find:

public double TotalHours { get; }

The code probably should look like this:

public bool ShouldPlot
{
  get
  {
    ....
    if (Time.TimeOfDay.TotalHours < 10.25) return true;
    ....
  }
}

Also keep in mind that you mustn't check floating-point numbers for direct equality ("==", "!="). And don't forget about typecasting.

Switch statement

Tip 1

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);
}

V3002 The switch statement does not cover all values of the 'TradingDayType' enum: EconomicEvent. TradingCalendar.cs 79

The type of the variable type is TradingDayType, and it is enum:

public enum TradingDayType
{
  BusinessDay,
  PublicHoliday,
  Weekend,
  OptionExpiration,
  FutureExpiration,
  FutureRoll,
  SymbolDelisting,
  EquityDividends,
  EconomicEvent
}

If you count, you will see that there are 9 elements in the enumeration, but only 8 are checked in switch. Such situation could happen due to code extension. To prevent this, I always recommend explicitly using default:

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);
}

As you may have noticed, the return statement that stood after the switch moved to the default section. In this case, the program's logic has not changed, but I still recommend writing it this way.

Code extensibility is the reason for it. In the case of the original, it's possible to safely add some logic before return false, without suspecting that this is the default of the switch statement. Now everything is evident and clear.

However, if you think that only part of the enumeration elements should always be processed in your case, you can throw an exception:

default:
  throw new CustomExeption("Invalid enumeration element");

Personally, I got hooked on this syntactic sugar C# 8.0:

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
  };
};

Tip 2

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);
  }
}

V3139 Two or more case-branches perform the same actions. SecurityCacheTests.cs 510

In two different cases, 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 that way:

public string[] GetPropertiesBy(SecuritySeedData type)
{
  switch (type)
  {
    case SecuritySeedData.None:
      return new string[0];

    case SecuritySeedData.OpenInterest:
    case SecuritySeedData.OpenInterestTick:
      return new[] { "OpenInterest" };

    ....
  }
}

This clearly indicates what we want and removes an extra line as well. :)

If statement

Example 1

[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);
  }
  ....
}

V3022 Expression 'symbol.SecurityType != SecurityType.Equity || resolution != Resolution.Daily || resolution != Resolution.Hour' is always true. LiveTradingDataFeedTests.cs 1431

The condition is always true. After all, in order to have the condition failed, the variable resolution must have the Resolution.Daily value and Resolution.Hour at a time. A possible fixed variant:

[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);
  }
  ....
}

Some tips for the if statement. When there is a condition that consists entirely of "||" operators, then after writing, verify whether the same variable is checked for inequality to something else several times in a row.

The situation is similar in the condition with the "&&" operator. If a variable is checked for equality to something repeatedly, it is most likely to be a logical error.

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.

Example 2

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));
}

V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the first argument. StringExtensions.cs 311

According to the analyzer the value -1 can be passed as the first argument of the Substring method. This will cause an exception of the type System.ArgumentOutOfRangeException. 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.

The startIndex parameter has the int type, so its values are in the [-2147483648, 2147483647] range. Therefore, to prevent bounds violation, the developer wrote the following condition:

if (startIndex < -1)
{
  startIndex = 0;
}

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 variable (from the analyzer's point of view) is -1.

In these situations, I suggest using a construction like this:

if (variable < value)
{
  variable = value;
}

This combination is much easier to read, since it involves one less value. So, I suggest you fix the problem like this:

public static string SafeSubstring(....)
{
  ....
  if (startIndex < 0)
  {
    startIndex = 0;
  }

  return value.Substring(startIndex, 
                         Math.Min(length, value.Length - startIndex));
}

You can say that we could have just changed the sign in the condition in the initial example:

if (startIndex <= -1)
{
  startIndex = 0;
}

The error also disappears. However, the logic will look like this:

if (variable <= value - 1)
{
  variable = value;
}

Agree that it looks overloaded.

Example 3

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)}");  
  }
  ....
}

V3080 Possible null dereference. Consider inspecting 'buyingPowerModel'. BasicTemplateFuturesAlgorithm.cs 107

V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'buyingPowerModel', 'futureMarginModel'. BasicTemplateFuturesAlgorithm.cs 105

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 will be strictly null inside, dereferencing will occur:

$"Found: {buyingPowerModel.GetType().Name}. "

The reason is that the condition has a wrong variable compared to null. It's obvious that futureMarginModel should be written instead of buyingPowerModel. A fixed version:

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)}");  
  }
  ....
}

However, there is still a problem with dereferencing buyingPowerModel inside the condition. After all, futureMarginModel will be null not only when it is not FutureMarginModel, but also when buyingPowerModel is null. So I suggest this version:

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)}");   
  }
  ....
}

Personally, I have recently got to like writing such constructions using is. So the code becomes shorter and it is more difficult to make a mistake. This example is completely similar to the example above:

public override void OnEndOfAlgorithm()
{
  var buyingPowerModel = Securities[_contractSymbol].BuyingPowerModel;
  if (!(buyingPowerModel is FutureMarginModel futureMarginModel))
  {
    ....
  }
  ....
}

Moreover, in C# 9.0 we will be able to write the keyword not:

public override void OnEndOfAlgorithm()
{
  var buyingPowerModel = Securities[_contractSymbol].BuyingPowerModel;
  if (buyingPowerModel is not FutureMarginModel futureMarginModel)
  {
    ....
  }
  ....
}

Example 4

public static readonly Dictionary<....> 
  FuturesExpiryDictionary = new Dictionary<....>()
{
  ....
  if (twoMonthsPriorToContractMonth.Month == 2)
  {
    lastBusinessDay = FuturesExpiryUtilityFunctions
                      .NthLastBusinessDay(twoMonthsPriorToContractMonth, 1);
  }
  else
  {
    lastBusinessDay = FuturesExpiryUtilityFunctions
                      .NthLastBusinessDay(twoMonthsPriorToContractMonth, 1);
  }
  ....
}

V3004 The 'then' statement is equivalent to the 'else' statement. FuturesExpiryFunctions.cs 1561

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:

public static readonly Dictionary<....> 
  FuturesExpiryDictionary = new Dictionary<....>()
{
  ....
  if (twoMonthsPriorToContractMonth.Month == 2)
  {
    lastBusinessDay = FuturesExpiryUtilityFunctions
                      .NthLastBusinessDay(twoMonthsPriorToContractMonth, 2);
  }
  else
  {
    lastBusinessDay = FuturesExpiryUtilityFunctions
                      .NthLastBusinessDay(twoMonthsPriorToContractMonth, 1);
  }
  ....
}

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:

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.

Example 5

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}";
  }
}

V3022 Expression 'request.Parameters.Count > 0' is always true. GDAXBrokerage.Utility.cs 63

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.

To avoid this, when you are in a condition, always keep in mind at what values you will enter it.

A possible fixed variant:

public AuthenticationToken GetAuthenticationToken(IRestRequest request)
{
  ....
  if (request.Method == Method.GET && request.Parameters.Count > 0)
  {
    var parameters = string.Join(....);
    url = $"{request.Resource}?{parameters}";
  }
}

Example 6

public bool Setup(SetupHandlerParameters parameters)
{
  ....
  if (job.UserPlan == UserPlan.Free)
  {
    MaxOrders = 10000;
  }
  else
  {
    MaxOrders = int.MaxValue;
    MaximumRuntime += MaximumRuntime;
  }

  MaxOrders = job.Controls.BacktestingMaxOrders; // <=
  ....
}

V3008 The 'MaxOrders' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 244, 240. BacktestingSetupHandler.cs 244

Here, the MaxOrders variable is assigned a value twice in a row. In other words, logic with conditions is unnecessary.

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:

public bool Setup(SetupHandlerParameters parameters)
{
  ....
  if (job.UserPlan != UserPlan.Free)
  {
    MaximumRuntime += MaximumRuntime;
  }

  MaxOrders = job.Controls.BacktestingMaxOrders;
  ....
}

Typical human failings

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.

General recommendations on them:

When copying something, make changes to the copy as soon as you paste it;
review the code;
use special tools that will look for errors for you.

Case 1

public class FisherTransform : BarIndicator, IIndicatorWarmUpPeriodProvider
{
  private readonly Minimum _medianMin;
  private readonly Maximum _medianMax;
  public override bool IsReady => _medianMax.IsReady && _medianMax.IsReady;
}

V3001 There are identical sub-expressions '_medianMax.IsReady' to the left and to the right of the '&&' operator. FisherTransform.cs 72

In this example, the IsReady field must depend on two conditions, but in fact it depends on one. It's all the fault of a typo. Most likely, instead of _medianMin, _medianMax was written. A fixed version:

public class FisherTransform : BarIndicator, IIndicatorWarmUpPeriodProvider
{
  private readonly Minimum _medianMin;
  private readonly Maximum _medianMax;
  public override bool IsReady => _medianMin.IsReady && _medianMax.IsReady;
}

Case 2

public BacktestResultPacket(....) : base(PacketType.BacktestResult)
{
  try
  {
    Progress = Math.Round(progress, 3);
    SessionId = job.SessionId; // <=
    PeriodFinish = endDate;
    PeriodStart = startDate;
    CompileId = job.CompileId;
    Channel = job.Channel;
    BacktestId = job.BacktestId;
    Results = results;
    Name = job.Name;
    UserId = job.UserId;
    ProjectId = job.ProjectId;
    SessionId = job.SessionId; // <=
    TradeableDates = job.TradeableDates;
  }
  catch (Exception err) 
  {
    Log.Error(err);
  }
}

V3008 The 'SessionId' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 182, 172. BacktestResultPacket.cs 182

The class has many fields that must be initialized – many lines in the constructor. Everything is merged, and one field is initialized several times. In this case, there may be an extra initialization, or they forgot to initialize some other field.

If you are interested, you can also check out other errors found by this diagnostic rule.

Case 3

private const string jsonWithScore =
            "{" +
            "\"id\":\"e02be50f56a8496b9ba995d19a904ada\"," +
            "\"group-id\":\"a02be50f56a8496b9ba995d19a904ada\"," +
            "\"source-model\":\"mySourceModel-1\"," +
            "\"generated-time\":1520711961.00055," +
            "\"created-time\":1520711961.00055," +
            "\"close-time\":1520711961.00055," +
            "\"symbol\":\"BTCUSD XJ\"," +
            "\"ticker\":\"BTCUSD\"," +
            "\"type\":\"price\"," +
            "\"reference\":9143.53," +
            "\"reference-final\":9243.53," +
            "\"direction\":\"up\"," +
            "\"period\":5.0," +
            "\"magnitude\":0.025," +
            "\"confidence\":null," +
            "\"weight\":null," +
            "\"score-final\":true," +
            "\"score-magnitude\":1.0," +
            "\"score-direction\":1.0," +
            "\"estimated-value\":1113.2484}";

private const string jsonWithExpectedOutputFromMissingCreatedTimeValue =
            "{" +
            "\"id\":\"e02be50f56a8496b9ba995d19a904ada\"," +
            "\"group-id\":\"a02be50f56a8496b9ba995d19a904ada\"," +
            "\"source-model\":\"mySourceModel-1\"," +
            "\"generated-time\":1520711961.00055," +
            "\"created-time\":1520711961.00055," +
            "\"close-time\":1520711961.00055," +
            "\"symbol\":\"BTCUSD XJ\"," +
            "\"ticker\":\"BTCUSD\"," +
            "\"type\":\"price\"," +
            "\"reference\":9143.53," +
            "\"reference-final\":9243.53," +
            "\"direction\":\"up\"," +
            "\"period\":5.0," +
            "\"magnitude\":0.025," +
            "\"confidence\":null," +
            "\"weight\":null," +
            "\"score-final\":true," +
            "\"score-magnitude\":1.0," +
            "\"score-direction\":1.0," +
            "\"estimated-value\":1113.2484}";

V3091 Empirical analysis. It is possible that a typo is present inside the string literal. The 'score' word is suspicious. InsightJsonConverterTests.cs 209

Sorry for the large and scary code. Here different fields have the same values. This is a classic error from the copy-paste family. Copied, fell into the muse, forgot to make changes – here's the error.

Case 4

private void ScanForEntrance()
{
  var shares = (int)(allowedDollarLoss/expectedCaptureRange);
  ....
  if (ShouldEnterLong)
  {
    MarketTicket = MarketOrder(symbol, shares);
    ....
  }
  else if (ShouldEnterShort)
  {
    MarketTicket = MarketOrder(symbol, - -shares); // <=
    ....
    StopLossTicket = StopMarketOrder(symbol, -shares, stopPrice);
    ....
  }
  ....
}

V3075 The '-' operation is executed 2 or more times in succession. Consider inspecting the '- -shares' expression. OpeningBreakoutAlgorithm.cs 328

The unary operator "-" was used twice in a row. Thus, the value passed to the MarketOrder method remains unchanged. It's a tricky question how many unary minuses should be left here. Perhaps the prefix decrement operator "--" was meant to be here, but the space bar was accidentally pressed. There are so many variants, so one of the possible corrected options is:

private void ScanForEntrance()
{
  ....
  if (ShouldEnterLong)
  {
    MarketTicket = MarketOrder(symbol, shares);
    ....
  }
  else if (ShouldEnterShort)
  {
    MarketTicket = MarketOrder(symbol, -shares);
    ....
    StopLossTicket = StopMarketOrder(symbol, -shares, stopPrice);
    ....
  }
  ....
}

Case 5

private readonly SubscriptionDataConfig _config;
private readonly DateTime _date;
private readonly bool _isLiveMode;
private readonly BaseData _factory;

public ZipEntryNameSubscriptionDataSourceReader(
    SubscriptionDataConfig config, 
    DateTime date,
    bool isLiveMode)
{
  _config = config;
  _date = date;
  _isLiveMode = isLiveMode;
  _factory = _factory = config.GetBaseDataInstance(); // <=
}

V3005 The '_factory' variable is assigned to itself. ZipEntryNameSubscriptionDataSourceReader.cs 50

The _factory field is assigned the same value twice. There are only four fields in the class, so it's probably just a misprint. A fixed version:

public ZipEntryNameSubscriptionDataSourceReader(....)
{
  _config = config;
  _date = date;
  _isLiveMode = isLiveMode;
  _factory = config.GetBaseDataInstance();
}

Conclusion

There are many places where you can make a mistake. We notice and fix some of them immediately. Some of them are fixed in a code review, but I recommend assigning some of them to special tools.

Also, if you like such a format, please write about it. I'll do more like this. Thank you!

Talking About Errors in the QuantConnect Lean Code