• 🏆 Texturing Contest #33 is OPEN! Contestants must re-texture a SD unit model found in-game (Warcraft 3 Classic), recreating the unit into a peaceful NPC version. 🔗Click here to enter!
  • It's time for the first HD Modeling Contest of 2024. Join the theme discussion for Hive's HD Modeling Contest #6! Click here to post your idea!

[Java] Code Review?

Status
Not open for further replies.
Level 31
Joined
Jul 10, 2007
Messages
6,306
Hopefully I have gotten better at legit coding since the last time I posted something. I'm actually writing this with some serious seriousness ^)^.

I am using the standard Java things as well as Guava from Google.

I haven't documented some of the last additions to the code yet, so forgive me for that.


Now yes, I know that an immutable class should be final, but this is a better API.


I actually tried to code some of this pretty efficiently as well. I originally had some simpler code for building the map. I'd first flatten everything, which made the stuff easier to work with. However, that had quite a bit more overhead, so I made the single monstrous method instead >.<.

I plan on using this class (and many others) in a symbol table resources for general compilers coded in Java ; D. Right now I'm recoding quite a bit of the symbol table to be better and use more Java things. Originally, a Symbol Privilege could only be a composite of one derived type. It also relied on reflection to instantiate that derived type in unions/intersections. Another class called SymbolPrivilegeSet handled a variety of symbol privilege types, but it required a very rigid skeleton because it used a linked. The privilege types had to be the same on everything and they had to be in the same order. Not only that, but I had repeated classes (SymbolPrivilegeRegional, SymbolPrivilegeSetRegional). I think this is quite a bit better.

Java:
package symboltable;

import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;

/*
 * public class SymbolPrivilege implements Collection<SymbolPrivilege>
 * 
 * 	Constructors
 * 	-	public SymbolPrivilege()
 * 	-	public SymbolPrivilege(ImmutableMap<Class<? extends SymbolPrivilege>, ImmutableSet<SymbolPrivilege>> map)
 * 	-	public static SymbolPrivilege createEmpty()
 * 
 * 	Methods
 * 	-	protected boolean acceptsMask(ImmutableSet<SymbolPrivilege> requiredMask, ImmutableSet<SymbolPrivilege> providedMask)
 * 	-	public static SymbolPrivilege intersect(Collection<? extends Collection<?>>... collections)
 * 	-	public static SymbolPrivilege union(Collection<? extends Collection<?>>... collections)
 * 	-	public boolean accepts(SymbolPrivilege providedPrivileges)
 * 	-	public static boolean isSymbolPrivilege(Object sybmolPrivilege)
 * 
 * 	Basic Methods
 * 	-	public boolean contains(Object object)
 * 	-	public boolean containsAll(Collection<?> collection)
 * 	-	public boolean equals(Object symbolPrivilege)
 * 	-	public boolean isEmpty()
 * 	-	public int size()
 * 	-	public Iterator<SymbolPrivilege> iterator()
 * 	-	public Object[] toArray()
 * 	-	public <T> T[] toArray(T[] a)
 */

/**
 * A symbol privilege grants rights to access another symbol privilege. These rights are stored as
 * a set of flags. Accessing a symbol privilege requires the flags of that symbol privilege. The flags
 * provided are from the accessing symbol privilege.
 * <p>
 * A symbol privilege can intersect and union with collections. However, terminal nodes of these
 * collections must be symbol privileges.
 * <p>
 * A symbol privilege can be used for things like access modifiers or requirements
 * to go down a link in a virtual network. The default implementation dictates are
 * that all requirements are met if the required set of flags are a subset of the
 * provided set of flags.
 */
public class SymbolPrivilege implements Set<SymbolPrivilege>
{
	/**
	 * A map from this point on is a Map containing sets, which in turn contain privileges.
	 * A mask from this point on is a Set containing privileges.
	 * The map organizes the privileges by class. Without this organization, privileges
	 * would not be able to interact with each other without having both the same types
	 * of privileges and the same order.
	 */
	private final ImmutableMap<Class<? extends SymbolPrivilege>, ImmutableSet<SymbolPrivilege>>	map;

