[Mono-devel-list] RFC

[Ben Maurer]

> -			string defRelationName = "Relation";
> -			for (int index = 1; Contains (defRelationName); ++index) {
> +			int index = 1;
> +			string defRelationName = "Relation" +index;
> +			for (; Contains (defRelationName); ++index) {
>  				defRelationName = "Relation" + index;

Ok, thats just ugly and unreadable. In general, for loops should only be
used for:
      * for (int i = 0; i < count; i++)
      * for (Link l = head; l != null; l = l.Next)
Other than that, please just use while ().

> +			try{
> +				if (ignoreCase)
> +					ciProvider = new CaseInsensitiveHashCodeProvider(_dataTable.Locale);
> +			}
> +			catch (Exception e)
> +			{
> +				//TODO remove try catch block after CaseInsensitiveHashCodeProvider is implemented 
> +			}

We really should not be working around things like this. They tend to be
forgotten. Your best bet is to make a default impl of the insensitive
provider. Honestly, it does not matter if it can only handle a-z and
ignores all funky languages with accents, etc. Better to have a
workaround deep down that is easy to rip out and replace with good code
than tons of workarounds in our codebase that we must code for.

Also, some general coding style comments:

Mono's style is the following:

using System;
using System.IO;

namespace Mono.Blah.Foo {
	class Bar {
		int blah;
		int foo;
		
		void PrivateMethod ()
		{
			if (blah < 0)
				foo = 1;
			else
				foo = 0;

			if (foo + blah == 1) {
				PublicMethod (foo ++);
				blah --;
			}
		}

		public void PublicMethod ()
		{
			try {
				...
			} catch (IOException e) {
				...
			} catch (Exception e) {
				...
			}
		}
	}
}

Note how the braces for each statment are done, compare:
class Foo {
}

to

public void Foo ()
{
}

to

if (foo) {
}

You should try to keep to these styles.

Also, do a + b not a+b. Do a [foo], not a[foo]. foo () not foo().

When you make a pretty major change like this, it helps if you can send
a nice changelog, detailing the specific changes made to each method.
This helps a reviewer (like myself) focus on what you are trying to do.

Also, adding comments to new code is a great idea, always strive to have
the cleanest code in the file you are changing.

A bugfix like this should have an NUnit test to go along, and you should
make sure it passes in MS. Again, even if the test coverage is not 100%
for the existing code, try to cover yourself 100% (it helps us not blame
you).

I realize the coding style stuff is not fun, but it is hard to work
together on cvs without a unified coding style.

-- Ben