• LZ79LRU (unregistered)

    Assuming that the logic of the code is correct, which it probably is the only sin here is not wanting to use nested try / catch blocks with catch actually doing what it's supposed to. But to be fair there, we are all so used to catch being used only to log and rethrow that it is understandable if unfortunate that someone might feel anxiety about writing "productive" code in a catch block.

  • (nodebb)

    One possibility is that the Framework function sometimes returns null and sometimes throws an exception. Which would make the framework TRWTF.

  • (nodebb)

    The only reason I can think of for the two calls to get the image is the framework might be doing something that the first SHOULD work, but maybe too small of a timeout value makes the first call fail like 90% of the time, and the second call happens to use the cached data created by the first call, which, if allowed to run 10ms more, would allow the image to be received.

  • (nodebb)

    They first try to fetch a localized image, if that fails they try to get a default one and if that fails they just return a white Brush. The way I see it the exception isn't captured because it isn't relevant and the code is implemented with the implication that some or all requested images will be missing.

    The part that actually caught my attention was the "BorderChannelGroupBrush" variable which looks like it was an instance o maybe a static made local, probably as a result of a change in requirements.

  • Hal (unregistered)

    Honestly I don't hate this. It could be done better for sure; but the approach does reduce the branching (at the source code level anyway) which generally speaking is good for clarity and extensiblity.

    A structure of more deeply nested if, switch/case, try blocks might make it feel like the code is more expressive and better describes all possible stats, but that would likely be just that a feeling, and more likely would just create more potential corner cases and greater uncertainty about the actual behavior.

  • PedanticRobot (unregistered)

    If imageName doesn't have a dot, then LastIndexOf() will return -1, in which case imageName.Substring(0, -1) will throw an exception.

  • TheCPUWizard (unregistered)

    If the first attempt throws OR returns a null image.....

  • (nodebb) in reply to LZ79LRU

    I was thinking the same. If "boomerang code" is bad, "boomerang code in catch blocks" is worse.

  • (nodebb)

    At least it only took until the fourth post for someone to remark that it isn't two attempts to fetch the same image, but one for the localised image (frobble-en.jpg) and one for the non-localised image (frobble.jpg).

    The thing that makes my teeth itch, however, is that there is no way to see at runtime (when compiled in debug mode, naturally) that we failed to fetch both images.

    Even if it's just the equivalent of an ifdef'ed call to OutputDebugString in the catch blocks, it would be nice to have some indication that it failed, and even better, why.

    Addendum 2025-01-15 12:11: Note: the indication is to distinguish "we downloaded a white image" from "we failed totally to download an image"

  • (nodebb)

    I think this one is actually reflecting for the real WTF. First off, there's nothing really wrong with getting the index of the extension, especially since this doesn't seem to be some random images but some assets that get loaded into this obvious ancient WinForm app from a remote server.

    However ServicePageImageUrlOnContentServer is the weirdest method name I have ever read. It's just a collection of nouns. So this puts a lot into perspective and I think the main issue might even be found there. I guess this methods just doesn't do proper exception handling, fails with improper exceptions, falls through with null, is not deterministic, all the fun stuff.

    So yeah, this code will not get any beauty points for sure but heck would it be interesting to get a peek into this weird method.

    Addendum 2025-01-15 12:14: WPF app, not WinForm obviously.

  • (nodebb) in reply to LZ79LRU

    it is understandable if unfortunate that someone might feel anxiety about writing "productive" code in a catch block

    I try to avoid doing that because an uncaught exception in a catch block terminates the program immediately.

  • David-T (unregistered) in reply to Joe_D

    So catch them.

    An uncaught exception anywhere terminates your program (ignoring async/delegate weirdness).

  • Duke of New York (unregistered)

    Never apologize for coding linear logic linearly.

  • (nodebb)

    I think the idea of nested try-catch blocks is worse than what's implemented here. You don't use exceptions for determining path flow. What they really need is some sort of TryGet... method from "Framework", since they expect the localised file to not exist some of the time, instead of using a method that throws an exception in such cases. But perhaps that library is out of their hands.

  • (nodebb)

    The biggest problem I would complain of in this code is that it assumes that every possible exception is caused by the resource being absent rather than server or network failure. It's not too bad reacting to a 404 error by retrieving an alternative resource (though it's an unnecessary slowdown when it could ask for the correct resource directly), but hiding every other possible error is much worse. Given the mention of language, the right sort of failure will go undetected until someone posts a screenshot on thedailyWTF showing mixed or wrong language being displayed.

  • Loren Pechtel (unregistered)

    What we have here is fallback modes.

    Attempt to get the language-localized version of the image.

    If there's no localized version get the general version.

    And if you can't even get that you return white rather than failing. Completely reasonable behavior in the case of a renderer of some kind. Whether it should be done by nesting in the catch blocks or checking to see if you got something could be argued either way and doesn't rise to WTF levels to me.

Leave a comment on “Brushing Up”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article