enums, generics and other animals

[adrian.brock]

I don't understand why there is an InjectOption and an InjectionOption?

The reason the ControllerState and ControllerMode are the way they are
is because the MC was originally written on JDK1.4
so they couldn't be written as enums.

Actually, since ControllerState is extensible (you can create your own states)
it can't be an Enum.

All of these should just be enums throughout

Cardinality
FromContext
InjectionOption (already is InjectOption???)
AutowireType

Since we are doing 2.0.0 we could also decde to change ControllerMode to an Enum
as well?

Also the FromContext is a misuse of generics.
The purpose of generics is to produce compile time errors, the way this class
is used, it is simply not possible for the compiler to do so.

It looks to me like the generic is only here to avoid having to manually code
the check/cast of ControllerContext -> KernelControllerContext
but you should do that by hand anyway to give a better error message than CCE

e.g. MetaDataFromContext

 public MetaData internalExecute(ControllerContext context)
 {
 if (context instanceof KernelControllerContext == false)
 throw new IllegalArgumentException("Cannot get metadata from context " + context);
 KernelControllerContext kernelContext = (KernelControllerContext) context;
 return kernelContext.getScopeInfo().getMetaData();
 }

In fact, this is wrong anyway, since you can get a ScopeInfo/Metadata
from any context. ;-)

[alesj]

I don't understand why there is an InjectOption and an InjectionOption?

The non enum version was first added.
But when the annotation support kicked in, I didn't want to rely on the String, so the enum was added, but the old one was never removed. :-(

You can also see that the FromContext suffers from the same syndrome.
Where Inject(ion)Option should really be enum, since it only has finite number of usages, the FromContext due to its Dynamic case cannot be.

So, it the case you use annotations, you cannot get the DynamicFromContext.

The non-enum FromContext is really an impl detail.
And perhaps to avoid confusion, it can be renamed.

All of these should just be enums throughout

Cardinality
FromContext
InjectionOption (already is InjectOption???)
AutowireType

The Cardinality and FromContext cannot be enums.

The Cardinality can be whatever, not just those 4 usual usages.
e.g. 2..7, 5..n, etc

For FromContext see DynamicFromContext.

Since we are doing 2.0.0 we could also decde to change ControllerMode to an Enum as well?

Since I'll change a few of them anyway, I don't see why not this one as well.

Also the FromContext is a misuse of generics.
The purpose of generics is to produce compile time errors, the way this class is used, it is simply not possible for the compiler to do so.

It looks to me like the generic is only here to avoid having to manually code the check/cast of ControllerContext -> KernelControllerContext
but you should do that by hand anyway to give a better error message than CCE.

I'm already doing validation, so the error msg is useful.
But you're right, this is a misuse. A handy one, but a misuse. :-)
I'll remove the generics.

[adrian.brock]

"alesj" wrote:

I don't understand why there is an InjectOption and an InjectionOption?

So, it the case you use annotations, you cannot get the DynamicFromContext.
...
For FromContext see DynamicFromContext.

What is that rubbish? :-)

Why are you exposing "random" things?

How do we know what people have used in the api if they can just
select parts of it using reflection?
You might as well let people write

<property><script type="javascript">do what you like here but its not very declarative
or guaranteed to work in future versions</script></property>

It also exposes things that are "private interfaces"

// Intended for the user
public interface ControllerContext {}

public class MyControllerContext implements ControllerContext,
 ImplementDetails // Not intended for the user and subject to change without notice
{
}

[adrian.brock]

"alesj" wrote:

The Cardinality can be whatever, not just those 4 usual usages.
e.g. 2..7, 5..n, etc

ok.

[alesj]

"alesj" wrote:

I don't understand why there is an InjectOption and an InjectionOption?

So, it the case you use annotations, you cannot get the DynamicFromContext.
...
For FromContext see DynamicFromContext.

What is that rubbish? :-)

Deleted. :-(

It looked cool at that time.
So that I wouldn't have to write new impl every time Diesler wanted to pull some detail from the underlying component. :-)

So, another enum coming up.

[adrian.brock]

"alesj" wrote:

It looked cool at that time.

Beware of "cool features". :-)