	/**
	 * This constructor is used when defining a new privilege.
	 * This new privilege is used in composite privileges.
	 */
	public SymbolPrivilege()
	{
		map = ImmutableMap.of(getClass(), ImmutableSet.of(this));
	} // SymbolPrivilege

	/**
	 * This constructor is used by composite privileges.
	 * 
	 * @param mask				the union/intersection of flags from several privileges
	 */
	public SymbolPrivilege(ImmutableMap<Class<? extends SymbolPrivilege>, ImmutableSet<SymbolPrivilege>> map)
	{
		this.map = map;
	} // SymbolPrivilege

	/**
	 * This is used to create an empty symbol privilege. It can be useful for
	 * things that don't actually require any privileges.
	 * 
	 * @return					empty privilege
	 */
	public static SymbolPrivilege createEmpty()
	{
		return new SymbolPrivilege(ImmutableMap.of());
	} // createEmpty

	/**
	 * This should be implemented by derived classes. This default implementation determines whether the provided
	 * privileges are accepted by the required privileges or not.
	 * 
	 * @param requiredMask		privileges required for success
	 * @param providedMask		privileges provided
	 * @return					provided privileges were accepted
	 */
	protected boolean acceptsMask(ImmutableSet<SymbolPrivilege> requiredMask, ImmutableSet<SymbolPrivilege> providedMask)
	{
		// the base privilege requires that provided privileges has all required
		// privileges
		return providedMask.containsAll(requiredMask);
	} // acceptsMask

	/**
	 * This method is here because arguments to generic functions must be exact.
	 * A HashMap<Set> is different from a HashMap<ImmutableSet>.
	 * 
	 * @param map				mask to add to
	 * @param mapToAdd			mask to add
	 */
	private static <T extends Set<SymbolPrivilege>> void addMap(Map<Class<? extends SymbolPrivilege>, HashSet<SymbolPrivilege>> map,
																Map<Class<? extends SymbolPrivilege>, T> mapToAdd)
	{
		// union the two masks together
		for (Entry<Class<? extends SymbolPrivilege>, T> entry : mapToAdd.entrySet())
		{
			if (!map.containsKey(entry.getKey()))
			{
				map.put(entry.getKey(), new HashSet<SymbolPrivilege>());
			} // if

			map.get(entry.getKey()).addAll(entry.getValue());
		} // for
	} // addMap

	/**
	 * This method is here because arguments to generic functions must be exact.
	 * A HashMap<Set> is different from a HashMap<ImmutableSet>.
	 * 
	 * @param map				mask to intersect
	 * @param mapToAdd			mask to retain values of
	 */
	private static <T extends Set<SymbolPrivilege>> void retainMap(Map<Class<? extends SymbolPrivilege>, HashSet<SymbolPrivilege>> map,
																	Map<Class<? extends SymbolPrivilege>, T> mapToAdd)
	{
		// intersect types
		for (Entry<Class<? extends SymbolPrivilege>, HashSet<SymbolPrivilege>> entry : map.entrySet())
		{
			if (!mapToAdd.containsKey(entry.getKey()))
			{
				map.remove(entry.getKey());
			} // if
		} // for

		// intersect masks
		for (Entry<Class<? extends SymbolPrivilege>, T> entry : mapToAdd.entrySet())
		{
			if (map.containsKey(entry.getKey()))
			{
				map.get(entry.getKey()).retainAll(entry.getValue());
			} // if
		} // for
	} // retainMap

	/**
	 * Builds a map from a collection of privileges. The collection
	 * may be of other collections as well.
	 * 
	 * @param collections		collections, privileges, etc
	 * @return					map of all privileges
	 */
	@SuppressWarnings("unchecked")
	@SafeVarargs
	private static Map<Class<? extends SymbolPrivilege>, HashSet<SymbolPrivilege>> buildMap(Collection<? extends Collection<?>>... collections)
	{
		// this is the composite mask of all passed in privileges
		HashMap<Class<? extends SymbolPrivilege>, HashSet<SymbolPrivilege>> map =
																					new HashMap<Class<? extends SymbolPrivilege>, HashSet<SymbolPrivilege>>();

		for (Collection<? extends Collection<?>> collection : collections)
		{
			// treat null privileges as the empty set
			if (collection != null)
			{
				if (isSymbolPrivilege(collection))
				{
					// if the privilege is a symbol privilege, add the
					// privilege's mask to the mask being built
					addMap(map, ((SymbolPrivilege) collection).map);
				} // if
				else
				{
					// if the privilege is a collection, then build a mask from
					// it
					addMap(map, buildMap((Collection<? extends Collection<?>>[]) collection.toArray()));
				} // else
			} // if
		} // for

		return map;
	} // buildMap

