cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Frank Zhang <Frank.Zh...@citrix.com>
Subject RE: [DESIGN] Why is enum a class...
Date Tue, 23 Jul 2013 18:37:10 GMT
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



Mime
View raw message