Possibly flawed algorithm

[gggustafson]

Type of issue

Other (describe below)

Description

In the "namespace IncludeTag" example, I do not believe that:

 if ((a == int.MaxValue && b > 0) || 
        (b == int.MaxValue && a > 0))

is the correct algorithm. For integers I suggest:

const int MAXINT_DIV_10 = int.MaxValue / 10;
const int MAXINT_MOD_10 = int.MaxValue % 10;

if ( !( ( a < MAXINT_DIV_10 ) ||
   ( ( a = MAXINT_DIV_10 ) && 
   ( b <= MAXINT_MOD_10 ) ) ) )
{
    throw new System.OverflowException ( );
}

return ( a + b );

For doubles, the algorithm is somewhat the same.

Aside: How do I get indentation to work?

Page URL

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/xmldoc/examples

Content source URL

https://github.com/dotnet/docs/blob/main/docs/csharp/language-reference/xmldoc/examples.md

Document Version Independent Id

33d9f302-a3aa-a165-ec64-a4382feb093e

Article author

@BillWagner

Metadata

• ID: 60a08067-b91f-3c9a-0569-30d098089990
• Service: dotnet-csharp
• Sub-service: lang-reference

[BillWagner]

Hi @gggustafson

First the aside: If you fence code in triple-backslashes, GitHub will preserve formatting. I edited your text above as an example.

On to the main point:

The current code matches the description in the XML comments. I'd prefer keeping it the same.

[gggustafson]

What happens if both parameters have the value (MAXINT / 2 + 1)? You get an exception.

All I'm concerned about are newbies thinking that the algorithm is correct (I mean it's from MS!! Isn't it?)

[adegeo]

Hi Gus. I can see where you're coming from 😃 But I would say that the context that this code is shown in, isn't there to be an example of overflow detection nor computer science best practices, it's a simple demonstration of the documentation system. So yes, it's a flawed algorithm, but the algorithm isn't the focus of the article.

If a newbie grabbed a book on C off the shelf and there's a demonstration about doing some string operations, they may not see code guarding against every type of buffer overflow possibility, maybe just 1 or even none, depending on the context of the example. The same applies here, the context here doesn't warrant a good math library example, the math code is irrelevant except to show some simple code that can be talked about in the documentation comments above it. In this case, it shows to document the <exception> thrown by the method. None of the other methods even implement guard rails. Time, money, context, factor into how complex the example should be to demonstrate the concept.

---

In .NET, overflow checking is a little more complicated than just this (you may know all this, but I'm just explaining it for anyone else that runs across this issue). For example, if you're using C#, the code in the article example doesn't actually overflow, it's just an imposed overflow check. The following code runs fine and wraps the value to a negative number instead of throwing an exception:

int a = int.MaxValue / 2 + 1;
int b = int.MaxValue / 2 + 1;
Console.Write(a + b);

But if add checked in there and you'll get the exception raised automatically:

int a = int.MaxValue / 2 + 1;
int b = int.MaxValue / 2 + 1;
Console.Write(checked(a + b));

If you happen to be using VB, you'll get an exception raised by the first snippet and instead need to opt-out through a project setting.

Really the best way to do it would be to let the runtime handle it for you and force the mode you want. If you want wrapping, the .Add method would be doing return unchecked(a + b). If you wanted to throw the exception and disallow wrapping, you would do return checked(a + b).

[gggustafson]

I am of the opinion that all documentation needs to be correct. I am willing to accept that as an example of documentation, this example suffices. But there needs to be a warning. If no warning then the example should be rewritten.

[BillWagner]

I've added this to the backlog, and added the "help wanted" label.

I'm not against a different sample, as long as all the important doc elements are involved. However, I don't see making these updates as a priority.