Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java

[Scott Gray]

Good point, but it would be better to do this:
if (...) {
} else if (...) {
} else if (...) {
} else {
    Debug.logWarning(...);
}
return false;

And about changing:
if (value == null) return true;


Regards
Scott

On 25/11/2009, at 12:06 AM, Jacques Le Roux wrote:

personally, I don't have a problem with (and actually prefer) using a single line for simple statements.

I see your point now (that's why I asked was not sure what you meaned). But we need to differentiate known from unknown, so I propose rather

Index: framework/base/src/org/ofbiz/base/util/ObjectType.java
===================================================================

@@ -754,25 +754,36 @@
   }

   public static boolean isEmpty(Object value) {
-        if (value == null) return true;
+        if (value == null) {
+            return true;
+        }

       if (value instanceof String) {
           if (((String) value).length() == 0) {
               return true;
+            } else {
+         return false;
           }
       } else if (value instanceof Collection) {
           if (((Collection<?>) value).size() == 0) {
               return true;
+            } else {
+         return false;
           }
       } else if (value instanceof Map) {
           if (((Map<?,?>) value).size() == 0) {
               return true;
+            } else {
+         return false;
           }
       } else if (value instanceof CharSequence) {
           if (((CharSequence) value).length() == 0) {
               return true;
+            } else {
+         return false;
           }
       }

       return false;
   }

Jacques


To: <dev@ofbiz.apache.org>
Sent: Tuesday, November 24, 2009 11:44 AM

--- framework/base/src/org/ofbiz/base/util/ObjectType.java (revision 883647) +++ framework/base/src/org/ofbiz/base/util/ObjectType.java (working copy) + Debug.logWarning("In ObjectType.isEmpty(Object) returning false for unknown not-null Object.", module); ----- Original Message ----- From: "Scott Gray" <scott.g...@hotwaxmedia.com > Subject: Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/src/ org/ofbiz/entity/util/EntitySaxReader.java

Wrong spot, it needs to go just before the end of the method:

return false;

Regards
Scott

On 24/11/2009, at 11:33 PM, Jacques Le Roux wrote:

Debug.logWarning("In ObjectType.isEmpty(Object) returning false for unknown not-null Object.");

Hi Scott,



Index: framework/base/src/org/ofbiz/base/util/ObjectType.java
===================================================================

@@ -754,7 +754,10 @@
  }

  public static boolean isEmpty(Object value) {
-        if (value == null) return true;
+        if (value == null) {
+            Debug.logWarning("a warning", module);
+            return true;
+        }

Jacques


From: "Scott Gray" <scott.g...@hotwaxmedia.com>

What do you think about adding this check at the beginning of ObjectType.isEmpty() something like --- framework/base/src/org/ofbiz/base/util/ObjectType.java (revision 883647) +++ framework/base/src/org/ofbiz/base/util/ObjectType.java (working copy)

Thanks Jacques, it wasn't a compilation problem but a run- install problem, empty fields were being treated as empty strings instead of null which was causing all sorts of fk problems during data load.

Regards
Scott

On 24/11/2009, at 9:49 PM, Jacques Le Roux wrote:

If you do work on UtilValidate it might be a good idea to log a warning whenever an object is passed in that we can only do a null check on, it might help people avoid this sort of issue and help us identify any areas where the checks aren't doing what we probably intended them to do.

Thanks Scott,



Jacques

From: <lekt...@apache.org>

I did not notice build issues. I will double check possible CharSequence uses or rather coonsider implementing UtilValidate.is(Not)Empty for it (I can't see any reason to not do it at this stage)

Author: lektran
Date: Tue Nov 24 07:43:18 2009
New Revision: 883613

URL: http://svn.apache.org/viewvc?rev=883613&view=rev
Log:


Modified:



URL:
http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java?rev=883613&r1=883612&r2=883613&view=diff

@@ -375,7 +375,7 @@

     if (currentValue != null) {
         if (currentFieldName != null) {

@@ -499,7 +499,7 @@
             CharSequence name = attributes.getLocalName(i);
             CharSequence value = attributes.getValue(i);

-                if (UtilValidate.isEmpty(name)) {
+                if (name == null || name.length() == 0) {
                 name = attributes.getQName(i);
             }

@@ -548,12 +548,12 @@
                 CharSequence name = attributes.getLocalName(i);
                 CharSequence value = attributes.getValue(i);

-                    if (UtilValidate.isEmpty(name)) {
+                    if (name == null || name.length() == 0) {
                     name = attributes.getQName(i);
                 }
                 try {
                     // treat empty strings as nulls
-                        if (UtilValidate.isNotEmpty(value)) {

                         } else {

Revert Jacques changes to EntitySaxReader.java from r883549 and 883507, the UtilValidate.is(Not)Empty methods are not setup to test CharSequence instances. Hopefully there aren't too many more out there. ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/ EntitySaxReader.java Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/ util/ EntitySaxReader.java = = = = = = = = = = = = = ================================================================= --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/ EntitySaxReader.java (original) +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/ EntitySaxReader.java Tue Nov 24 07:43:18 2009 - if (UtilValidate.isNotEmpty(currentFieldValue)) { + if (currentFieldValue != null && currentFieldValue.length() > 0) { if (currentValue .getModelEntity().isField(currentFieldName.toString())) { ModelEntity modelEntity = currentValue.getModelEntity(); ModelField modelField = modelEntity.getField(currentFieldName.toString()); newElement.setAttribute(name.toString(), value.toString()); + if (value != null && value.length() > 0) { if (currentValue.getModelEntity().isField(name.toString())) { currentValue.setString(name.toString(), value.toString());

smime.p7s
Description: S/MIME cryptographic signature