Return to Answer

Somewhat heavy-handed grammar & typo cleanup.

edited Oct 21, 2015 at 23:15

1.3k
10
15

Test review

It is not clear from yourthe code, in your upper code block, whether you are doing three separate tests, or if that is one test method doing three assertions.

Any wayAnyway it is common to strive for that one test method has only one assertion. Having multiple tests in one test method is considered a bad test regime.

Using Assert.Fail() is also somewhat uncommon. It would be better to use Assert.IsTrue() or Assert.IsFalse() and move the condition into the actual assert.

Finally, regarding the assertions, I'm not thrilled about your texts. What information do you get from "Extra error shown" or "Error 1 text"? Let the test cases and texts be clear and informative, so that you know what actions to take if it fails at some point in time.

Style review

First of all, Dmitry has some excellent point regarding refactoring in his answer. My comments, are more on the existing code in your lower block:

Not good toDon't mix brace styles – In some cases you have braces around the if blocks, and some places you don't. You are much better off using braces always! Even though some consider this a deceasedisease, braceophilia, you will save your selfyourself time and trouble always including the braces
Clean up indentation of your code – Your code is very confusing when looking at the indentation. This is both a reason for you to include braces (which would clearly indicate blocks), and it will (with a decent editor) help you get the indentation correct.

As it standstands, it is confusing which level of the if statement the Count and/or Displayed statements are at. They should be at distinct indentation levels.
Not custom to have aAvoid negated functionfunctions – There is a reason why the ! operator existexists, and duplicating your logic like you've done is a code smell. Keep one of them, and make that one correct and efficient, and use the ! operator to get the opposite version. (Or in some rare cases include the negated version, but let it call the ordinary version (as suggested by Dmitry))
Don't use if blocks to return the condition – This is already covered by Dmitry, but instead of doing if (someCondition) { return true; } else { return false; }, do simplify it to return someCondition;

Test review

It is not clear from your code, in your upper code block, whether you are doing three separate tests, or if that is one test method doing three assertions.

Any way it is common to strive for that one test method has only one assertion. Having multiple tests in one test method is considered a bad test regime.

Using Assert.Fail() is also somewhat uncommon. It would be better to use Assert.IsTrue() or Assert.IsFalse() and move the condition into the actual assert.

Style review

First of all, Dmitry has some excellent point regarding refactoring in his answer. My comments, are more on the existing code in your lower block:

Not good to mix brace styles – In some cases you have braces around the if blocks, and some places you don't. You are much better off using braces always! Even though some consider this a decease, braceophilia, you will save your self time and trouble always including the braces
Clean up indentation of your code – Your code is very confusing when looking at the indentation. This is both a reason for you to include braces (which would clearly indicate blocks), and it will (with a decent editor) help you get the indentation correct.

As it stand, it is confusing which level of the if statement the Count and/or Displayed statements are at. They should be at distinct indentation levels.
Not custom to have a negated function – There is a reason why the ! operator exist, and duplicating your logic like you've done is a code smell. Keep one of them, and make that one correct and efficient, and use the ! operator to get the opposite version. (Or in some rare cases include the negated version, but let it call the ordinary version (as suggested by Dmitry))
Don't use if blocks to return the condition – This is already covered by Dmitry, but instead of doing if (someCondition) { return true; } else { return false; }, do simplify it to return someCondition;

Test review

It is not clear from the code in your upper code block, whether you are doing three separate tests, or if that is one test method doing three assertions.

Anyway it is common to strive for that one test method has only one assertion. Having multiple tests in one test method is considered a bad test regime.

Using Assert.Fail() is also somewhat uncommon. It would be better to use Assert.IsTrue() or Assert.IsFalse() and move the condition into the actual assert.

Style review

First of all, Dmitry has some excellent point regarding refactoring in his answer. My comments are more on the existing code in your lower block:

Don't mix brace styles – In some cases you have braces around the if blocks, and some places you don't. You are much better off using braces always! Even though some consider this a disease, braceophilia, you will save yourself time and trouble always including the braces
Clean up indentation of your code – Your code is very confusing when looking at the indentation. This is both a reason for you to include braces (which would clearly indicate blocks), and it will (with a decent editor) help you get the indentation correct.

As it stands, it is confusing which level of the if statement the Count and/or Displayed statements are at. They should be at distinct indentation levels.
Avoid negated functions – There is a reason why the ! operator exists, and duplicating your logic like you've done is a code smell. Keep one of them, and make that one correct and efficient, and use the ! operator to get the opposite version. (Or in some rare cases include the negated version, but let it call the ordinary version (as suggested by Dmitry))
Don't use if blocks to return the condition – This is already covered by Dmitry, but instead of doing if (someCondition) { return true; } else { return false; }, simplify it to return someCondition;

Source Link

answered Oct 21, 2015 at 22:12

holroy

answered Oct 21, 2015 at 22:12

holroy

11.8k
1
27
59

Test review

It is not clear from your code, in your upper code block, whether you are doing three separate tests, or if that is one test method doing three assertions.

Any way it is common to strive for that one test method has only one assertion. Having multiple tests in one test method is considered a bad test regime.

Using Assert.Fail() is also somewhat uncommon. It would be better to use Assert.IsTrue() or Assert.IsFalse() and move the condition into the actual assert.

Style review

First of all, Dmitry has some excellent point regarding refactoring in his answer. My comments, are more on the existing code in your lower block:

Not good to mix brace styles – In some cases you have braces around the if blocks, and some places you don't. You are much better off using braces always! Even though some consider this a decease, braceophilia, you will save your self time and trouble always including the braces
Clean up indentation of your code – Your code is very confusing when looking at the indentation. This is both a reason for you to include braces (which would clearly indicate blocks), and it will (with a decent editor) help you get the indentation correct.

As it stand, it is confusing which level of the if statement the Count and/or Displayed statements are at. They should be at distinct indentation levels.
Not custom to have a negated function – There is a reason why the ! operator exist, and duplicating your logic like you've done is a code smell. Keep one of them, and make that one correct and efficient, and use the ! operator to get the opposite version. (Or in some rare cases include the negated version, but let it call the ordinary version (as suggested by Dmitry))
Don't use if blocks to return the condition – This is already covered by Dmitry, but instead of doing if (someCondition) { return true; } else { return false; }, do simplify it to return someCondition;

lang-cs