	/**
	 * This compiles a map into a SymbolPrivilege. Each set is first converted into an immutable
	 * set. After this, the map is converted into an immutable map.
	 * 
	 * @param map				map to compile
	 * @return					privilege with map
	 */
	private static SymbolPrivilege compileMap(Map<Class<? extends SymbolPrivilege>, HashSet<SymbolPrivilege>> map)
	{
		// have to create a new map to specifically hold ImmutableSet
		Map<Class<? extends SymbolPrivilege>, ImmutableSet<SymbolPrivilege>> transitionMap =
																								new HashMap<Class<? extends SymbolPrivilege>, ImmutableSet<SymbolPrivilege>>();

		// add the old map to the new map, converting Set to ImmutableSet
		for (Map.Entry<Class<? extends SymbolPrivilege>, HashSet<SymbolPrivilege>> entry : map.entrySet())
		{
			// if the mask isn't required, there is no point in
			// evaluating the mask
			if (!entry.getValue().isEmpty())
			{
				transitionMap.put(entry.getKey(), ImmutableSet.copyOf(entry.getValue()));
			} // if
		} // for

		return new SymbolPrivilege(ImmutableMap.copyOf(transitionMap));
	} // compileMap

	/**
	 * This determines if a map is empty. Requires a special check because
	 * a map is empty if either the HashMap itself is empty or the sets it
	 * contains are empty.
	 * 
	 * @param map				map to check for emptiness
	 * @return					is the map empty?
	 */
	private static <T extends Set<SymbolPrivilege>> boolean isMapEmpty(Map<Class<? extends SymbolPrivilege>, T> map)
	{
		for (T mask : map.values())
		{
			if (!mask.isEmpty())
			{
				return false;
			} // if
		} // for

		return true;
	} // isMapEmpty

	/**
	 * This is used to directly intersect a set of collections together into a map. A HashSet
	 * is used rather than an ImmutableSet because it is more efficient than recreating
	 * the structure over and over again. This HashSet is the map of the intersection.
	 * <p>
	 * A map is returned instead of a privilege because the method is recursive and
	 * adds the returned value to itself.
	 * 
	 * @param symbolPrivileges		set of collections of privileges to intersect
	 * @return						map of privilege
	 */
	@SuppressWarnings("unchecked")
	@SafeVarargs
	private static Map<Class<? extends SymbolPrivilege>, HashSet<SymbolPrivilege>> intersectToMap(	Collection<? extends Collection<?>>... collections)
	{
		Map<Class<? extends SymbolPrivilege>, HashSet<SymbolPrivilege>> map = buildMap(collections);

		// intersect with each individual privilege
		for (Collection<? extends Collection<?>> collection : collections)
		{
			// if the map is already empty, there is no point in continuing
			// an intersection with the empty set is still the empty set
			if (isMapEmpty(map))
			{
				break;
			} // if

			if (collection == null)
			{
				// if the privilege is null, treat it as the empty set
				map.clear();
			} // if
			else if (isSymbolPrivilege(collection))
			{
				// only retain privileges contained in the privilege
				retainMap(map, ((SymbolPrivilege) collection).map);
			} // else
			else
			{
				retainMap(map, intersectToMap((Collection<? extends Collection<?>>[]) collection.toArray()));
			} // else
		} // for

		return map;
	} // intersectToMap

	/**
	 * This returns a composite SymbolPrivilege from the intersection of composite SymbolPrivileges
	 * passed in. If a non-composite SymbolPrivilege is passed in, it will return a composite containing
	 * only that one privilege.
	 * 
	 * @param collections			collections of symbol privileges
	 * @return						intersection of the passed in privileges
	 */
	@SafeVarargs
	public static SymbolPrivilege intersect(Collection<? extends Collection<?>>... collections)
	{
		return compileMap(intersectToMap(collections));
	} // intersect

