A team member was having a bit of trouble with the following code:
The above code, redacted and simplified, gives you the idea of the problem. We have a database, an interim temp file to write to, and an upload process.
The issue was that the
upload call was just not working. How frustrating!
Eagle-eyed readers may spot that the temporary file, being written to by the
OutputStream inside the second function, hasn’t been closed when the
upload call is being made. That was the bug, and the fix was to move the call to
upload to outside of the try-with-resources block, which had the job of closing the stream, thus freeing up the file (on Windows at least, you can’t read a file that you’re also writing!).
The Bug Wasn’t That!
This was a bug that was hard to spot and was a consequence of a structural decision. The reason the upload call belonged outside of the export operation wasn’t just to ensure the stream was close, it was also because it was a different stage in the overall process. The process should have been:
- Create temp location
- Export into temp location
- Upload from temp location to destination
- Clean up temp location
The indentation here reflects the scoping.
The implementation above merges the two middle steps and even seemed to put the upload operation INSIDE the export. This was a structural bug.
There’s an argument that the above code, should have been written as three functions not two:
- One function to manage the temporary files and then call into…
- The next function that calls export, then upload
- The export function
This is a nice example of where some additional precision on getting the structure right would have reduced the likelihood of a hard-to-spot bug catching us out.