You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Adrian Crum <ad...@hlmksw.com> on 2010/04/15 00:03:03 UTC
Re: svn commit: r934179 - in /ofbiz/trunk/framework/base: build.xml
src/org/ofbiz/base/util/Assert.java src/org/ofbiz/base/util/test/AssertTests.java
I'm thinking about putting this back in. The Validate syntax seems kind
of clunky to me. I despise re-inventing the wheel, but at the same time
I liked my syntax better.
Using Validate:
public void myMethod(Object foo, Object bar) {
Validate.notNull(foo);
Validate.notNull(bar);
// Throws NullPointerException "The validated object is null"
}
Using Assert:
public void myMethod(Object foo, Object bar) {
Assert.argumentsNotNull("foo", foo, "bar", bar);
// Throws IllegalArgumentException "foo (or bar) cannot be null"
}
Comments are welcome.
-Adrian
adrianc@apache.org wrote:
> Author: adrianc
> Date: Wed Apr 14 20:22:26 2010
> New Revision: 934179
>
> URL: http://svn.apache.org/viewvc?rev=934179&view=rev
> Log:
> Reverting my last commits. I just found out Apache Commons Validate does the same thing.
>
> Removed:
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/Assert.java
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/test/AssertTests.java
> Modified:
> ofbiz/trunk/framework/base/build.xml
>
> Modified: ofbiz/trunk/framework/base/build.xml
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/build.xml?rev=934179&r1=934178&r2=934179&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/base/build.xml (original)
> +++ ofbiz/trunk/framework/base/build.xml Wed Apr 14 20:22:26 2010
> @@ -41,7 +41,7 @@ under the License.
> </patternset>
>
> <filelist id="test.classes" dir="${src.dir}">
> - <file name="org/ofbiz/base/util/test/AssertTests.java"/>
> + <!--
> <file name="org/ofbiz/base/lang/test/ComparableRangeTests.java"/>
> <file name="org/ofbiz/base/util/test/IndentingWriterTests.java"/>
> <file name="org/ofbiz/base/util/test/ObjectTypeTests.java"/>
> @@ -60,6 +60,7 @@ under the License.
> <file name="org/ofbiz/base/concurrent/test/SyncTTLObjectTest.java"/>
> <file name="org/ofbiz/base/concurrent/test/AsyncTTLObjectTest.java"/>
> <file name="org/ofbiz/base/concurrent/test/TTLCachedObjectTest.java"/>
> + -->
> </filelist>
>
> <target name="init">
>
>
>
Re: svn commit: r934179 - in /ofbiz/trunk/framework/base: build.xml src/org/ofbiz/base/util/Assert.java src/org/ofbiz/base/util/test/AssertTests.java
Posted by Adrian Crum <ad...@yahoo.com>.
Bob,
Thanks for the reply! Sorry for the confusion - my local copy got mangled and told me the files were still there when in fact they had been reverted. Deleting my local copy and doing a fresh checkout restored sanity. The bottom line is, I took the files back out - so that's why you can't see them. Check out the previous revision to see them.
My motivation for committing it is code quality. I like fail-fast designs. If you give me a bad argument, then I will give you an exception right back. The Assert class has methods that allow you to check arguments in a single line - making argument checking very convenient. If the arguments are valid, nothing happens. If the arguments are invalid, then an exception is thrown - with a message that tells the programmer what's wrong.
If I commit the files, there will be no need to extend Validate - the code is simple enough that extending Validate will just make things more complicated. I will include one nice feature Validate has that I didn't think of - a list of valid choices in the exception message:
"foo is incorrect type. Must be one of MyClass, YourClass, SomeClass."
-Adrian
--- On Wed, 4/14/10, Bob Morley <rm...@emforium.com> wrote:
> From: Bob Morley <rm...@emforium.com>
> Subject: Re: svn commit: r934179 - in /ofbiz/trunk/framework/base: build.xml src/org/ofbiz/base/util/Assert.java src/org/ofbiz/base/util/test/AssertTests.java
> To: dev@ofbiz.apache.org
> Date: Wednesday, April 14, 2010, 9:02 PM
>
>
> Adrian Crum wrote:
> >
> >> Comments are welcome.
> >
> Your first mistake! heh
>
> It does not appear my trunk checkout has these classes in
> them ... I am
> guessing that the use here is in standard classes (not in
> junit testers).
> If you have a second to look at my patch associated with
> OFBIZ-3670; I
> provided a BaseTestCase that provides similar assertions
> that add-on to the
> set provided by junit (I am taking assertEmpty type guys
> that use the
> UtilValidate.isEmpty) ... If the intent was to do this from
> unit testers I
> would consider putting them in this base class.
>
> As for comments, I would _consider_ the following minor
> tweaks ...
>
> a) (if possible) create an org.ofbiz.base.util.Validate
> that extends the one
> from Apache commons with your enhancement
> b) would consider overloading "notNull" rather than
> introducing a new method
> c) without seeing the class, my guess is that you have it
> overloaded to
> support n name/value pairs (where n is arbitrarily largish
> ... like 6) :)
> d) may consider creating a Pair object so we could have a
> signature
> something like ..
>
> Validate.notNull(Pair.create("foo", foo),
> Pair.create("bar", bar));
>
> public static void notNull(Pair... objects) {
> ...
> }
>
> You can also provide a notNull that just takes an
> open-ended set of objects
> and does what the apache commons implementation does (just
> on each one). Ok
> those are my ramblings ...
> --
> View this message in context: http://n4.nabble.com/Re-svn-commit-r934179-in-ofbiz-trunk-framework-base-build-xml-src-org-ofbiz-base-util-Assert-java-sra-tp1840515p1840770.html
> Sent from the OFBiz - Dev mailing list archive at
> Nabble.com.
>
Re: svn commit: r934179 - in /ofbiz/trunk/framework/base: build.xml
src/org/ofbiz/base/util/Assert.java
src/org/ofbiz/base/util/test/AssertTests.java
Posted by Bob Morley <rm...@emforium.com>.
Adrian Crum wrote:
>
>> Comments are welcome.
>
Your first mistake! heh
It does not appear my trunk checkout has these classes in them ... I am
guessing that the use here is in standard classes (not in junit testers).
If you have a second to look at my patch associated with OFBIZ-3670; I
provided a BaseTestCase that provides similar assertions that add-on to the
set provided by junit (I am taking assertEmpty type guys that use the
UtilValidate.isEmpty) ... If the intent was to do this from unit testers I
would consider putting them in this base class.
As for comments, I would _consider_ the following minor tweaks ...
a) (if possible) create an org.ofbiz.base.util.Validate that extends the one
from Apache commons with your enhancement
b) would consider overloading "notNull" rather than introducing a new method
c) without seeing the class, my guess is that you have it overloaded to
support n name/value pairs (where n is arbitrarily largish ... like 6) :)
d) may consider creating a Pair object so we could have a signature
something like ..
Validate.notNull(Pair.create("foo", foo), Pair.create("bar", bar));
public static void notNull(Pair... objects) {
...
}
You can also provide a notNull that just takes an open-ended set of objects
and does what the apache commons implementation does (just on each one). Ok
those are my ramblings ...
--
View this message in context: http://n4.nabble.com/Re-svn-commit-r934179-in-ofbiz-trunk-framework-base-build-xml-src-org-ofbiz-base-util-Assert-java-sra-tp1840515p1840770.html
Sent from the OFBiz - Dev mailing list archive at Nabble.com.
Re: svn commit: r934179 - in /ofbiz/trunk/framework/base: build.xml
src/org/ofbiz/base/util/Assert.java src/org/ofbiz/base/util/test/AssertTests.java
Posted by Adrian Crum <ad...@hlmksw.com>.
Okay, that's weird. I just realized my revert didn't revert anything.
Adrian Crum wrote:
> I'm thinking about putting this back in. The Validate syntax seems kind
> of clunky to me. I despise re-inventing the wheel, but at the same time
> I liked my syntax better.
>
> Using Validate:
>
> public void myMethod(Object foo, Object bar) {
> Validate.notNull(foo);
> Validate.notNull(bar);
> // Throws NullPointerException "The validated object is null"
> }
>
> Using Assert:
>
> public void myMethod(Object foo, Object bar) {
> Assert.argumentsNotNull("foo", foo, "bar", bar);
> // Throws IllegalArgumentException "foo (or bar) cannot be null"
> }
>
> Comments are welcome.
>
> -Adrian
>
> adrianc@apache.org wrote:
>> Author: adrianc
>> Date: Wed Apr 14 20:22:26 2010
>> New Revision: 934179
>>
>> URL: http://svn.apache.org/viewvc?rev=934179&view=rev
>> Log:
>> Reverting my last commits. I just found out Apache Commons Validate
>> does the same thing.
>>
>> Removed:
>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/Assert.java
>>
>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/test/AssertTests.java
>> Modified:
>> ofbiz/trunk/framework/base/build.xml
>>
>> Modified: ofbiz/trunk/framework/base/build.xml
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/build.xml?rev=934179&r1=934178&r2=934179&view=diff
>>
>> ==============================================================================
>>
>> --- ofbiz/trunk/framework/base/build.xml (original)
>> +++ ofbiz/trunk/framework/base/build.xml Wed Apr 14 20:22:26 2010
>> @@ -41,7 +41,7 @@ under the License.
>> </patternset>
>>
>> <filelist id="test.classes" dir="${src.dir}">
>> - <file name="org/ofbiz/base/util/test/AssertTests.java"/>
>> + <!--
>> <file
>> name="org/ofbiz/base/lang/test/ComparableRangeTests.java"/>
>> <file
>> name="org/ofbiz/base/util/test/IndentingWriterTests.java"/>
>> <file name="org/ofbiz/base/util/test/ObjectTypeTests.java"/>
>> @@ -60,6 +60,7 @@ under the License.
>> <file
>> name="org/ofbiz/base/concurrent/test/SyncTTLObjectTest.java"/>
>> <file
>> name="org/ofbiz/base/concurrent/test/AsyncTTLObjectTest.java"/>
>> <file
>> name="org/ofbiz/base/concurrent/test/TTLCachedObjectTest.java"/>
>> + -->
>> </filelist>
>>
>> <target name="init">
>>
>>
>>
>