	/**
	 * This returns a composite SymbolPrivilege from the union of SymbolPrivileges passed in.
	 * 
	 * @param collections			collections of symbol privileges
	 * @return						union of the passed in privileges
	 */
	@SafeVarargs
	public static SymbolPrivilege union(Collection<? extends Collection<?>>... collections)
	{
		return compileMap(buildMap(collections));
	} // union

	/**
	 * Checks to see if an object is an instance of SymbolPrivilege.
	 *
	 * @param				an Object that may be a SymbolPrivilege
	 * @return				is the object a SymbolPrivilege?
	 */
	public static boolean isSymbolPrivilege(Object sybmolPrivilege)
	{
		return SymbolPrivilege.class.isAssignableFrom(sybmolPrivilege.getClass());
	} // isSymbolPrivilege

	/**
	 * This will return true if the provided privileges are accepted by this
	 * privilege.
	 * 
	 * @param this				 required privileges
	 * @param providedPrivileges provided privileges
	 * @return					 provided privileges accepted?
	 */
	public boolean accepts(SymbolPrivilege providedPrivileges)
	{
		ImmutableSet<SymbolPrivilege> providedMask;

		// all masks must be accepted
		for (Map.Entry<Class<? extends SymbolPrivilege>, ImmutableSet<SymbolPrivilege>> entry : this.map.entrySet())
		{
			// retrieve the provided mask, which may not exist
			if (providedPrivileges.map.containsKey(entry.getKey()))
			{
				providedMask = providedPrivileges.map.get(entry.getKey());
			} // if
			else
			{
				providedMask = ImmutableSet.of();
			} // else

			// in compileMap, empty masks are not included in the final map
			// as such, this will always be safe
			// furthermore, if no privileges are required, then the provided
			// privileges should always be accepted
			// "this" isn't used because "this" isn't a derived class
			if (!entry.getValue().iterator().next().acceptsMask(entry.getValue(), providedMask))
			{
				// only exit if not accepted, otherwise keep going
				return false;
			} // if
		} // for

		// if all masks were accepted, then the privileges are accepted
		return true;
	} // accepts

	/**
	 * This will return true if the map contains the object. This
	 * will only true if the object is a derived type of SymbolPrivilege.
	 * 
	 * @param object			the unknown object
	 * @return					contains object?
	 */
	@Override
	public boolean contains(Object object)
	{
		// object is a privilege
		// map contains the object's type
		// mask contains the object
		return isSymbolPrivilege(object) && map.containsKey(object.getClass()) && map.get(object.getClass()).contains(object);
	} // contains

	/**
	 * This will return true if all of the elements of the collection are contained.
	 * 
	 * @param c					some Collection (symbol privilege or otherwise)
	 * @return					does this privilege contain all elements?
	 */
	@Override
	public boolean containsAll(Collection<?> collection)
	{
		// the collection has objects in it, but the types of objects are
		// unknown
		for (Object object : collection)
		{
			// a privilege is also a collection
			if (Collection.class.isAssignableFrom(object.getClass()))
			{
				if (isSymbolPrivilege(object))
				{
					// this will take each element of the object and then make
					// sure that this map contains it and every element it has
					// if the map doesn't contain it, return false
					for (Map.Entry<Class<? extends SymbolPrivilege>, ImmutableSet<SymbolPrivilege>> entry : ((SymbolPrivilege) object).map.entrySet())
					{
						if (!map.containsKey(entry.getKey()) || !map.get(entry.getKey()).containsAll(entry.getValue()))
						{
							return false;
						} // if
					} // for
				} // if
				else
				{
					// this will occur when the object is a collection, and in
					// that case, call the method on it again
					if (!containsAll(((Collection<?>) object)))
					{
						return false;
					} // if
				} // else
			} // if
			else
			{
				// if the object was not a collection, then
				return false;
			} // else
		} // for

		return true;
	} // containsAll

	/**
	 * This will return true if the passed in object is an instance of SymbolPrivilege
	 * and the two compared SymbolPrivileges contain each other.
	 * 
	 * @return					do the symbol privileges contain each other?
	 */
	@Override
	public boolean equals(Object symbolPrivilege)
	{
		return isSymbolPrivilege(symbolPrivilege) && this.containsAll((SymbolPrivilege) symbolPrivilege)
				&& ((SymbolPrivilege) symbolPrivilege).containsAll(this);
	} // equals

