Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
Locked
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
 
Administrator
|
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
|
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. 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. Regards Scott On 24/11/2009, at 9:49 PM, Jacques Le Roux wrote: > Thanks Scott, > > 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) > > Jacques > > From: <[hidden email]> >> Author: lektran >> Date: Tue Nov 24 07:43:18 2009 >> New Revision: 883613 >> >> URL: http://svn.apache.org/viewvc?rev=883613&view=rev >> Log: >> 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. >> >> Modified: >> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/ >> EntitySaxReader.java >> >> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/ >> EntitySaxReader.java >> 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 >> = >> = >> = >> = >> = >> = >> = >> = >> = >> ===================================================================== >> --- 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 >> @@ -375,7 +375,7 @@ >> >> if (currentValue != null) { >> if (currentFieldName != null) { >> - 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()); >> @@ -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); >> } >> newElement.setAttribute(name.toString(), >> value.toString()); >> @@ -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)) { >> + if (value != null && value.length() > 0) { >> if >> (currentValue.getModelEntity().isField(name.toString())) { >> >> currentValue.setString(name.toString(), value.toString()); >> } else { >> >> > > |
smime.p7s (4K)
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
|
Administrator
|
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
|
|
Wrong spot, it needs to go just before the end of the method:
Debug.logWarning("In ObjectType.isEmpty(Object) returning false for unknown not-null Object."); return false; Regards Scott On 24/11/2009, at 11:33 PM, Jacques Le Roux wrote: > Hi Scott, > > What do you think about adding this check at the beginning of > ObjectType.isEmpty() something like > > Index: framework/base/src/org/ofbiz/base/util/ObjectType.java > =================================================================== > --- framework/base/src/org/ofbiz/base/util/ObjectType.java (revision > 883647) > +++ framework/base/src/org/ofbiz/base/util/ObjectType.java (working > copy) > @@ -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" <[hidden email]> >> 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. >> >> 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. >> >> Regards >> Scott >> >> On 24/11/2009, at 9:49 PM, Jacques Le Roux wrote: >> >>> Thanks Scott, >>> >>> 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) >>> >>> Jacques >>> >>> From: <[hidden email]> >>>> Author: lektran >>>> Date: Tue Nov 24 07:43:18 2009 >>>> New Revision: 883613 >>>> >>>> URL: http://svn.apache.org/viewvc?rev=883613&view=rev >>>> Log: >>>> 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. >>>> >>>> Modified: >>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/ >>>> EntitySaxReader.java >>>> >>>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/ >>>> EntitySaxReader.java >>>> 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 >>>> = = = = = = = = = >>>> = >>>> = >>>> =================================================================== >>>> --- 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 >>>> @@ -375,7 +375,7 @@ >>>> >>>> if (currentValue != null) { >>>> if (currentFieldName != null) { >>>> - 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()); >>>> @@ -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); >>>> } >>>> newElement.setAttribute(name.toString(), >>>> value.toString()); >>>> @@ -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)) { >>>> + if (value != null && value.length() > 0) { >>>> if >>>> (currentValue.getModelEntity().isField(name.toString())) { >>>> >>>> currentValue.setString(name.toString(), value.toString()); >>>> } else { >>>> >>>> >>> >>> >> > > |
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
|
Administrator
|
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
|
|
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; personally, I don't have a problem with (and actually prefer) using a single line for simple statements. Regards Scott On 25/11/2009, at 12:06 AM, Jacques Le Roux wrote: > 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 > =================================================================== > --- framework/base/src/org/ofbiz/base/util/ObjectType.java (revision > 883647) > +++ framework/base/src/org/ofbiz/base/util/ObjectType.java (working > copy) > @@ -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; > } > } > + Debug.logWarning("In ObjectType.isEmpty(Object) returning > false for unknown not-null Object.", module); > return false; > } > > Jacques > > ----- Original Message ----- From: "Scott Gray" <[hidden email] > > > To: <[hidden email]> > Sent: Tuesday, November 24, 2009 11:44 AM > 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: >> Debug.logWarning("In ObjectType.isEmpty(Object) returning false >> for unknown not-null Object."); >> return false; >> >> Regards >> Scott >> >> On 24/11/2009, at 11:33 PM, Jacques Le Roux wrote: >> >>> Hi Scott, >>> >>> What do you think about adding this check at the beginning of >>> ObjectType.isEmpty() something like >>> >>> Index: framework/base/src/org/ofbiz/base/util/ObjectType.java >>> =================================================================== >>> --- framework/base/src/org/ofbiz/base/util/ObjectType.java >>> (revision 883647) >>> +++ framework/base/src/org/ofbiz/base/util/ObjectType.java >>> (working copy) >>> @@ -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" <[hidden email]> >>>> 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. >>>> >>>> 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. >>>> >>>> Regards >>>> Scott >>>> >>>> On 24/11/2009, at 9:49 PM, Jacques Le Roux wrote: >>>> >>>>> Thanks Scott, >>>>> >>>>> 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) >>>>> >>>>> Jacques >>>>> >>>>> From: <[hidden email]> >>>>>> Author: lektran >>>>>> Date: Tue Nov 24 07:43:18 2009 >>>>>> New Revision: 883613 >>>>>> >>>>>> URL: http://svn.apache.org/viewvc?rev=883613&view=rev >>>>>> Log: >>>>>> 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. >>>>>> >>>>>> Modified: >>>>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/ >>>>>> EntitySaxReader.java >>>>>> >>>>>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/ >>>>>> util/ EntitySaxReader.java >>>>>> 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 >>>>>> = = = = = = = = = = = >>>>>> = >>>>>> = >>>>>> ================================================================= >>>>>> --- 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 >>>>>> @@ -375,7 +375,7 @@ >>>>>> >>>>>> if (currentValue != null) { >>>>>> if (currentFieldName != null) { >>>>>> - 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()); >>>>>> @@ -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); >>>>>> } >>>>>> newElement.setAttribute(name.toString(), >>>>>> value.toString()); >>>>>> @@ -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)) { >>>>>> + if (value != null && value.length() > >>>>>> 0) { >>>>>> if >>>>>> (currentValue.getModelEntity().isField(name.toString())) { >>>>>> >>>>>> currentValue.setString(name.toString(), value.toString()); >>>>>> } else { >>>>>> >>>>>> >>>>> >>>>> >>>> >>> >>> >> > > |
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
|
Administrator
|
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
|
|
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
|
|
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
|
|
Instance of is a bad practice to get into in object oriented code -
might as well just go back to C and switch statements. Cheers, Ruppert On Nov 24, 2009, at 8:45 AM, Adam Heath wrote: > Adrian Crum wrote: >> Or better yet, overload the method so you don't need all those >> conditionals. > > If the type of the object in the calling method is Object, then it > won't call the overloaded method. > > However, this is better: > > if (object instanceof String) { > return UtilValidate.isEmpty((String) object); > } |
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
|
|
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
|
|
Everybody's got their own style dude - I could find you 10 articles
that will flip it. Besides - there are no absolutes in this world, so I'll refrain from spending my day trying to prove that the instance that you were referring to IS a bad practice. When writing object oriented code I also think it's generally a "bad" practice to just use Object, but instead tend to not use that catch all as the way I write my code - but hey that's just me. I think better thought thru designs yield cleaner interfaces that doing things like "instanceof" and setting everything to object. Just my two cents - feel free to drop them on the floor (face up) for the next lucky person walking by :) Cheers, Ruppert -- Tim Ruppert HotWax Media http://www.hotwaxmedia.com o:801.649.6594 f:801.649.6595 On Nov 24, 2009, at 11:09 AM, Adam Heath wrote: > Tim Ruppert wrote: >> Instance of is a bad practice to get into in object oriented code - >> might as well just go back to C and switch statements. > > == > String foo = (String) context.get("foo"); > > if (UtilValidate.isEmpty(foo)) { > } > > Object bar = context.get("bar"); > Map barMap; > if (UtilValidate.isEmpty(bar)) { > return; > } else if (bar instanceof String) { > barMap = parseStringAsJSONMap((String) bar); > } else if (bar instanceof Collection) { > barMap = convertCollectionToMap((Collection) bar); > } else if (bar instanceof Map) { > barMap = (Map) bar; > } else { > throw new IllegalArgumentException("Can't handle bar"); > } > == > > The first isEmpty call calls isEmpty(String). The latter calls > isEmpty(Object). The latter method then needs to do the instanceof > itself. But I assume you understand that. > > Besides, you are deep into the 'instanceof always considered bad > camp'. This is wrong. See > > http://www.velocityreviews.com/forums/t302491-instanceof-not-always-bad-the-instanceof-myth.html > > > > > > >> >> Cheers, >> Ruppert >> >> On Nov 24, 2009, at 8:45 AM, Adam Heath wrote: >> >>> Adrian Crum wrote: >>>> Or better yet, overload the method so you don't need all those >>>> conditionals. >>> >>> If the type of the object in the calling method is Object, then it >>> won't call the overloaded method. >>> >>> However, this is better: >>> >>> if (object instanceof String) { >>> return UtilValidate.isEmpty((String) object); >>> } >> > |
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
|
|
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
|
Administrator
|
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
|
|
Re: svn commit: r883613 - /ofbiz/trunk/framework/entity/
|
|
Thanks for the clear explanation Adrian. Instanceof was put into the
language because there are times when it needs to be used - and it sounds like we're using it. I'd have to look at the specific lines to know if I could clean it up more to have methods and interface implementations that know better how to handle the appropriate type of objects - but in this case it seems like the simplest thing that will work. Again, thanks for the heads up - much appreciated. Cheers, Ruppert -- Tim Ruppert HotWax Media http://www.hotwaxmedia.com o:801.649.6594 f:801.649.6595 On Nov 24, 2009, at 12:01 PM, Adrian Crum wrote: > I think for the most part we're following good design principals. > The use of instanceof in this case is because we don't know the > object's type at compile time, and that happens often in the script > interpreter code. > > Take the new converter code as an example. It uses Gang of Four > design patterns and one of Martin Fowler's refactoring methods, yet > it still needs to use instanceof. If you have Object A that is an > unknown type, and you need to convert it to Object type B, you have > to use instanceof to determine the type of Object A in order to > lookup the correct converter. > > -Adrian > > > Tim Ruppert wrote: >> Everybody's got their own style dude - I could find you 10 articles >> that will flip it. Besides - there are no absolutes in this world, >> so I'll refrain from spending my day trying to prove that the >> instance that you were referring to IS a bad practice. >> When writing object oriented code I also think it's generally a >> "bad" practice to just use Object, but instead tend to not use that >> catch all as the way I write my code - but hey that's just me. I >> think better thought thru designs yield cleaner interfaces that >> doing things like "instanceof" and setting everything to object. >> Just my two cents - feel free to drop them on the floor (face up) >> for the next lucky person walking by :) >> Cheers, >> Ruppert >> -- >> Tim Ruppert >> HotWax Media >> http://www.hotwaxmedia.com >> o:801.649.6594 >> f:801.649.6595 >> On Nov 24, 2009, at 11:09 AM, Adam Heath wrote: >>> Tim Ruppert wrote: >>>> Instance of is a bad practice to get into in object oriented code - >>>> might as well just go back to C and switch statements. >>> >>> == >>> String foo = (String) context.get("foo"); >>> >>> if (UtilValidate.isEmpty(foo)) { >>> } >>> >>> Object bar = context.get("bar"); >>> Map barMap; >>> if (UtilValidate.isEmpty(bar)) { >>> return; >>> } else if (bar instanceof String) { >>> barMap = parseStringAsJSONMap((String) bar); >>> } else if (bar instanceof Collection) { >>> barMap = convertCollectionToMap((Collection) bar); >>> } else if (bar instanceof Map) { >>> barMap = (Map) bar; >>> } else { >>> throw new IllegalArgumentException("Can't handle bar"); >>> } >>> == >>> >>> The first isEmpty call calls isEmpty(String). The latter calls >>> isEmpty(Object). The latter method then needs to do the instanceof >>> itself. But I assume you understand that. >>> >>> Besides, you are deep into the 'instanceof always considered bad >>> camp'. This is wrong. See >>> >>> http://www.velocityreviews.com/forums/t302491-instanceof-not-always-bad-the-instanceof-myth.html >>> >>> >>> >>> >>> >>> >>>> >>>> Cheers, >>>> Ruppert >>>> >>>> On Nov 24, 2009, at 8:45 AM, Adam Heath wrote: >>>> >>>>> Adrian Crum wrote: >>>>>> Or better yet, overload the method so you don't need all those >>>>>> conditionals. >>>>> >>>>> If the type of the object in the calling method is Object, then it >>>>> won't call the overloaded method. >>>>> >>>>> However, this is better: >>>>> >>>>> if (object instanceof String) { >>>>> return UtilValidate.isEmpty((String) object); >>>>> } >>>> >>> |
| Free forum by Nabble | Edit this page |