Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cloudstack.apache.org
Received-SPF: pass (athena.apache.org: domain of daan.hoogland@gmail.com
 designates 209.85.214.171 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <46B74B18-8416-48F8-831C-E4742EC7B28C@basho.com>
References: <20CF38CB4385CE4D9D1558D52A0FC0580818F9@SJCPEX01CL03.citrite.net>
 <20CF38CB4385CE4D9D1558D52A0FC05808192A@SJCPEX01CL03.citrite.net>
 <F7F94CD9B5E67A448CE8C69340A88B4908EB2D@SJCPEX01CL03.citrite.net>
 <20CF38CB4385CE4D9D1558D52A0FC05808197D@SJCPEX01CL03.citrite.net>
 <CAGQtxvZBzZ6kg2TmKfzXqeK3+Ac9yby6vL0vXF3tW8dYCpGXHw@mail.gmail.com>
 <8748335A-1DBA-4ABD-A831-DDBBBF60FA81@basho.com>
 <F7F94CD9B5E67A448CE8C69340A88B4908FB19@SJCPEX01CL03.citrite.net>
 <CAGQtxvZ_j9LaSc3=JLtA57kxSrvGpoRdThhKsubS08s7gcQCkg@mail.gmail.com>
 <46B74B18-8416-48F8-831C-E4742EC7B28C@basho.com>
From: Daan Hoogland <daan.hoogland@gmail.com>
Date: Wed, 24 Jul 2013 15:19:01 +0200
Message-ID: 
 <CAGQtxvZ=vkrqckQV7no2ZOR3gG9zhHc-yp1pZTATJy0gYYCb8Q@mail.gmail.com>
Subject: Re: [DESIGN] Why is enum a class...
To: dev <dev@cloudstack.apache.org>
Content-Type: multipart/alternative; boundary=001a11c258380c496804e241c2e0

--001a11c258380c496804e241c2e0
Content-Type: text/plain; charset=ISO-8859-1

And as an example: the way the net should be isolated or the broadcast
should be constructed are typically behavioral issues, whilst the fact that
it is being used for storage or private network or routing can be caught in
a constant/enum value. right?


On Wed, Jul 24, 2013 at 2:37 PM, John Burwell <jburwell@basho.com> wrote:

> Daan and Frank,
>
> First, I think its important to separate when and where an enumeration
> should be used from how to best employ enums when their use is appropriate.
>  Enums are a finite set of values defined at compile-time.  Therefore, they
> shouldn't employed in situations where extensibility of the value set is
> required whether by clients (including plugins).
>
> For plugins in particular, I think we should be preferring interfaces for
> all extension points.  To this end, our plugin model should be defining
> behavioral, not data, extension points to reduce coupling between the
> orchestration components and plugins.  If we are running into situations
> where plugins can't extend a data structure such as an enumeration, the
> root cause it likely that we have too tightly coupled the plugin and should
> refine to a behavioral extension point.
>
> Thanks,
> -John
>
> On Jul 24, 2013, at 6:31 AM, Daan Hoogland <daan.hoogland@gmail.com>
> wrote:
>
> > 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.Zhang@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:jburwell@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.hoogland@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.Huang@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.Zhang@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.Huang@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.Huang@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
> >>
> >>
> >>
>
>

--001a11c258380c496804e241c2e0--