Reduce unnecessary condition determination

[HyunSangHan]

Description of the Change

In test/reporters/helpers.js,

1. Changed some if to else if for code efficiency.
2. Changed the code by using || for code readability.

Benefits

1. Removing unnecessary discrimination processes.
   • If event is ifStr1, we no longer need to determine if event is ifStr2 or ifStr3. But when helpers.js is executed, it goes through all of if statements, because they exist independently of each other. By using else if, once the preceding callback function is executed, the execution of the latter callback function will be passed unconditionally.
2. Minimizing the code length.

Possible Drawbacks

I think it will not make any side effects.

[coveralls]

Coverage remained the same at 92.9% when pulling 974b064 on HyunSangHan:fix-helpers-if into 9cbb6f6 on mochajs:master.

[juergba]

These changes don't make sense to me.
The function signature shows three different parameters (ifstr1, ifstr2, ifstr3). If this design was chosen intentionally, then your changes are incorrect.

[HyunSangHan]

@juergba
Thank you very much for reviewing my PR.
With your advice, I reviewed the intentions of the original code writer.

1. I checked out the history of helpers.js.

   According to Refactor Reporter tests #3272, helpers.js was created in the process of refactoring the duplicated code of the test files in test/reporters.

   createRunnerFunction in helpers.js is composed entirely of switch-case statements. Depending on what runStr is, the functions to return are defined one by one. This means that createRunnerFunction is not meant to be reused, just a collection of similarly structured code for refactoring purposes.

   So I could conclude: the code I modified will not affect what I didn't expect, and it will work independently of each other. This is supported by the fact that when I ran the test with my code, it all passed.

2. Nevertheless, as you said, I thought more about the possibilities of the intended design.

   Compare the two cases below.

   (1) If only using if separately in succession

   if (event === ifStr1) {
     callback(a);
   }
   if (event === ifStr2) {
     callback(b);
   }

   (2) If using if else

   if (event === ifStr1) {
     callback(a);
   } else if (event === ifStr2) {
     callback(b);
   }

   Since the event does not change during this process, in most cases the codes mentioned above will do the same. It works differently only in one case that ifStr1 and ifStr2 are same. When ifStr1 and ifStr2 are same to each other, (1) executes both callback(a); and callback(b); while (2) only executes callback(a);.

   If the original author had intentionally designed something like this, it was probably because there were some situations where both callback(a); and callback(b); have to run, that is, ifStr1 and ifStr2 are equal.

   I've looked through every case using the createMockRunner in test/reporters/*.spec.js to see if those situations really exist.

   In most cases, the logic inside the case statement was just for event === ifStr1.

   Only five cases in the helpers.js deal with event === ifStr2, so I've looked at those five:

   • case 'start test'
   • case 'suite suite end'
   • case 'pass end'
   • case 'test end fail'
   • case 'fail end pass'.

   When I checked the use cases of createMockRunner for these five mentioned above, I found 17 times as below.

   case 'start test'

   // 1 time
   var runner = createMockRunner(
     'start test',
     EVENT_RUN_BEGIN,
     EVENT_TEST_BEGIN,
     null,
     test
   );

   // 2 times
   var runner = createMockRunner(
     'start test',
     EVENT_TEST_END,
     EVENT_TEST_PENDING,
     null,
     test
   );

   // 2 times
   var runner = createMockRunner(
     'start test',
     EVENT_TEST_END,
     EVENT_TEST_PASS,
     null,
     test
   );

   case 'suite suite end'

   // 1 time
   var runner = createMockRunner(
     'suite suite end',
     EVENT_SUITE_BEGIN,
     EVENT_SUITE_END,
     EVENT_RUN_END,
     expectedSuite
   );

   case 'pass end'

   // 1 time
   var runner = createMockRunner(
     'pass end',
     EVENT_TEST_PASS,
     EVENT_RUN_END,
     null,
     expectedTest
   );

   case 'test end fail'

   // 8 times
   var runner = createMockRunner(
     'test end fail',
     EVENT_TEST_END,
     EVENT_TEST_FAIL,
     null,
     test,
     error
   );

   case 'fail end pass'

   // 2 times
   var runner = createMockRunner(
     'fail end pass',
     EVENT_TEST_FAIL,
     EVENT_RUN_END,
     EVENT_TEST_PASS,
     test
   );

   But as you can see, the case that ifStr1 is the same as ifStr2 never appeared.

   Therefore, it can be assumed that the original author did not expect anything by writing if separately. That's why I think the original author didn't design it intentionally.

---

Now, you know that changing if, if to if, if else is no problem.

Once event === ifStr1 is satisfied, you don't have to check anymore for event === ifStr2 or event === ifStr3. By modifying it to if else, you can expect some efficiency.

• If a match was found before the third if, then at least the third if statement will be skipped.
• If a match was found in first, it will skip both second and third statements.

[juergba]

@HyunSangHan I can see you have put quite an effort in this PR, but I'm not convinced of its correctness.

• following your argumentation, createRunnerFunction's signature has been declared without intention and is therefore just a bad design. But with your changes - stopping at half way - the result is IMO even worse and more confusing.
• the last few months I made some changes to the uncaught exception handling. There are cases were ifStr1 === ifStr2, eg. the same test failing twice. So we have real szenarios where this signature may make sense. If our CI tests don't cover those szenarios, doesn't mean we should change this factory function.

[juergba]

@HyunSangHan I'm closing this PR, as explained in my review.
Thank you for your input.

[HyunSangHan]

@juergba
Okay. Thanks for explanation.