But Did It Really Succeed?



  • I never do contracts, but I took an extremely small one from a friend of a friend when I was between jobs.  The company was a very technically competent one so I thought it'd be no problem.  I wrote my little module.  It was only a couple of hundred lines of networking code (one C++ class), nothing fancy or difficult.  It worked well.  I commented generously including all parameters, return values, and error cases.  I included sample code and tests.  I followed their horrid-looking coding standards to the letter.  There was little that any reasonable person would find fault with.

    I was rather surprised to get my code bounced to me, and more so to be told that it was not checking error conditions.  Particularly because my code meticulously checks error conditions.  I check if every new returns NULL.  I check the return value of every function to make sure it succeeded, and if it failed handle the error and propagate it.

    But no, the manager found the one place I didn't handle errors - when a function call succeeded.  That is, if the function returned success, I didn't call WSAGetLastError() (the Windows equivalent of checking errno for networking calls) to see if something went wrong anyway.  I tried to explain to the manager that the error condition is not necessarily cleared on success.  In fact, it is usually not touched.  And besides, what's the point of testing it, the function signaled that it has succeeded?  I tried to provide quotes from the relevant documentation to back my case.

    Return Value:
    If no error occurs, socket returns a descriptor referencing the new socket. Otherwise, a value of INVALID_SOCKET is returned, and a specific error code can be retrieved by calling WSAGetLastError.

    The manager's solution to this: call WSASetLastError(0) before making the function call, clearing the error condition.  So now the code

    sock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); // this is from memory
    if(sock == INVALID_SOCKET)
    {
      // do some error handling
    }

    becomes

    WSASetLastError(0);
    sock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
    if(sock == INVALID_SOCKET || WSAGetLastError() != 0)
    {
      // do some error handling
    }

    I tried arguing that the code actually has a bug in it.  If socket() (or any of the numerous Winsock functions that I used) encountered and successfully handled an error while doing its thing, WSAGetLastError() would get us the error from that, even though a valid socket is returned (and now we are leaking sockets, though that is easily addressed).  And besides, it's absurd to check the error in case of success, the documentation is very clear on this.  The manager would have none of it.  When the number of lines of email written by me exceeded more than twice the number of lines in the code, I gave up.  I modified my submitted code to do what he requested, put a huge comment at the top of the code explaining the issue, my resistance to it, and who is responsible for it being in there.

    To this day, years later, it annoys me that somewhere out there is a piece of code written by me that has had bugs forced into it.



  • Gah!  I've seen the same sort of logic used with "errno" also.  The point of those variables/macros is to to give further information on an error already discovered.

    I'd have had two if statements, the one for real errors, and another that closed the socket correctly before grudgingly doing unneeded error handling.



  • God I hope I never have to work for someone like that. Just out of curiosity though, why would you be checking new for a return value of NULL? If new fails to allocate memory, it throws an exception doesn't it?



  • [quote user="joe_bruin"]

    I never do contracts, but I took an extremely small one from a friend of a friend when I was between jobs.  [/quote]

    Aaaaah. I see. This is the problem right here.

    If you were a professional Enterprise level contractor, you would have explained that, due to limitations in the existing library, you would need to wrap winsock to be able to handle all possible error conditions, especially those pesky errors that occur when there's nothing wrong, and then of course wrap the wrapper to handle all possible success condition errors that may occur in there, and so on.

    That way, any people who came along after would probably just use the Winsock Wrapper Wrapper Helper Enterprise Error Elimination Environment (WWWHEEEE) without reading the code, because at 643768871255000983 LOC, who would really want to? You'd look productive, earn big $$$ and if it was a closed source shop, you'd be able to mention it on your CV without anyone realising what an absolute dog it was!

    It's all about managing to con money out of idiots who don't know what they're asking expectation. Learn to leverage the synergy, baby!



  • [quote user="segfaults_are_fun"]Just out of curiosity though, why would you be checking new for a return value of NULL? If new fails to allocate memory, it throws an exception doesn't it?[/quote]

    You are correct, sir. It throws std::bad_alloc, or so this test code I just wrote tells me.  It's been years since I've done C++ and mine's gotten a little rusty (in fact, the above described was my last cpp job, and that was in the 20th century).  Since then it's been all C, where we don't have exceptions and we check our mallocs for NULL.



  • [quote user="segfaults_are_fun"] If new fails to allocate memory, it throws an exception doesn't it?[/quote]

     

    assuming you have exceptions enabled - i work with a lot of game code that doesn't (for obvious reasons)  



  • If I were you, I would have tried to figure out an actual case where the internal workings of socket would have and handle (successfully) and error, and pointed that out to the PHB.


Log in to reply