	/**
	 * This determines whether the symbol privilege mask is empty or not.
	 * 
	 * @return					does the symbol privilege contain any flags
	 */
	@Override
	public boolean isEmpty()
	{
		return isMapEmpty(map);
	} // isEmpty

	public static class PrivilegeIterator implements Iterator<SymbolPrivilege>
	{
		private Iterator<ImmutableSet<SymbolPrivilege>>	maskIterator;
		private Iterator<SymbolPrivilege>				privilegeIterator;

		public PrivilegeIterator(SymbolPrivilege privilege)
		{
			maskIterator = privilege.map.values().iterator();
			privilegeIterator = maskIterator.next().iterator();
		} // PrivilegeIterator

		private void nextMask()
		{
			while (!privilegeIterator.hasNext() && maskIterator.hasNext())
			{
				privilegeIterator = maskIterator.next().iterator();
			} // if
		} // nextMask

		public boolean hasNext()
		{
			nextMask();

			return maskIterator.hasNext() || privilegeIterator.hasNext();
		} // hasNext

		public SymbolPrivilege next()
		{
			nextMask();

			return privilegeIterator.next();
		} // next

		@Deprecated
		public void remove()
		{
		} // remove
	} // PrivilegeIterator

	/**
	 * How many flags the mask contains.
	 * 
	 * @return					flag count
	 */
	@Override
	public int size()
	{
		int length = 0;

		for (ImmutableSet<SymbolPrivilege> mask : map.values())
		{
			length += mask.size();
		} // for

		return length;
	} // size

	/**
	 * This will return an iterator to loop over the mask.
	 * 
	 * @return					iterator from mask
	 */
	@Override
	public Iterator<SymbolPrivilege> iterator()
	{
		return new PrivilegeIterator(this);
	} // iterator

	private static ImmutableSet<SymbolPrivilege> flattenMap(ImmutableMap<Class<? extends SymbolPrivilege>, ImmutableSet<SymbolPrivilege>> map)
	{
		ImmutableSet.Builder<SymbolPrivilege> builder = new ImmutableSet.Builder<SymbolPrivilege>();

		for (ImmutableSet<SymbolPrivilege> entry : map.values())
		{
			builder.addAll(entry);
		} // for

		return builder.build();
	} // flattenMap

	/**
	 * This will return the elements of the mask in an array.
	 * 
	 * @return					array containing elements of mask
	 */
	@Override
	public Object[] toArray()
	{
		return flattenMap(map).toArray();
	} // toArray

	/**
	 * This will return the elements of the mask in an array.
	 * 
	 * @return					array containing elements of mask
	 */
	@Override
	public <T> T[] toArray(T[] a)
	{
		return flattenMap(map).toArray(a);
	} // toArray

	/**
	 * This method will not do anything and always return false because privileges are
	 * immutable.
	 */
	@Deprecated
	@Override
	public boolean add(SymbolPrivilege e)
	{
		return false;
	} // add

	/**
	 * This method will not do anything and always return false because privileges are
	 * immutable.
	 */
	@Deprecated
	@Override
	public boolean remove(Object o)
	{
		return false;
	} // remove

	/**
	 * This method will not do anything and always return false because privileges are
	 * immutable.
	 */
	@Deprecated
	@Override
	public boolean addAll(Collection<? extends SymbolPrivilege> c)
	{
		return false;
	} // addAll

	/**
	 * This method will not do anything and always return false because privileges are
	 * immutable.
	 */
	@Deprecated
	@Override
	public boolean removeAll(Collection<?> c)
	{
		return false;
	} // removeAll

	/**
	 * This method will not do anything and always return false because privileges are
	 * immutable.
	 */
	@Deprecated
	@Override
	public boolean retainAll(Collection<?> c)
	{
		return false;
	} // retainAll

	/**
	 * This method will not do anything because privileges are immutable.
	 */
	@Deprecated
	@Override
	public void clear()
	{

	} // clear
} // SymbolPrivilege

And here is a custom privilege to show how easy the thing above is to work with ;D.

Java:
package symboltable;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;

