Are KB articles allowed to have poorly written code? (KB: 320348)

If you take a look at KB article 320348 you might think it was the holy grail if you were trying to figure out how to compare binary files using the .NET Framework.  However, the code isn't really written all that well and to the less astute programmer, it might go straight into production use without so much as a code change to make sure the routine worked properly.  So what am I worried about from this function?

1.  Not checking the null string case.  You really shouldn't pass a null string into a FileStream.  Just not good coding practice.
2.  Not checking for file existence before creating the FileStreams.  “Try“ing for exceptional programming they are (without the try).
3.  Not putting the FileStream's into a using statement or something that protects their closure if something goes wrong:
   (for instance, if FileStream1 exists and FileStream2 throws an exception FileStream1 stays open a while.)

I can live with the rest of the code, it doesn't seem all that bad.  But the question remains.  Should KB articles that attempt to create non-existent technology do so with the mindset of LCS (least cost solution) or should they adopt a best practices approach?  Should the code be usable out of the box, or should it simply be a technology sample that needs to be cleansed by the proper authorities before being used in an actual application?  I welcome any comments, negative or positive, since I'm really curious what other people think.

http://support.microsoft.com/default.aspx?scid=kb;EN-US;320348

Published Monday, January 26, 2004 5:54 PM by Justin Rogers
Filed under: ,

Comments

Monday, January 26, 2004 9:34 PM by Matt Berther

# re: Are KB articles allowed to have poorly written code? (KB: 320348)

Justin: I'm with you on this. I cant remember how many times I've seen sample code in the MSDN library where the samples don't even compile.

When I start looking for how to do something, I look towards Google rather than the MSDN, because I tend to find a lot better samples on the web.

As a last resort, Ill use an MSDN sample, but only as that. A sample. I wont cut and paste the code into my project as is.

It's really too bad, however, it has helped me to write better API documentation. :=)
Monday, January 26, 2004 9:34 PM by Edgar Sánchez

# They definetively shouldn't

Many programmers just copy and paste Microsoft samples, if the examples do something silly as using the sa SQL Server user, the programmer will feel authorized to do the same. So please go the "best practices" way.
Tuesday, January 27, 2004 3:00 AM by Scott

# re: Are KB articles allowed to have poorly written code? (KB: 320348)

Yeah, why should Wrox books have a monopoly on errata. I think MSDN should include MORE bad code.

Now that the MVP's are allowed to write KB articles willy nilly it's just going to get worse.

Tuesday, January 27, 2004 4:28 AM by Pavel Lebedinsky

# re: Are KB articles allowed to have poorly written code? (KB: 320348)

> 2. Not checking for file existence before creating the FileStreams.

This is actually the right thing to do. FileStream is documented to throw FileNotFoundException if the file does not exist, and this is exactly what a function like FileCompare should do anyway.

You could call File.Exists() but what would you do if it returns false? Throw FileNotFoundException? Then what about the case where the file exists but you do not have access to it? Or if the file is on a remote share and there's a network problem?

File.Exists is rarely useful in library code like this FileCompare function. It lumps all possible failures into a single bool value and it is prone to race conditions.
Tuesday, January 27, 2004 4:29 AM by Simon

# re: Are KB articles allowed to have poorly written code? (KB: 320348)

The KB above is a prime example, the code they show is to improve the use of getting XML from an ADO stream. However they use a classic perf issue of concatenating strings.
This code can be improved greatly using an array, but they don't want to change the KB.

So don't do as they say or as they do!
Tuesday, January 27, 2004 9:05 AM by Paul Menefee

# re: Are KB articles allowed to have poorly written code? (KB: 320348)

I've got to defend MS here, a bit. The point of the KB it demonstrate a particular code example. We harass them all the time about being verbose and for once they cut to the chase. And just tell us what we need to know. If I'm looking for a quick solution to a particular problem I don't need to see it wrapped in a bunch of production ready code. You know, and after giving this a little more thought perhaps that's their point. If they handed programmers half-baked code that forces the programmer to take ownership and wield the code ore attuned to their application.
Tuesday, January 27, 2004 9:20 AM by Derick Bailey

# re: Are KB articles allowed to have poorly written code? (KB: 320348)

I agree with Paul Menefee.

When I'm looking up code examples in a KB article or on another website, I don't want to see it all wrapped up in best practices... I want to know how it works and why. I don't care that they missed catching a filestream exception for a null string - I know to take care of that.

If every peice of sample code was obfuscated in production ready best practices code, noone would ever learn anything because all they would have to do is cut & paste from a hundred examples and press "play" in their IDE.

I think this is more a question of properly training developers. If your coworkers or employees are dumb enough to cut & paste sample code and expect it to be production ready, maybe you owe it to them to spend some time educating them on best practices and the difference between samples / production code.
Friday, January 30, 2004 5:55 PM by Justin Rogers

# re: Are KB articles allowed to have poorly written code? (KB: 320348)

Everyone focused on my assertion that file checking should exist, but that wasn't the biggest problem with the code. The largest problem is that their code paths left an open file stream if the first file opened but the second file didn't. I was pointing out ALL of the issues with the sample to demonstrate how many they were, and by pointing out that they left open file streams I also pointed out an extreme example. I honestly think you should thoroughly read the entire message before making comments on only part of it.

As it stands I'm just guessing everyone that defended MSDN here agrees that resources like file streams don't really need to be closed explicitly. And the fact that if you called this method twice before the GC kicked in you'd get an access violation on the file in question because it is already open.
Thursday, February 19, 2004 3:07 PM by Bill

# re: Are KB articles allowed to have poorly written code? (KB: 320348)

<<
Should KB articles that attempt to create non-existent technology do so with the mindset of LCS...
>>

In my opinion, kb articles, msdn docs and all of the sample code need to demonstrate their associated concepts. Production quality code is, in my opinion, not what these things should be expected to provide. They're not supposed to be teaching people how to write code. They aren't code repositories.

<<
This is actually the right thing to do.
>>

I disagree. The "right thing" to do is to validate input.

<<
As it stands I'm just guessing everyone that defended MSDN here agrees that resources like file streams don't really need...
>>

Your logic is flawed.

Leave a Comment

(required) 
(required) 
(optional)
(required)