Skip to content

Reduce unnecessary condition determination #4197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Reduce unnecessary condition determination #4197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2c21e61
Change 'if' to 'else if'
HyunSangHan Feb 29, 2020
4c7175b
Minimize number of 'if' statements
HyunSangHan Feb 29, 2020
974b064
Fix runStr parameter to improve unity
HyunSangHan Mar 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 6 additions & 17 deletions test/reporters/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,7 @@ function createRunnerFunction(runStr, ifStr1, ifStr2, ifStr3, arg1, arg2) {
return function(event, callback) {
if (event === ifStr1) {
callback();
}
if (event === ifStr2) {
} else if (event === ifStr2) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@HyunSangHan HyunSangHan Mar 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

callback(test);
}
};
Expand All @@ -94,11 +93,7 @@ function createRunnerFunction(runStr, ifStr1, ifStr2, ifStr3, arg1, arg2) {
return function(event, callback) {
if (event === ifStr1) {
callback(expectedSuite);
}
if (event === ifStr2) {
callback();
}
if (event === ifStr3) {
} else if (event === ifStr2 || event === ifStr3) {
callback();
}
};
Expand All @@ -107,19 +102,17 @@ function createRunnerFunction(runStr, ifStr1, ifStr2, ifStr3, arg1, arg2) {
return function(event, callback) {
if (event === ifStr1) {
callback(test);
}
if (event === ifStr2) {
} else if (event === ifStr2) {
callback();
}
};
case 'test end fail':
case 'end fail':
test = arg1;
var error = arg2;
return function(event, callback) {
if (event === ifStr1) {
callback();
}
if (event === ifStr2) {
} else if (event === ifStr2) {
callback(test, error);
}
};
Expand All @@ -128,11 +121,7 @@ function createRunnerFunction(runStr, ifStr1, ifStr2, ifStr3, arg1, arg2) {
test = arg1;
if (event === ifStr1) {
callback(test, {});
}
if (event === ifStr2) {
callback(test);
}
if (event === ifStr3) {
} else if (event === ifStr2 || event === ifStr3) {
callback(test);
}
};
Expand Down
16 changes: 8 additions & 8 deletions test/reporters/tap.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ describe('TAP reporter', function() {
message: expectedErrorMessage
};
var runner = createMockRunner(
'test end fail',
'end fail',
EVENT_TEST_END,
EVENT_TEST_FAIL,
null,
Expand Down Expand Up @@ -163,7 +163,7 @@ describe('TAP reporter', function() {
stack: expectedStack
};
var runner = createMockRunner(
'test end fail',
'end fail',
EVENT_TEST_END,
EVENT_TEST_FAIL,
null,
Expand Down Expand Up @@ -194,7 +194,7 @@ describe('TAP reporter', function() {
message: expectedErrorMessage
};
var runner = createMockRunner(
'test end fail',
'end fail',
EVENT_TEST_END,
EVENT_TEST_FAIL,
null,
Expand Down Expand Up @@ -230,7 +230,7 @@ describe('TAP reporter', function() {
var test = createTest();
var error = {};
var runner = createMockRunner(
'test end fail',
'end fail',
EVENT_TEST_END,
EVENT_TEST_FAIL,
null,
Expand Down Expand Up @@ -390,7 +390,7 @@ describe('TAP reporter', function() {
message: expectedErrorMessage
};
var runner = createMockRunner(
'test end fail',
'end fail',
EVENT_TEST_END,
EVENT_TEST_FAIL,
null,
Expand Down Expand Up @@ -430,7 +430,7 @@ describe('TAP reporter', function() {
stack: expectedStack
};
var runner = createMockRunner(
'test end fail',
'end fail',
EVENT_TEST_END,
EVENT_TEST_FAIL,
null,
Expand Down Expand Up @@ -464,7 +464,7 @@ describe('TAP reporter', function() {
message: expectedErrorMessage
};
var runner = createMockRunner(
'test end fail',
'end fail',
EVENT_TEST_END,
EVENT_TEST_FAIL,
null,
Expand Down Expand Up @@ -504,7 +504,7 @@ describe('TAP reporter', function() {
var test = createTest();
var error = {};
var runner = createMockRunner(
'test end fail',
'end fail',
EVENT_TEST_END,
EVENT_TEST_FAIL,
null,
Expand Down