Read on Twitter
Another week, another legacy project. Here are some things that I found in the code that you may want to avoid doing in your own projects.
Division by zero gets silenced. The result of the entire operation becomes false, which conveniently gets cast to zero as well.
Lessons:
- Check for a zero before dividing and make your own decision.
- Don& #39;t let PHP cast false/string/null to zero. Catch those values early.
Lessons:
- Check for a zero before dividing and make your own decision.
- Don& #39;t let PHP cast false/string/null to zero. Catch those values early.
A very large chunk of code inside a try, with the catch also failing because the $result hasn& #39;t been assigned a value yet.
- Don& #39;t let your catch rely on operations inside the try.
- Ideally, have only one line inside a try block. You can have an outer try/catch for the app.
- Don& #39;t let your catch rely on operations inside the try.
- Ideally, have only one line inside a try block. You can have an outer try/catch for the app.
A "unit" test that connects to another server.
- Unit tests shouldn& #39;t connect to servers/databases/filesystem.
- End-to-end tests can connect to mocked services. For example, you can use Wiremock in your Docker environment (tutorial: https://medium.com/@jw_ng/mocking-with-wiremock-and-docker-1f1601bd10e4).">https://medium.com/@jw_ng/mo...
- Unit tests shouldn& #39;t connect to servers/databases/filesystem.
- End-to-end tests can connect to mocked services. For example, you can use Wiremock in your Docker environment (tutorial: https://medium.com/@jw_ng/mocking-with-wiremock-and-docker-1f1601bd10e4).">https://medium.com/@jw_ng/mo...
Incorrect implementation of the topological sorting algorithm, which may cause incorrect calculations on billions of monetary transactions.
- Use existing packages for generic algorithms when possible. For topological sort, I used this: https://github.com/marcj/topsort.php">https://github.com/marcj/top...
- Use existing packages for generic algorithms when possible. For topological sort, I used this: https://github.com/marcj/topsort.php">https://github.com/marcj/top...
Someone copy-pasted the actual output of an integration test and put it in the assertion. But that value was wrong due to a bug. Once the bug was fixed, the test started failing.
- Expectations for a test& #39;s output should be hand-crafted. Otherwise, you can be endorsing bugs.
- Expectations for a test& #39;s output should be hand-crafted. Otherwise, you can be endorsing bugs.
Disregard for initialization:
- Pushing to an undefined array.
- Setting the property of an undefined object.
- Unsetting keys of an undefined array.
- Concatenating undefined variables.
- Multiplying by undefined variables.
- Pushing to an undefined array.
- Setting the property of an undefined object.
- Unsetting keys of an undefined array.
- Concatenating undefined variables.
- Multiplying by undefined variables.
A test failure is dependent on an unknown previous test in the same suite. This means that I have to run it for 27 minutes each time I want feedback.
- Each test much be independent.
- In addition to your E2E tests, also write unit tests to quickly pinpoint issues.
- Each test much be independent.
- In addition to your E2E tests, also write unit tests to quickly pinpoint issues.
An E2E test fails. The expected output is an XML containing 6,602 lines. That is a lot of output and even more lines of code to sift through.
Recommendation:
Create concise expectations, usually by only checking the few relevant tags, while validating the structure via XSD.
Recommendation:
Create concise expectations, usually by only checking the few relevant tags, while validating the structure via XSD.
An incorrectly mocked date in a test causes a credit card& #39;s validation to start failing. The expiry date was in May 2020, but last test execution was in Jan 2020.
Recommendation:
Check that you mock dates correctly by trying the test with a value that would fail without mocking.
Recommendation:
Check that you mock dates correctly by trying the test with a value that would fail without mocking.
A test fails because it expects 1.5600000000000001 instead of 1.56. The problem is that the expectation is based on a bug involving float arithmetics.
Recommendation:
Write your own expected value instead of blindly copying what the execution produced.
Recommendation:
Write your own expected value instead of blindly copying what the execution produced.
Catching the generic Exception, thinking than an inaccessible URL means "no taxes".
Recommendation:
- Catch only the exceptions that you expect in a given context.
- Anything unexpected should be caught higher up and treated as an error.
Recommendation:
- Catch only the exceptions that you expect in a given context.
- Anything unexpected should be caught higher up and treated as an error.
This other catch block is 300 lines long. I don& #39;t even think that I write classes that long.
Recommendation:
- Keep your try blocks short, ideally a single statement.
- Keep your methods short, ideally under 10 statements.
Recommendation:
- Keep your try blocks short, ideally a single statement.
- Keep your methods short, ideally under 10 statements.
Formatted numbers used in calculations, yielding incorrect results. https://3v4l.org/3JBUg
Recommendation:
Convert">https://3v4l.org/3JBUg&quo... from string to int/float before any operation meant for numbers. And don& #39;t just cast it. Remove thousands separator and such first.
Recommendation:
Convert">https://3v4l.org/3JBUg&quo... from string to int/float before any operation meant for numbers. And don& #39;t just cast it. Remove thousands separator and such first.
The version of PHP_CodeSniffer was not explicit in Vagrant, and so it installed the latest, incorrect one. I had to check all the tags on GitHub to match the version.
Recommendation:
- When writing Vagrant/Docker commands, specify versions.
- Commit project& #39;s composer.lock
Recommendation:
- When writing Vagrant/Docker commands, specify versions.
- Commit project& #39;s composer.lock