They'll come back to haunt you when you need to make changes
and realise it is no longer supportable in its current form because you did a "quick and dirty".

Or in this case, DONT realise you are removing a feature because
there's no contract or test for it. :-)

Don't do a feature unless it is obviously correct, but even then
only when somebody asks for (needs) it.

It's less to support, maintain, fix and answer annoying questions
(from users that can't be bothered to read the docs) about in future. :-)

[alesj]

Where to keep all this enums?
Since they are now scattered all over the place:

ControllerMode --> org.jboss.dependency.spi // OK, this one is here to stay
FromContext --> org.jboss.beans.metadata.plugins
AutowireType --> org.jboss.beans.metadata.spi
InjectOption --> org.jboss.beans.metadata.api.annotations

The idea with annotations was to keep them in an 'api' package + have separate artifact for them, so user can use just that for his beans.

And since now both, beans metadata and annotations, use the same enums, we should find some appropriate place. :-)

Perhaps org.jboss.beans.metadata.api.enums?
But there is the AutowireType which is part of spi? :-(
Moving all but that one there?

[adrian.brock]

If they are used by the annotations then they should be in the api.

We should really look to move all the annotations (MC, Managed, etc.)
to the jboss-integration project as seperate subprojects
so people can consume just them for compiles as they need them

No need to do this straightaway though. You'll conflict with what I'm doing
if you do that now. ;-)

[alesj]

If they are used by the annotations then they should be in the api.

Even the AutowireType?
Moving it out of spi?

[adrian.brock]

"alesj" wrote:

If they are used by the annotations then they should be in the api.

Even the AutowireType?
Moving it out of spi?

Yes. If it is used by the annotations then people need it to compile.

Also, even for things like ControllerState you can create a helper class.
e.g.

public interface MicrocontainerConstants
{
 String CREATE = "Create";
 String INSTANTIATED = "Instantiated";
}

That way people can reference the constants in their annotations
instead of suffering from typos for the common values. ;-)

[alesj]

Also, even for things like ControllerState you can create a helper class.
e.g.

public interface MicrocontainerConstants
{
 String CREATE = "Create";
 String INSTANTIATED = "Instantiated";
}

That way people can reference the constants in their annotations
instead of suffering from typos for the common values. ;-)

I though using interfaces to get access to constants was anti-pattern. ;-)

[adrian.brock]

"alesj" wrote:

I though(t) using interfaces to get access to constants was anti-pattern. ;-)

It's only an anti-pattern if you are lazy/stupid and "implement" the interface ;-)

The import static and enums have replaced this usage but only if your
constants are actually enums, i.e. a fixed set of values.

[alesj]

Another refactoring question. :-)
Since for xml handling I still need to get the right enum by matching the underlying strings.
The thing is that on enums there is already a valueOf method which does similar thing, matching enum's name with input string parameter.
But in our case the names do not match exactly the underlying strings, resulting in needing something like getInstance method which iterates over the enums and does a string match.
Should I just use the valueOf and fix the strings in our tests?
Probably not, since it will break quite some things. :-)

[starksm64]

You can write an XmlAdapter to handle this:
https://svn.jboss.org/repos/jbossas/projects/metadata/trunk/src/main/java/org/jboss/metadata/javaee/spec/TransactionAttributeTypeAdapter.java

[adrian.brock]

"alesj" wrote:
Another refactoring question. :-)
Since for xml handling I still need to get the right enum by matching the underlying strings.
The thing is that on enums there is already a valueOf method which does similar thing, matching enum's name with input string parameter.
But in our case the names do not match exactly the underlying strings, resulting in needing something like getInstance method which iterates over the enums and does a string match.
Should I just use the valueOf and fix the strings in our tests?
Probably not, since it will break quite some things. :-)

RTFM :-)

You can actually do some pretty weird stuff, so don't overuse it ;-)

@XmlEnum(Integer.class)
public enum OldEnglishMoney
{
 @XmlEnumValue("D")
 PENNY(1),
 @XmlEnumValue("S")
 SHILLING(12)
 @XmlEnumValue("L")
 POUND(240)
}