# Inside Java Enterprise code: let's check Flowable

Java is famous for its widespread use in enterprise applications\. Business processes need to be managed\. The Flowable platform can help with that\! What's more, the project is written in Java and is open source\. So, we can analyze it and search for errors using PVS\-Studio\.

![1195_flowable_article/image1.png](https://import.viva64.com/docx/blog/1195_flowable_article/image1.png)

## Terrifying words

What's in the foreword is pretty much all I can say about the project without just repeating the information on Wikipedia or the official website\. So, this is a good idea to visit it: [Flowable: Business Process Automation \| Low\-code \| Workflow Automation](https://www.flowable.com/)\.

Business means Enterprise\. Java and Enterprise are a well\-known bundle\! [That's why](https://projects.haykranen.nl/java/) many lines of code can be compared to the length of an essay\. This made me bother to slightly format the code to increase its readability\. Note that most fragments shown here probably look different in the original code\.

To make sure we're really working with [Java](https://github.com/EnterpriseQualityCoding/FizzBuzzEnterpriseEdition), look at some _short_ class names:

* TimerChangeProcessDefinitionSuspensionStateJobHandler;
* IntermediateThrowCompensationEventActivityBehavior;
* AbstractExternalInvocationBpmnParseHandler;
* ProcessInstanceStartEventSubscriptionModificationBuilderImpl;
* UnfinishedHistoricActivityInstanceByProcessInstanceIdMatcher;
* ParallelMultiInstanceWithNoWaitStatesAsyncLeaveJobHandler;
* EventSubProcessVariableListenerlStartEventActivityBehavior\.

But wait, there's more\. The record holder for length is the _UpdateChannelDefinitionTypeAndImplementationForAllChannelDefinitionsCmd_ class\. And when it comes to tests, the same name with the _Test_ suffix wins\.

Now that we've seen the Enterprise names, I suggest we move on to the code base\. To keep with tradition, we'll use PVS\-Studio for this task, and see if we can find the most interesting things\. Put on your suit jackets and let's go\.

## Instant execution

We begin our journey with tests\. Although tests are designed to detect bugs in the program logic, it's just as easy to make a mistake in them as it is in regular code\. Nobody writes tests for tests, but a static analyzer can check them\.

By the way, just recently we've written a [separate article](https://pvs-studio.com/en/blog/posts/cpp/1179/) about finding bugs in unit tests\! I encourage testers to read it\.

Here though, let's take a look at the following snippet:

```cpp
@Test
@CmmnDeployment
public void testExpressionReturnsFuture() {
  CountDownLatch latch = new CountDownLatch(2);
  TestBeanReturnsFuture testBean = new TestBeanReturnsFuture(latch, 
    cmmnEngineConfiguration.getAsyncTaskExecutor());
  CaseInstance caseInstance = cmmnRuntimeService.createCaseInstanceBuilder()
    .caseDefinitionKey("myCase")
    .transientVariable("bean", testBean)
    .start();

  assertCaseInstanceEnded(caseInstance);

  if (....) {
    HistoricCaseInstance historicCaseInstance = ....;

    // Every service task sleeps for 1s, 
    // but when we use futures it should take less then 2s.
    long durationInMillis = historicCaseInstance.getEndTime().getTime() – 
      historicCaseInstance.getEndTime().getTime();
    assertThat(durationInMillis).isLessThan(1500);
  }
}
```

The test checks the service execution time\. Unfortunately, _durationInMillis_ is always zero because it subtracts _getEndTime\(\)_ from _getEndTime\(\)_\. So, the test that checks whether this value is less than 1500 is always positive because 0 < 1500, which is easy to prove after reading [Principia Mathematica](https://en.wikipedia.org/wiki/Principia_Mathematica)\.

The fix is quite easy:

```cpp
long durationInMillis = historicCaseInstance.getEndTime().getTime() –
  historicCaseInstance.getStartTime().getTime();
```

I restarted the test with this fix, and it passed\. Now let's look at the PVS\-Studio analyzer message that helped us find the issue:

[V6001](https://pvs-studio.com/en/docs/warnings/v6001/) There are identical sub\-expressions historicCaseInstance\.getEndTime\(\)\.getTime\(\)' to the left and to the right of the '\-' operator\. ServiceTaskWithFuturesTest\.java 76

## BigDecimal

Next up is BigDecimal, a legend of fintech:

```cpp
public class ELExecutionContextBuilder {
  ....
  protected static void preProcessInputVariables(DecisionTable decisionTable,
    Map<String, Object> inputVariables) {
    ....
    // check if transformation is needed
    for (Map.Entry<String, Object> inputVariable : inputVariables.entrySet()) {
      String inputVariableName = inputVariable.getKey();
      try {
        Object inputVariableValue = inputVariable.getValue();
        if (inputVariableValue instanceof LocalDate) {
          Date transformedDate = ((LocalDate) inputVariableValue).toDate();
          inputVariables.put(inputVariableName, transformedDate);
        } else if (inputVariableValue instanceof java.time.LocalDate) {
          Date transformedDate = 
            Date.from(((java.time.LocalDate) inputVariableValue).atStartOfDay()
              .atZone(ZoneId.systemDefault())
              .toInstant());
          inputVariables.put(inputVariableName, transformedDate);
        } else if (....) {
          BigInteger transformedNumber = 
            new BigInteger(inputVariableValue.toString());
          inputVariables.put(inputVariableName, transformedNumber);
        } else if (inputVariableValue instanceof Double) {
          BigDecimal transformedNumber = 
            new BigDecimal((Double) inputVariableValue);
          inputVariables.put(inputVariableName, transformedNumber);
        } else if (inputVariableValue instanceof Float) {
          double doubleValue = 
            Double.parseDouble(inputVariableValue.toString());
          BigDecimal transformedNumber = new BigDecimal(doubleValue);   // <=
          inputVariables.put(inputVariableName, transformedNumber);
        }
      } catch (Exception ex) {
        throw new FlowableException(....);
      }
    }
  }
}
```

I recommend that you don't gaze into the code for too long, or it will gaze back at you\. We're interested in working with the aforementioned _BigDecimal_\. Let's look at the following note in [JavaDoc](https://docs.oracle.com/en/java/javase/23/docs/api/java.base/java/math/BigDecimal.html#%3Cinit%3E(double)) to clarify the details:

> The results of this constructor can be somewhat unpredictable\. One might assume that writing new BigDecimal\(0\.1\) in Java creates a BigDecimal which is exactly equal to 0\.1 \(an unscaled value of 1, with a scale of 1\), but it is actually equal to 0\.1000000000000000055511151231257827021181583404541015625\. This is because 0\.1 cannot be represented exactly as a double \(or, for that matter, as a binary fraction of any finite length\)\. Thus, the value that is being passed in to the constructor is not exactly equal to 0\.1, appearances notwithstanding\.

The same document recommends using _BigDecimal\.valueOf\(double\)_ or _new BigDecimal\(String\)\. _At the same time, the value in the specified fragment is parsed to _String_ and then back to _double_\. Does it make any difference? Let's check out just for the fun of it:

```cpp
void main() {
  double d = 0.1;
  BigDecimal bigDecimal;

  bigDecimal = new BigDecimal(d);
  println("new BigDecimal(double): \t" + bigDecimal.toPlainString());

  bigDecimal = new BigDecimal(String.valueOf(d));
  println("new BigDecimal(double -> string): \t" + 
    bigDecimal.toPlainString());

  bigDecimal = new BigDecimal(Double.parseDouble(String.valueOf(d)));
  println("new BigDecimal(double -> string -> double): \t" + 
    bigDecimal.toPlainString());

  bigDecimal = BigDecimal.valueOf(d);
  println("BigDecimal.valueOf(double): \t" + bigDecimal.toPlainString());
}
```

<details>
   <summary>void main\\\(\\\)? Is this Java code?</summary>

Yes\! For details, see [JEP\-477](https://openjdk.org/jeps/477)\. However, at the time of writing, this is still a preview feature\. I used it in the article to simplify this already simple fragment and not to write _public static void main\(String\[\] args\)_\. We have enough wordy strings in the Enterprise class names\.


</details>
We get the following output:

```cpp
new BigDecimal(double): 
  0.1000000000000000055511151231257827021181583404541015625
new BigDecimal(double -> string): 0.1
new BigDecimal(double -> string -> double):
  0.1000000000000000055511151231257827021181583404541015625
BigDecimal.valueOf(double): 0.1
```

Well, additional conversions of _double_ to string and back don't change the behavior\. In any case, we need to use either _valueOf_ or the constructor with the _String_ argument if we want to get 0\.1 from the 0\.1 input value \(based on the method name, it handles _input variables_\)\.

As a result, not only do we get rid of a potentially unexpected value, but we also eliminate the redundant _String_ to _double_ parsing operation:

```cpp
new BigDecimal(inputVariableValue.toString());
```

Lastly, here's the PVS\-Studio analyzer warning:

[V6068](https://pvs-studio.com/en/docs/warnings/v6068/) Constructor call can result in imprecise representation of the initialized value\. ELExecutionContextBuilder\.java 137

## Space is always at the end

```cpp
public class CronExpression implements Serializable, Cloneable {
  ....
  protected int findNextWhiteSpace(int i, String s) {
    for (; i < s.length() && (s.charAt(i) != ' ' || s.charAt(i) != '\t'); i++) {
    }

    return i;
  }
  ....
}
```

I'd like to note that the loop body is empty in the original code fragment \(otherwise it would have "\.\.\.\."\)\.

Here is an error we already encountered in other projects—one of the expressions is always true\. We're talking about _s\.charAt\(i\) \!\= '  ' \|\| s\.charAt\(i\) \!\= '\\t'_\. 

To get a false OR result, a character must be equal to two different characters at the same time\. Of course, this is impossible\.

The result is that the loop is executed as long as the _i < s\.length\(\)_ condition is _true_, and the method output is always the end of the string index, not the space character\.

The PVS\-Studio warning:

[V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 's\.charAt\(i\) \!\= '\\t'' is always true\. CronExpression\.java 963

## Immortal tests

```cpp
@Test
void convertJsonToModelKeyWithFixedValue() {
  ChannelModel channelModel = readJson(....);
  assertThat(channelModel)
    .isInstanceOfSatisfying(KafkaOutboundChannelModel.class, model -> {
      model.getRecordKey().getFixedValue().equals("customer");
    });
}

@Test
void convertJsonToModelKeyWithDelegateExpression() {
  ChannelModel channelModel = readJson(....);
  assertThat(channelModel)
    .isInstanceOfSatisfying(KafkaOutboundChannelModel.class, model -> {
      model.getRecordKey().getDelegateExpression().equals("${test}");
    });
}

@Test
void convertJsonToModelKeyWithEventField() {
  ChannelModel channelModel = readJson(....);
  assertThat(channelModel)
    B.isInstanceOfSatisfying(KafkaOutboundChannelModel.class, model -> {
      model.getRecordKey().getEventField().equals("customer");
    });
}
```

We do a short loop and meet the tests again\. Once again, they aren't falling\. Let's first look at the warnings that the PVS\-Studio analyzer issues, then we'll fix them:

* [V6010](https://pvs-studio.com/en/docs/warnings/v6010/) The return value of function 'equals' is required to be utilized\. KafkaOutboundChannelJsonConverterTest\.java 45
* [V6010](https://pvs-studio.com/en/docs/warnings/v6010/) The return value of function 'equals' is required to be utilized\. KafkaOutboundChannelJsonConverterTest\.java 53
* [V6010](https://pvs-studio.com/en/docs/warnings/v6010/) The return value of function 'equals' is required to be utilized\. KafkaOutboundChannelJsonConverterTest\.java 61

The developer probably expected the second argument of the _isInstanceOfSatisfying_ method to be _Function<T, Boolean\>_ for some reason\. I have no idea why there might have been such an expectation, but in reality, the method takes _Consumer<T\>_\. Maybe the problem is simple carelessness, and developers may have thought that the _equals_ method was equivalent to _AssertEquals_\.

For the test to fail, we need an exception\. Let's just wrap the _equals_ calls in _assertThat\(\.\.\.\.\)\.isEqualTo\(\.\.\.\.\)_\. Here's an example:

```cpp
assertThat(model.getRecordKey().getEventField()).isEqualTo("customer");
```

## Self\-parent

```cpp
public class ProcessExecutionLogger {
  protected void internalPopulateExecutionTree(....) {
    if (parentMapping.containsKey(parentNode.getId())) {
      for (....) {
        ....

        childNode.setParentNode(childNode);                        // <=
        parentNode.getChildNodes().add(childNode);

        internalPopulateExecutionTree(childNode, parentMapping);
      }
    }
  }
}
```

The _childNode_ object is a parent of itself due to the broken hierarchy here\. The necessary dependency is indirectly indicated by the line that immediately follows: _parentNode\.getChildNodes\(\)\.add\(childNote\)_\. So, the parent is aware of the child but not the other way around\. I'm wondering if there's a risk when iterating over such nodes to reach a _StackOverflowError_?

Here's the PVS\-Studio analyzer message about this anomaly:

[V6100](https://pvs-studio.com/en/docs/warnings/v6100/) An object is used as an argument to its own method\. Consider checking the first actual argument of the 'setParentNode' method\. ProcessExecutionLogger\.java 107

## Single iteration

```cpp
protected String getTargetNameSpace(XMLStreamReader xmlStreamReader) {
  String targetNameSpace = null;
  try {
    while (xmlStreamReader.hasNext()) {
      try {
        xmlStreamReader.next();
      } catch (Exception e) {
        LOGGER.debug("Error reading XML document", e);
        throw new DmnXMLException("Error reading XML", e);
      }
      targetNameSpace = xmlStreamReader.getNamespaceURI();
      break;
    }
  } catch (XMLStreamException e) {
    LOGGER.error("Error processing DMN document", e);
    throw new DmnXMLException("Error processing DMN document", e);
  }

  return targetNameSpace;
}
```

Let's dig into an unexpected _break_ that reduces the number of iterations to one regardless of the content in _xmlStreamReader_\. That may have been the intention, but then the devs should've used _if_\. This is a very strange resource handling\.

A small reference to git enables finding the previous version of the method \(it has been renamed\):

```cpp
protected boolean isDMN12(XMLStreamReader xtr) {
    try {
        while (xtr.hasNext()) {
            try {
                xtr.next();
            } catch (Exception e) {
                LOGGER.debug("Error reading XML document", e);
                throw new DmnXMLException("Error reading XML", e);
            }
            return DMN_12_TARGET_NAMESPACE.equals(xtr.getNamespaceURI());
        }
        return false;
    } catch (XMLStreamException e) {
        LOGGER.error("Error processing DMN document", e);
        throw new DmnXMLException("Error processing DMN document", e);
    }
}
```

This version uses _return_ instead of _break_, and there's no condition again\. The developers probably just decided to use _while_ in this method because the _xtr\.hasNext\(\)_ method is called\. 

Regardless of the original method concept, PVS\-Studio correctly issues a warning, because such code raises too many questions:

[V6037](https://pvs-studio.com/en/docs/warnings/v6037/) An unconditional 'break' within a loop\. DmnXMLConverter\.java 187

## Non\-empty collection

```cpp
protected void addRecipient(MimeMessage message, 
  Collection<String> recipients, 
  Message.RecipientType recipientType) {
  if (recipients == null || recipients.isEmpty()) {
    return;
  }
  Collection<String> newRecipients = recipients;
  Collection<String> forceRecipients = defaultsConfiguration.forceTo();
  if (forceRecipients != null && !forceRecipients.isEmpty()) {
    newRecipients = forceRecipients;
  }
  if (!newRecipients.isEmpty()) {
    for (String t : newRecipients) {
      try {
        message.addRecipient(recipientType, createInternetAddress(t));
      } catch (MessagingException e) {
        throw new FlowableMailException(....);
      }
    }
  }
}
```

This is a pointless check because _newRecipients_ takes the value of either _forceRecipients_ or _recipients_\. Both are checked for _isEmpty_ beforehand\.

It's also pointless because the _for \(String t : newRecipients\)_ loop wouldn't iterate over an empty collection anyway\.

The PVS\-Studio analyzer pointed out this redundant code:

Expression '\!newRecipients\.isEmpty\(\)' is always true\. JakartaMailFlowableMailClient\.java 194

## Field without value

```cpp
public class CallbackData {
  protected String callbackId;
  protected String callbackType;
  protected String instanceId;
  protected String oldState;
  protected String newState;
  protected Map<String, Object> additionalData = null;

  public CallbackData(String callbackId, String callbackType, 
      String instanceId, String oldState, 
      String newState, Map<String, Object> additionalData) {
    this.callbackId = callbackId;
    this.callbackType = callbackType;
    this.instanceId = instanceId;
    this.oldState = oldState;
    this.newState = newState;
  }

  ....

  public Map<String, Object> getAdditionalData() {
    return additionalData;
  }

  public void setAdditionalData(Map<String, Object> additionalData) {
    this.additionalData = additionalData;
  }
}
```

The PVS\-Studio warning:

[V6022](https://pvs-studio.com/en/docs/warnings/v6022/) Parameter 'additionalData' is not used inside constructor body\. CallbackData\.java 29

This is a rather hypothetical case\. A developer's use of the specified constructor can have unpleasant consequences, because if they pass _additionalData_ to it, the value won't be written\. A nasty _NullPointerException_ will occur, for example, when using the _getAdditionalData\(\)_ method if it's not expected to return _null_\.

## Possible null

```cpp
protected void endAllHistoricActivities(....) {
  ProcessEngineConfigurationImpl processEngineConfiguration = CommandContextUtil
      .getProcessEngineConfiguration();
  if (!processEngineConfiguration.getHistoryLevel()
        .isAtLeast(HistoryLevel.ACTIVITY)) {
    return;
  }

  List<HistoricActivityInstanceEntity> historicActivityInstances = ....;

  Clock clock = processEngineConfiguration.getClock();
  for (var historicActivityInstance : historicActivityInstances) {
    historicActivityInstance.markEnded(deleteReason, clock.getCurrentTime());

    // Fire event
    FlowableEventDispatcher eventDispatcher = null;
    if (processEngineConfiguration != null) {                        // <=
      eventDispatcher = processEngineConfiguration.getEventDispatcher();
    }
    if (eventDispatcher != null && eventDispatcher.isEnabled()) {
      seventDispatcher.dispatchEvent(....),
      processEngineConfiguration.getEngineCfgKey());                 // <=
    }
  }
}
```

_CommandContextUtil\.getProcessEngineConfiguration\(\)_ can return _null_\. So, since the _null_ check is done in the code, it is expected\. To be sure, let's look at this method:

```cpp
public static ProcessEngineConfigurationImpl getProcessEngineConfiguration() {
    return getProcessEngineConfiguration(getCommandContext());
}

public static ProcessEngineConfigurationImpl
  getProcessEngineConfiguration(CommandContext commandContext) {
    if (commandContext != null) {
        return ....
    }
    return null;
}
```

Can _commandContext_ be _null_? We continue through the call chain:

```cpp
public static CommandContext getCommandContext() {
    return Context.getCommandContext();
}

public static CommandContext getCommandContext() {
    Stack<CommandContext> stack = getStack(commandContextThreadLocal);
    if (stack.isEmpty()) {
        return null;
    }
    return stack.peek();
}
```

If _stack_ is empty, _null_ is returned\. It can be difficult to check whether we can get an empty collection, so let's stop here for now\. 

So, the call of_ processEngineConfiguration\.getHistoryLevel\(\)_ can result in _NullPointerException\._ 

The PVS\-Studio warning:

[V6060](https://pvs-studio.com/en/docs/warnings/v6060/) The 'processEngineConfiguration' reference was utilized before it was verified against null\. TerminateEndEventActivityBehavior\.java 166, TerminateEndEventActivityBehavior\.java 179

## ClassCastException

```cpp
public class IntegerDataObject extends ValuedDataObject {

  @Override
  public void setValue(Object value) {
    if (value instanceof String &&
      !StringUtils.isEmpty(((String) value).trim())) {
      this.value = Integer.valueOf(value.toString());
    } else if (value instanceof Number) {
      this.value = (Integer) value;                  // <=
    }
  }

  @Override
  public IntegerDataObject clone() {
    IntegerDataObject clone = new IntegerDataObject();
    clone.setValues(this);
    return clone;
  }
}
```

Here we've got a strange check for _Number _with a following casting to _Integer_, as well as to _Long_ and _Double_ in other _ValuedDataObject_ subclasses\. Perhaps the developer expected this to have some effect, or, as with primitive types, there would be an appropriate conversion\. However, the following program execution actually ends with a _ClassCastException_:

```cpp
void main() {
    Number integer = 528;
    println((Float)integer);
}
```

Here's the result:

```cpp
Exception in thread "main" java.lang.ClassCastException: 
  class java.lang.Integer cannot be cast to class java.lang.Float 
  (java.lang.Integer and java.lang.Float 
    are in module java.base of loader 'bootstrap')
        at Exception.main(Exception.java:3)
```

So, I think we need to reconsider what's going on in these methods\.

The PVS\-Studio warning:

* [V6042](https://pvs-studio.com/en/docs/warnings/v6042/) The expression is checked for compatibility with type 'java\.lang\.Number' but is cast to type 'java\.lang\.Integer'\. IntegerDataObject\.java 27 
* [V6042](https://pvs-studio.com/en/docs/warnings/v6042/) The expression is checked for compatibility with type 'java\.lang\.Number' but is cast to type 'java\.lang\.Long'\. LongDataObject\.java 27 
* [V6042](https://pvs-studio.com/en/docs/warnings/v6042/) The expression is checked for compatibility with type 'java\.lang\.Number' but is cast to type 'java\.lang\.Double'\. DoubleDataObject\.java 27

## Editing code style

```cpp
@Override
protected DmnElement convertXMLToElement(XMLStreamReader xtr, 
  ConversionHelper conversionHelper) throws Exception {
  DecisionService decisionService = new DecisionService();
  decisionService.setId(xtr.getAttributeValue(null, ATTRIBUTE_ID));
  decisionService.setName(xtr.getAttributeValue(null, ATTRIBUTE_NAME));
  decisionService.setLabel(xtr.getAttributeValue(null, ATTRIBUTE_LABEL));

  boolean readyWithDecisionService = false;
  try {
    while (!readyWithDecisionService && xtr.hasNext()) {
      xtr.next();
      if (xtr.isStartElement() &&
          ELEMENT_OUTPUT_DECISION.equalsIgnoreCase(xtr.getLocalName())) {
        DmnElementReference ref = new DmnElementReference();
        ref.setHref(xtr.getAttributeValue(null, ATTRIBUTE_HREF));
        decisionService.addOutputDecision(ref);
      } if (xtr.isStartElement() &&
            ELEMENT_ENCAPSULATED_DECISION
              .equalsIgnoreCase(xtr.getLocalName())) {
        DmnElementReference ref = new DmnElementReference();
        ref.setHref(xtr.getAttributeValue(null, ATTRIBUTE_HREF));
        decisionService.addEncapsulatedDecision(ref);
      } if (xtr.isStartElement() && 
            ELEMENT_INPUT_DATA.equalsIgnoreCase(xtr.getLocalName())) {
        DmnElementReference ref = new DmnElementReference();
        ref.setHref(xtr.getAttributeValue(null, ATTRIBUTE_HREF));
        decisionService.addInputData(ref);
      } else if (xtr.isEndElement() && 
          getXMLElementName().equalsIgnoreCase(xtr.getLocalName())) {
        readyWithDecisionService = true;
      }
    }
  } catch (Exception e) {
    LOGGER.warn("Error parsing output entry", e);
  }
  return decisionService;
}
```

I wanted to find an issue where the _if_ statement instead of _else if_ would cause the last _else_ block execution even if one of the _if_ blocks worked out\. The case is similar to what I found in the [GeoGebra](https://pvs-studio.com/en/blog/posts/java/1151/) project\. However, everything here ends with _else if_, which means that nothing tragic can happen\.

As a result, all we get is unpleasant code formatting\. To improve readability, we can move _if_ to the next lines or add _else_\. The second option may be better, as it avoids performing multiple pointless CPU operations\.

The PVS\-Studio warning:

* [V6086](https://pvs-studio.com/en/docs/warnings/v6086/) Suspicious code formatting\. 'else' keyword is probably missing\. DecisionServiceXMLConverter\.java 45 
* [V6086](https://pvs-studio.com/en/docs/warnings/v6086/) Suspicious code formatting\. 'else' keyword is probably missing\. DecisionServiceXMLConverter\.java 49

## Redundant check

```cpp
protected void leaveFlowNode(FlowNode flowNode) {
  ....

  // Determine which sequence flows can be used for leaving
  List<SequenceFlow> outgoingSequenceFlows = new ArrayList<>();
  for (SequenceFlow sequenceFlow : flowNode.getOutgoingFlows()) {

    String skipExpressionString = sequenceFlow.getSkipExpression();
    if (!SkipExpressionUtil.isSkipExpressionEnabled(
         skipExpressionString, sequenceFlow.getId(), 
         execution, commandContext)
      ) {

      if (!evaluateConditions                                        // <=
      || (evaluateConditions &&                                      // <=
          ConditionUtil.hasTrueCondition(sequenceFlow, execution) &&
         (defaultSequenceFlowId == null ||
          !defaultSequenceFlowId.equals(sequenceFlow.getId())))) {
        outgoingSequenceFlows.add(sequenceFlow);
      }

    } else if (flowNode.getOutgoingFlows().size() == 1 ||
        SkipExpressionUtil.shouldSkipFlowElement(
          skipExpressionString, sequenceFlow.getId(), execution, commandContext)
        ) {

      ....
      outgoingSequenceFlows.add(sequenceFlow);
    }
  }
  ....
}
```

I found a lot of similar code fragments\. The condition in _if_ is extremely cumbersome and redundant as it is:

[V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'evaluateConditions' is always true\. TakeOutgoingSequenceFlowsOperation\.java 220

Entering the second expression in the _or_ statement occurs only if _evaluateConditions_ is _false_\. So, the expression on the left is meaningless in the _\(evaluateConditions && \.\.\.\)_ check\.

## Conclusion

Let's wrap up our tour on potential project bugs for now\. You can see that a lot of them were caused by simple carelessness\. For the same reason, the devs didn't see them during code review\. The size of a single PR can make you never want to open it again, so let's not blame the developers\.

Static analysis tools, on the other hand, check code regardless of its size\. PVS\-Studio helped us find those problematic fragments\. Although, that's not all the project has in store\. I didn't have to study the code base for a long time to navigate through the project structure, I just ran the analyzer\. While full integration into the development process will enable you to immediately detect errors in the code\.

Also, you can [try PVS\-Studio](https://pvs-studio.com/en/pvs-studio/try-free/) for free :\)