/*
 * public class SymbolPrivilegeRegional extends SymbolPrivilege
 * 
 * 	Constructors
 * 	-	public SymbolPrivilegeRegional()
 * 	-	public SymbolPrivilegeRegional(HashSet<SymbolPrivilege> mask)
 */

/**
 * Regional privileges deal with movement between groups of
 * symbols. Symbols can be grouped in any way, but the most
 * common grouping method is by file. C# provides the access
 * modifier internal to limit access to a symbol by file.
 * <p>
 * In this case, the provided privileges are the region of the
 * accessing symbol and the required privileges are the region of
 * the accessed symbol.
 * <p>
 * If the accessed symbol is not contained within a region, that is
 * the required privileges are empty, then the accessed symbol can
 * always be accessed.
 * <p>
 * If the access symbol is contained within a region, then it
 * can only be accessed if the accessing symbol shares some
 * region with the accessed symbol: the intersection
 * between the two sets of regions is not empty.
 */
public class SymbolPrivilegeRegional extends SymbolPrivilege
{
	/**
	 * Used when defining a new privilege.
	 */
	public SymbolPrivilegeRegional()
	{
		super();
	} // SymbolPrivilegeRegional

	/**
	 * Used when defining composite privileges.
	 */
	public SymbolPrivilegeRegional(ImmutableMap<Class<? extends SymbolPrivilege>, ImmutableSet<SymbolPrivilege>> map)
	{
		super(map);
	} // SymbolPrivilegeRegional

	/**
	 * Returns true if all of the elements of the destination
	 * are contained
	 * 
	 * @param this				 provided privileges
	 * @param requiredPrivileges required privileges
	 * @return					 provided privileges accepted?
	 */
	@Override
	protected boolean acceptsMask(ImmutableSet<SymbolPrivilege> requiredMask, ImmutableSet<SymbolPrivilege> providedMask)
	{
		return requiredMask.isEmpty() || !Sets.intersection(requiredMask, providedMask).isEmpty();
	} // acceptsMask
} // SymbolPrivilegeRegional
 

Deleted member 177737

D

Deleted member 177737

I'm just writing down these notes as I look over both of the files, so here goes...

First File:
  1. The amount of comments and JavaDoc is completely unnecessary.
  2. Remove the comments showing the ending brace for methods, if, if-else, for, while, class, etc... You don't need them in the vast majority of cases.
  3. You don't need a massive JavaDoc at the top of the class that outlines the methods and constructors. Most people will just do a quick scroll down and look themselves, but that's only if the IDE doesn't just show the available options when you go to use them.
  4. Other than the comments it looks fine.

Second File:
  1. Just as the first file, this seems to have a few unnecessary comments.
  2. Code looks fine.
 

Dr Super Good

Spell Reviewer
Level 63
Joined
Jan 18, 2005
Messages
27,191
Why are there deprecated methods in something brand new? Those methods should not be deprecated as the interface (java.util.Set) has them marked as not deprecated.

If they are not implemented (do nothing/not supported) then to be compliant with the Java standard they should throw an "UnsupportedOperationException" (an Error) exception when called. You should then state somewhere in the docs that they are not supported operations on that implementation of Set.

As far as I am aware deprecated is intended for use on methods that were once a core part of an interface but are no longer required and only there for backwards compatibility. The methods marked as deprecated still work as described however they are only there for backwards compatibility with components that use an old version of the interface. Deprecated functions should never be used by freshly written code as they might be subject to removal in a future revision.

An example could be a file I/O component where at first a basic I/O interface existed but was eventually expanded and revised adding an entirely new I/O interface to it. The I/O component can be updated and existing components will still work under the new system but will do so using the deprecated interface. Eventually when a component which uses the deprecated interface is revised it can be changed to use the new interface. Once all components use the new interface the next revision of the file I/O component might remove the deprecated member from its interface, breaking support for old versions of whatever used it.

Those methods are not "deprecated" since they are still a core part of the interfaces used (they are not deprecated in the interface). It is possible that companies like Google are using deprecated as a means of error checking (as it usually warns the programmer the method is deprecated) and not for its intended purpose. Normally the Set interface will throw a run time error when an unsupported method is called, which is a worse failure time than some warning in an IDE.
 
Status
Not open for further replies.
Top