[Mono-devel-list] Patch for System.Data.Common and System.Data.ProviderBase

[Boris Kirzner]

Hello Suresh and Uma,
Thanks for your reply

Coding style : you're right. Sorry for the mistakes, I'll try to keep 
the code as close as possible to the guidelines.

#if NET_2_0 : Added to code not use in PROVIDER_JVM. It was not clear 
enough from the changelog and I'll fix it.

DbParameterBase.Direction.get : you're right. It's better to initialize 
the variable, exactly as it was before.

DbParameterBase.Parent : used by DbParameterCollectionBase to track 
collection ownership on parameters (for example it should be impossible 
to add the same parameter to two different collections simultaneously). 
I choose to implement this through internal property rather that 
internal variable.

Private variables naming : since code guide lines do no define this 
well, I did the changes for the following reasons :
_paramValue -> _value and _name -> _parameterName : keep private member 
name close to property name, so the code will be more readable.
adding  _ to parameter names : to enable easy recognize of an errors like :

int myProperty;

public int MyProperty
{
    get { return MyProperty; }
}

I found a number of such an errors in both System.Data and 
DirectoryServices.

 From the other side I understand the value of keeping minimal code 
changes. It's up to you - I can stay on my proposed names if you thing 
the reasons are good enough or revert them if you want so.

Thanks,
Boris


Sureshkumar T wrote:

>>Index: DbParameterBase.cs
>>===================================================================
>>--- DbParameterBase.cs	(revision 44908)
>>+++ DbParameterBase.cs	(working copy)
>>@@ -4,6 +4,7 @@
>> // Author:
>> //   Sureshkumar T (tsureshkumar at novell.com)
>> //   Tim Coleman (tim at timcoleman.com)
>>+//   Boris Kirzner <borisk at mainsoft.com>
>> //
>> // Copyright (C) Tim Coleman, 2003
>> //
>>@@ -30,27 +31,30 @@
>> // WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>> //
>> 
>>-#if NET_2_0
>>+#if NET_2_0 || TARGET_JVM
>> 
>> using System.Data.Common;
>> 
>> namespace System.Data.ProviderBase {
>> 	public abstract class DbParameterBase : DbParameter
>> 	{
>>+		#region Fields
>> 
>>-                #region Fields
>>    
>>
>
>I guess this is a whitespace changes. 
>
>  
>
>>-                string _name;
>>-                ParameterDirection _direction = ParameterDirection.Input;
>>-                bool _isNullable = false;
>>+		string _parameterName;
>>    
>>
>
>not accepted. mere naming change. 
>
>  
>
>> 		int _size;
>>+#if NET_2_0
>> 		byte _precision;
>> 		byte _scale;
>>-                object _paramValue;
>>-                int _offset;
>> 		DataRowVersion _sourceVersion;
>>+#endif
>>+		object _value;
>>    
>>
>
>not accepted. mere naming change. 
>
>  
>
>>+		ParameterDirection _direction;
>>+		bool _isNullable;
>>+		int _offset;
>> 		string _sourceColumn;
>>+		DbParameterCollection _parent = null;
>> 
>>-                #endregion // Fields
>>+		#endregion // Fields
>> 
>> 		#region Constructors
>> 	
>>@@ -59,11 +63,19 @@
>> 		{
>> 		}
>> 
>>-		[MonoTODO]
>> 		protected DbParameterBase (DbParameterBase source)
>> 		{
>>+			if (source == null) {
>>+				throw ExceptionHelper.ArgumentNull("source");
>>    
>>
>
>this example, where mono's coding guidelines is not followed. refer
>bottom of this message.
>
>  
>
>>+			}
>>+
>>+			source.CopyTo(this);
>>+			ICloneable cloneable = source._value as ICloneable;
>>+			if (cloneable != null) 	{
>>+				_value = cloneable.Clone();
>>+			}
>> 		}
>>-
>>+        
>> 		#endregion // Constructors
>> 
>> 		#region Properties
>>@@ -73,9 +85,29 @@
>> 			get { throw new NotImplementedException (); }
>> 		}
>> 
>>-                public override ParameterDirection Direction {
>>-			get { return _direction; }
>>-			set { _direction = value; }
>>+		public override ParameterDirection Direction {
>>+			get {
>>+				if (_direction == ((ParameterDirection) 0)) {
>>+						return ParameterDirection.Input;
>>+				}
>>    
>>
>
>what is this check for? revert to previous default assignment of
>ParameterDirection.Input.
>
>  
>
>>+				return _direction;
>>+			}
>>+			set {
>>+				if (_direction != value) {
>>+					switch (value) {
>>+							case ParameterDirection.Input:
>>+							case ParameterDirection.Output:
>>+							case ParameterDirection.InputOutput:
>>+							case ParameterDirection.ReturnValue:
>>+							{
>>+								PropertyChanging();
>>+								_direction = value;
>>+								return;
>>+							}
>>+					}
>>+					throw ExceptionHelper.InvalidParameterDirection(value);
>>+				}
>>+			
>>    
>>
>
>By the property declaration, this property cannot be set of any value
>other than of type ParameterDirection. These exception handling are
>irrelevant. Or am I missing something?
>
>  
>
>>}		}
>> 
>>+		internal DbParameterCollection Parent
>>+		{
>>+			get { return _parent; }
>>+			set { _parent = value; }
>>+		}
>>+
>>    
>>
>
>what is the need for this parent reference? is the internal type
>justified?  Avoid using internal as far as possible as it complicates
>the design. Instead, write a internal method wherever you want to access
>internal data. i.e. add a public behavior to manipulate a state rather
>the state itself.
>
>  
>
>>+	* DbParameterBase.cs
>>+		- Private members names changed to suite naming conventions.
>>    
>>
>
>where did you get this naming convetion? I cannot accept changing _name
>to _parameterName and terming it as a naming convention. _name inside a
>DbParameterBase is always the Parameter's name. I don't see any
>advantages of this kind of convention. Please follow whatever defined as
>coding guidelines and if you want to add additional coding guidelies,
>please propose so that others could follow if good.
>
>it would be nice if you follow mono's coding guidelines, as many places
>they are not followed.
>
>  
>
>>+		- Implemented copy ctor.
>>+		- Reimplemented Direction, ParameterName, Size and SourceColumn properties.
>>+		- Added #ifdef NET_2_0 on methods not used in NET_1_1
>>    
>>
>
>is it the whole purpose? didn't you miss the NET_2_0 spec on top of the
>file.
>
>suresh
>  
>

-- 
Boris Kirzner
Mainsoft Corporation
http://www.mainsoft.com