Thanks Frank and John,
I've been strugling with the enums in Networks.java. A discussion on those
is needed I think. BroadcastDomainType seems to be a hybrid of a stable and
a plugin extendible enum. It breaks my mind and my every change.
regards,
Daan
On Tue, Jul 23, 2013 at 8:37 PM, Frank Zhang <frank.zh...@citrix.com> wrote:
> Speaking of method on enum, I used to think it's very handy feature but
> finally I failed on some scenarios and switched to own defined class.
> The problem is enum is singleton that you can't save stateful data in it.
> For example:
>
> public enum Type {
> private Object userData;
>
> private Object getUserData() {...}
> private void setUserData(Object data) {...}
> ....
>
> Type SOME_TYPE;
> }
>
> This kind of thing is somewhat useless, userData here is actually global,
> every call to SOME_TYPE.setUserData() would change its value.
> So for usage perspective the methods you define on enmu are like static
> method on class which has its limitations.
>
> As for methods in enmu body in your example, the only usage I can recall
> now is like what TimeUnit does. For CloudStack, we can do the similar
> thing: SizeUnit that translates storage/memory size in different
> quantities.
>
> public enum SizeUnit {
> BYTE {
> public long toKiloByte(long s) {
> return (s / (k / b));
> }
>
> public long toMegaByte(long s) {
> return (s / (m / b));
> }
>
> public long toGigaByte(long s) {
> return (s / (g / b));
> }
>
> public long toTeraByte(long s) {
> return (s / (t / b));
> }
> },
> KILOBYTE {
> public long toByte(long s) {
> return (s * (k / b));
> }
>
> public long toMegaByte(long s) {
> return (s / (m / k));
> }
>
> public long toGigaByte(long s) {
> return (s / (g / k));
> }
>
> public long toTeraByte(long s) {
> return (s / (t / k));
> }
> },
> MEGABYTE {
> public long toByte(long s) {
> return (s * (m / b));
> }
>
> public long toKiloByte(long s) {
> return (s * (m / k));
> }
>
> public long toGigaByte(long s) {
> return (s / (g / m));
> }
>
> public long toTeraByte(long s) {
> return (s / (t / m));
> }
> },
> GIGABYTE {
> public long toByte(long s) {
> return (s * (g / b));
> }
>
> public long toKiloByte(long s) {
> return (s * (g / k));
> }
>
> public long toMegaByte(long s) {
> return (s * (g / m));
> }
>
> public long toTeraByte(long s) {
> return (s / (t / g));
> }
> },
> TERABYTE {
> public long toByte(long s) {
> return (s * (t / b));
> }
>
> public long toKiloByte(long s) {
> return (s * (t / k));
> }
>
> public long toMegaByte(long s) {
> return (s * (t / m));
> }
>
> public long toGigaByte(long s) {
> return (s * (t / g));
> }
> };
>
> private static final long b = 1;
> private static final long k = b * 1024;
> private static final long m = k * 1024;
> private static final long g = m * 1024;
> private static final long t = g * 1024;
>
> public long toByte(long s) {
> throw new AbstractMethodError();
> }
> public long toKiloByte(long s) {
> throw new AbstractMethodError();
> }
> public long toMegaByte(long s) {
> throw new AbstractMethodError();
> }
> public long toGigaByte(long s) {
> throw new AbstractMethodError();
> }
> public long toTeraByte(long s) {
> throw new AbstractMethodError();
> }
> }
>
> This may explain some of my views on java enum. It does have lots of
> advantages like compiler check, native == operator. Just like
> Alex mentioned, for constants that unlikely to change in future, we can
> stick to enum. But for types that plugin may extend, we'd
> better to use own defined classes.
>
> From: John Burwell [mailto:jburw...@basho.com]
> Sent: Tuesday, July 23, 2013 6:25 AM
> To: dev@cloudstack.apache.org
> Subject: Re: [DESIGN] Why is enum a class...
>
> All,
>
> +1 to Alex's design suggestion. Another little know feature of
> enumerations is the ability to define abstract methods. Therefore, Alex's
> example can be expanded as follows:
>
> public enum Type {
> User(false)
> {
> @Override
> public void doWork() {
> }
> },
> DomainRouter(true)
> {
> @Override
> public void doWork() {
> }
> },
> ConsoleProxy(true)
> {
> @Override
> public void doWork(){
> {
> },
> SecondaryStorageVm(true)
> {
> @Override
> public void doWork(){
> {
> },
> ElasticIpVm(true)
> {
> @Override
> public void doWork(){
> {
> },
> ElasticLoadBalancerVm(true)
> {
> @Override
> public void doWork(){
> {
> },
> InternalLoadBalancerVm(true)
> {
> @Override
> public void doWork(){
> {
> }
>
> /*
> * UserBareMetal is only used for selecting VirtualMachineGuru,
> there is no
> * VM with this type. UserBareMetal should treat exactly as User.
> */
> UserBareMetal(false);
>
> private boolean _isSystemVm;
>
> private Type(Boolean isSystemVm) {
> _isSystemVm = isSystemVm;
> }
>
> public boolean isSystemVM() {
> return _isSystemVm;
> }
>
> public abstract doWork();
>
> }
>
> Personally, I see using switch statements with Java enumerations as a bad
> code smell. Java enumerations were specifically designed to allow the
> implementation of behavior on values. In addition to simplifying code, it
> greatly increases cohesion by defining all of the uses of the value in one
> place and using the compiler to validate coverage. Finally, it makes unit
> testing of this conditional functionality much more straight forward.
>
> To Frank's point, I don't see the need for another class. The Java
> compiler properly generates the boilerplate methods and ensures that all
> enumeration values are singletons within a class loader. Regardless of the
> state added to an enumeration value, User==User will always be true.
>
> Thanks,
> -John
>
> On Jul 23, 2013, at 3:48 AM, Daan Hoogland <daan.hoogl...@gmail.com>
> wrote:
>
>
> +1 I am having a war on the enums in Networks.java that justifies your
> solution, Frank
>
>
> On Tue, Jul 23, 2013 at 2:57 AM, Alex Huang <alex.hu...@citrix.com> wrote:
>
>
> +1 to do this for any enum that was designed to be added to by the plugins.
>
> I actually wrote a class for this before called Constant that did exactly
> this. The problem I had was that there was no way to guarantee compile
> time enumeration of all of the values like Enum did so it's possible for
> errors to be introduced but certainly we can follow a convention in this
> regard. Another way would be to use Spring to gather them and inject them
> into a holding class.
>
> --Alex
>
>
> -----Original Message-----
> From: Frank Zhang [mailto:frank.zh...@citrix.com]
> Sent: Monday, July 22, 2013 5:45 PM
> To: dev@cloudstack.apache.org
> Subject: RE: [DESIGN] Why is enum a class...
>
> Frankly speaking, if we are going to change enum, I would suggest not
> using
>
> enmu anymore, instead, defining our own class like:
>
> public class VmType {
> private static Map<String, VmType > types =
> Collections.synchronizedMap(new HashMap<String, VmType >());
> private final String typeName;
>
> public VmType (String typeName) {
> this.typeName = typeName;
> types.put(typeName, this);
> }
>
> public static VmType valueOf(String typeName) {
> VmType type = types.get(typeName);
> if (type == null) {
> throw new IllegalArgumentException("VmType type:
> " + typeName + " was not registered by any VmType");
> }
> return type;
> }
>
> @Override
> public String toString() {
> return typeName;
> }
>
> @Override
> public boolean equals(Object t) {
> if (t == null || !(t instanceof VmType)) {
> return false;
> }
>
> VmType type = (VmType)t;
> return type.toString().equals(typeName);
> }
>
> @Override
> public int hashCode() {
> return typeName.hashCode();
> }
>
> public static Set<String> getAllTypeNames() {
> return types.keySet();
> }
> }
>
> The only benefit of enum I see is you can use "==" instead of "equals()"
> when comparing variables. However, it makes your code tight, every time
> adding a new type you have to modify the enum.
>
> By above method, when a new plugin wants to extend a new VmType, it
> simply does:
>
> class MagicVmManagerImpl {
> public static final VmType type = new VmType("MagicVm"); }
>
>
>
> -----Original Message-----
> From: Alex Huang [mailto:alex.hu...@citrix.com]
> Sent: Monday, July 22, 2013 5:23 PM
> To: dev@cloudstack.apache.org
> Subject: RE: [DESIGN] Why is enum a class...
>
> BTW, this code already shows a bug that stems from the static method
> usage.
>
> It says ElasticIpVm is not a system vm (which I don't believe is
> true). Probably because whoever added elastic ip vm didn't see the
> static method and so didn't add the vm into the method.
>
> --Alex
>
>
> -----Original Message-----
> From: Alex Huang [mailto:alex.hu...@citrix.com]
> Sent: Monday, July 22, 2013 5:14 PM
> To: dev@cloudstack.apache.org
> Subject: [DESIGN] Why is enum a class...
>
> I just went over this code and thought it was related and might be
> interested to other developers.
>
> What's the difference between declaring a enum like this
>
> public enum Type {
> User,
> DomainRouter,
> ConsoleProxy,
> SecondaryStorageVm,
> ElasticIpVm,
> ElasticLoadBalancerVm,
> InternalLoadBalancerVm,
>
> /*
> * UserBareMetal is only used for selecting
> VirtualMachineGuru, there is no
> * VM with this type. UserBareMetal should treat exactly as
> User.
>
> */
> UserBareMetal;
>
> public static boolean isSystemVM(VirtualMachine.Type vmtype)
> {
>
> if (DomainRouter.equals(vmtype)
> || ConsoleProxy.equals(vmtype)
> || SecondaryStorageVm.equals(vmtype) ||
> InternalLoadBalancerVm.equals(vmtype)) {
> return true;
> }
> return false;
> }
> }
>
> Vs
>
> public enum Type {
> User(false),
> DomainRouter(true),
> ConsoleProxy(true),
> SecondaryStorageVm(true),
> ElasticIpVm(true),
> ElasticLoadBalancerVm(true),
> InternalLoadBalancerVm(true),
>
> /*
> * UserBareMetal is only used for selecting
> VirtualMachineGuru, there is no
> * VM with this type. UserBareMetal should treat exactly as
> User.
>
> */
> UserBareMetal(false);
>
> private boolean _isSystemVm;
>
> private Type(Boolean isSystemVm) {
> _isSystemVm = isSystemVm;
> }
>
> public boolean isSystemVM() {
> return _isSystemVm;
> }
> }
>
> The second declaration is more self-descriptive:
>
> - It tells developer when they add more to this enum, they have to
> specify whether it is a system vm. They may have missed the static
> method in the first declaration and causes failures later.
> - It tells developers using the enum that there's a property that
> let's them know it is a system vm so they can do discovery using a
> context-sensitive editor like Eclipse.
>
> This is the reason why enum is not a simple constant declaration in
> Java (vs C/C++).
> --Alex
>
>
>
