OFBiz › OFBiz - Dev

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

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

4 messages Options

Adrian Crum

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

[hidden email] 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"/>
> + 
> </filelist>
>
> <target name="init">
>
>
>

Adrian Crum

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

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
>
> [hidden email] 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"/>
>> + 
>> </filelist>
>>
>> <target name="init">
>>
>>
>>
>

Bob Morley

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

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 ...

Adrian Crum-2

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

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 <[hidden email]> wrote:

> From: Bob Morley <[hidden email]>
> 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: [hidden email]
> 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.
>