Lots of talk about different types of contributors etc and it should be
noted there are also lots of types of bugs.
Your other posts highlight specific: do this, do that, it
crashes/doesn't provide the expected result. That's helpful and will
tend to get a response fairly quickly as they may not require as much
time to verify and fix.
This post is very different and could be titled "Potential bugs in
OrderView.groovy". I don't think anybody involved in OFBiz can answer
your post with out doing a full code review of the file.
There are numerous possible reasons for including the first check and
not the second, it depends on lots of things and the file is split in to
several 'if' sections that may have a lot of impact on whether a
productStore is expected to be found. And in a file that is over 400
lines long it could take some effort to assess and justify the one thing
you've highlighted before dealing with the "I didn't note all of them"
others.
I think with the:
> If yes, why the first test ?
> If no, there is missing a test in the second case.
you might be over simplifying the problem as there are always other
dependencies.
So although it's a reasonable post to suggest there are problems in the
file the difficulty is you need someone else to volunteer a reasonable
amount of their own time to investigate, justify and fix a potential
issue. It's basically a retrospective code review that will take a lot
of effort, and carries it's own risks of introducing new problems.
Generally code quality gets looked at when someone is working on a file.
Don't take it personally on this post but I suspect you won't get
someone jumping in and adding/removing a speculative 'if' wrapper as you
are indirectly asking for quite a lot.
On the other hand if you read the code and can produce a test case that
triggers a null pointer exception on line 380....
Ray
Cimballi wrote:
Well, I don't know how to explain you, it seems evident to me...
In the first case you check if "productStore" is null or not. In the
second case, you don't check.
So, what is the correct behaviour ? Should an order be linked to a
productStore or not ?
If yes, why the first test ?
If no, there is missing a test in the second case.
From my point of view I would say no because an order with products of
type "service" don't need productStore.
To Scott : you should consider there are different kind of
contributors on open source projects, I'm the kind of contributor who
send emails when I find something I think is a bug, I'm still not in
the category of "patch providers" ! :-)
Cimballi
On Fri, May 29, 2009 at 7:16 PM, Scott Gray <[email protected]> wrote:
Hi Cimballi
Inconsistencies aren't necessarily bugs, but you are most welcome to create
a patch and jira issue for the corrections you think should be made.
Thanks
Scott
On 30/05/2009, at 10:39 AM, Cimballi wrote:
Hi,
There are several inconsistancies in the file
"applications/order/webapp/ordermgr/WEB-INF/actions/order/OrderView.groovy".
I didn't note all of them, but here is an example :
Line 260, productStore can be null :
productStore = orderHeader.getRelatedOne("ProductStore");
if (productStore) {
facility = productStore.getRelatedOne("Facility");
Line 380, here productStore cannot be null :
productStoreId =
orderHeader.getRelatedOne("ProductStore").productStoreId;
Cimballi
