Re: [Mono-devel-list] Patch for System.Data.Common and System.Data.ProviderBase
Hello all
Attached is the reworked patch.
I left the _names as is, but if you insist, I'll change them.
Thanks,
Boris
Sureshkumar T wrote:
I'll try to keep the code as close as possible to the guidelines.
Thanks.
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.
Ok.
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.
Ok, Understandable. But, name for a parameter is always parameter's
name. anyway, if you feel it is necessary, change them.
adding _ to parameter names : to enable easy recognize of an errors like :
int myProperty;
public int MyProperty
{
get { return MyProperty; }
}
here, anyway, we use CamelCase for properties and first-letter-small for
private members. we could have got the error by seeing M in return
statements.
Please feel free to post the reworked patch.
Thanks,
suresh.
--
Boris Kirzner
Mainsoft Corporation
http://www.mainsoft.com
Index: DbDataAdapter.cs
===
--- DbDataAdapter.cs(revision 44908)
+++ DbDataAdapter.cs(working copy)
@@ -889,7 +889,7 @@
break;
case UpdateStatus.ErrorsOccurred :
if (argsUpdating.Errors ==
null) {
- argsUpdating.Errors =
new DataException(RowUpdatedEvent: Errors occurred; no additional is
information available.);
+ argsUpdating.Errors =
ExceptionHelper.RowUpdatedError();
}
row.RowError +=
argsUpdating.Errors.Message;
if (!ContinueUpdateOnError) {
@@ -902,14 +902,14 @@
updateCount++;
continue;
default :
- throw new
ArgumentException(String.Format(Invalid UpdateStatus:
{0},argsUpdating.Status));
+ throw
ExceptionHelper.InvalidUpdateStatus(argsUpdating.Status);
}
command = argsUpdating.Command;
IDataReader reader = null;
try {
if (command == null) {
- throw new
InvalidOperationException(ADP_UpdateRequiresCommand + command);
+ throw
ExceptionHelper.UpdateRequiresCommand(commandName);
}
CommandBehavior commandBehavior =
CommandBehavior.Default;
@@ -999,7 +999,7 @@
break;
case UpdateStatus.ErrorsOccurred:
if (updatedArgs.Errors == null) {
-
updatedArgs.Errors = new DataException(RowUpdatedEvent: Errors occurred; no
additional is information available.);
+
updatedArgs.Errors = ExceptionHelper.RowUpdatedError();
}
row.RowError +=
updatedArgs.Errors.Message;
if
(!ContinueUpdateOnError) {
Index: ChangeLog
===
--- ChangeLog (revision 44908)
+++ ChangeLog (working copy)
@@ -1,3 +1,8 @@
+2005-05-23 Boris Kirzner [EMAIL PROTECTED]
+ * DbCommand.cs - added #ifdef NET_2_0 on DbCommandOptionalFeatures (not
used in TARGET_JVM).
+ * ExceptionHelper.cs - removed java references. Exceptions created on
formatted text messages. Code styling fixes.
+ * DbParameterCollection.cs - implemented indexer properties.
+
2005-05-20 Umadevi S [EMAIL PROTECTED]
* Added file DbProviderSpecificTypePropertyAttribute.cs
Index: DbCommand.cs
===
--- DbCommand.cs(revision
Re: [Mono-devel-list] Patch for System.Data.Common and System.Data.ProviderBase
Hi Boris. Nice to see that we are trying to get the providebase completed. I saw Suresh's reply to the DbParameter.cs class. I guess most of the comments hold for the DbParameterCollection.cs class also.. If we are writing up code separately and then merging them, I think we need to be more careful. Even in the previous index redesign checkin we had many such scenarios(Suresh's comments) in files. But since we were merging lot of code we didnt want to burden you guys with additionally redoing only what is required.. In future, is it possible that mainsoft has the same code as SVN? This way we would avoid the merge, and the problems emerging out of it.. Also in most parts of the code, you would have noticed that we stick to guidelines or whatever is used in the file is very close the guidelines Given that we have many people contributing to it, it is easier to maintain the code if we leave the code style and convention in every file the way it is, unless it very bad.. Also note, most of the mono class libraries doesnot use the _ convention for private variables.. Regards Uma Boris Kirzner [EMAIL PROTECTED] 05/23/05 9:41 PM Hello all Attached is a proposed patch for ProviderBase that implements some additional functionality for provider base classes. It also required me to made a slight changes in System.Data.Common. Additional change is adding ExcptionHelper class. This work done towards move of Mainsoft codebase to SVN. Please respond with your comments before this patch is committed. Boris -- Boris Kirzner Mainsoft Corporation http://www.mainsoft.com ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-devel-list] Patch for System.Data.Common and System.Data.ProviderBase
+ 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?
I was wrong. Any int value can be casted to this property. so the
exception handling is necessary.
Why not assigning ParameterDirection.Input by default to _direction
rather than checking with 0 and returning it back?
Thanks Best Regards,
suresh.
___
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-devel-list] Patch for System.Data.Common and System.Data.ProviderBase
I'll try to keep the code as close as possible to the guidelines.
Thanks.
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.
Ok.
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.
Ok, Understandable. But, name for a parameter is always parameter's
name. anyway, if you feel it is necessary, change them.
adding _ to parameter names : to enable easy recognize of an errors like :
int myProperty;
public int MyProperty
{
get { return MyProperty; }
}
here, anyway, we use CamelCase for properties and first-letter-small for
private members. we could have got the error by seeing M in return
statements.
Please feel free to post the reworked patch.
Thanks,
suresh.
___
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-devel-list] Patch for System.Data.Common and System.Data.ProviderBase
Hello Uma, You are absolutely right: having the same code base is very important. We think exactly like you do and all this effort is made in order to have the same code base. ProviderBase is the last namespace which we merge. Very soon after that we will add our private connected mode implementation into .jvm folders and completely switch our development to SVN. The code styling is a very important issue. Good style just prevents bugs and makes development faster and easier. Any notes about bad code styling in our patches are welcome and will be fixed immediately. Regarding the _ prefix, well, I think omitting it IS a very bad style. Just because we already solved bugs (not in System.Data) related to misuse of private fields. The fact it's not in use everywhere in mono is bad in itself. We strive for better in System.Data, like Suresh said me :-). In addition, on this occasion I would like to talk about reformatting of the code. There are many places where the files are DOS styled, spaces used instead of tabs etc. It just makes the maintainance more difficult, IDEs driving crazy, diffs and patches not working... I think that if we can make one special effort for fixing that, without changing the logic, we all will benefit in the long term. Regards, Konstantin Triger S Umadevi wrote: Hi Boris. Nice to see that we are trying to get the providebase completed. I saw Suresh's reply to the DbParameter.cs class. I guess most of the comments hold for the DbParameterCollection.cs class also.. If we are writing up code separately and then merging them, I think we need to be more careful. Even in the previous index redesign checkin we had many such scenarios(Suresh's comments) in files. But since we were merging lot of code we didnt want to burden you guys with additionally redoing only what is required.. In future, is it possible that mainsoft has the same code as SVN? This way we would avoid the merge, and the problems emerging out of it.. Also in most parts of the code, you would have noticed that we stick to guidelines or whatever is used in the file is very close the guidelines Given that we have many people contributing to it, it is easier to maintain the code if we leave the code style and convention in every file the way it is, unless it very bad.. Also note, most of the mono class libraries doesnot use the _ convention for private variables.. Regards Uma Boris Kirzner [EMAIL PROTECTED] 05/23/05 9:41 PM Hello all Attached is a proposed patch for ProviderBase that implements some additional functionality for provider base classes. It also required me to made a slight changes in System.Data.Common. Additional change is adding ExcptionHelper class. This work done towards move of Mainsoft codebase to SVN. Please respond with your comments before this patch is committed. Boris ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-devel-list] Patch for System.Data.Common and System.Data.ProviderBase
Just a word about _ prefix.
When you review a patch, it's much easier to catch errors when there is
a clear difference between local variables and class fields.
For example if you have see something like that:
xxx = 6;
Don't you want to know exactly that the class state was not changed?
Regards,
Konstantin Triger
Sureshkumar T wrote:
I'll try to keep the code as close as possible to the guidelines.
Thanks.
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.
Ok.
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.
Ok, Understandable. But, name for a parameter is always parameter's
name. anyway, if you feel it is necessary, change them.
adding _ to parameter names : to enable easy recognize of an errors like :
int myProperty;
public int MyProperty
{
get { return MyProperty; }
}
here, anyway, we use CamelCase for properties and first-letter-small for
private members. we could have got the error by seeing M in return
statements.
Please feel free to post the reworked patch.
Thanks,
suresh.
___
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list
___
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-devel-list] Patch for System.Data.Common and System.Data.ProviderBase
Hi Kosta, I don't think there is a corresponding term for this '_' matter in our coding guideline. The absence of rule sometimes implies that it was not ruled because it should not be ruled. Sometimes we can infer rules from existing sources. If we haven't prefixed '_' for fields, then that's not kind of rule in our long coding history. It is still possible to create further rules in the future, but for now there is another rule that should be applied here: If you are modifying someone else's code, try to keep the coding style similar. (excerpt from http://www.mono-project.com/Coding_Guidelines ) I don't think having '_' everywhere rule is enforceable. Some people want to maintain field names equivalent to that of MS.NET to enable runtime serialization interoperability. Atsushi Eno Konstantin Triger wrote: Hello Uma, You are absolutely right: having the same code base is very important. We think exactly like you do and all this effort is made in order to have the same code base. ProviderBase is the last namespace which we merge. Very soon after that we will add our private connected mode implementation into .jvm folders and completely switch our development to SVN. The code styling is a very important issue. Good style just prevents bugs and makes development faster and easier. Any notes about bad code styling in our patches are welcome and will be fixed immediately. Regarding the _ prefix, well, I think omitting it IS a very bad style. Just because we already solved bugs (not in System.Data) related to misuse of private fields. The fact it's not in use everywhere in mono is bad in itself. We strive for better in System.Data, like Suresh said me :-). In addition, on this occasion I would like to talk about reformatting of the code. There are many places where the files are DOS styled, spaces used instead of tabs etc. It just makes the maintainance more difficult, IDEs driving crazy, diffs and patches not working... I think that if we can make one special effort for fixing that, without changing the logic, we all will benefit in the long term. Regards, Konstantin Triger ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-devel-list] Patch for System.Data.Common and System.Data.ProviderBase
Index: DbParameterBase.cs
===
--- DbParameterBase.cs(revision 44908)
+++ DbParameterBase.cs(working copy)
@@ -4,6 +4,7 @@
// Author:
// Sureshkumar T ([EMAIL PROTECTED])
// Tim Coleman ([EMAIL PROTECTED])
+// Boris Kirzner [EMAIL PROTECTED]
//
// 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
