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