Busiest time in mall

The mall management is trying to figure out what was the busiest
moment in the mall in the last year. Each data entry includes a
timestamp (seconds in Unix Epoch format), an amount of people and
whether they entered or exited.


Example:

{ time: 1440084737,  count: 4,  ActionType: "Entry" }

How can I refactor this code?

public class MallTimeWithMaximumPeople
{
    public class DataComparision implements Comparator
    {
        @Override
        public int compare(EntryData data1, EntryData data2)
        {
           return (data1.epochTime - data2.epochTime);
        }
    }

    public class EntryData
    {
       private int count, epochTime;
       private ActionType actionType;

    public EntryData(int count, int epochTime, ActionType actionType)
    {
        this.count = count;
        this.epochTime = epochTime;
        this.actionType = actionType;
    }

    public int getCount()
    {
        return count;
    }

    public long getEpochTime()
    {
        return epochTime;
    }

    public ActionType getActionType()
    {
        return actionType;
    }
  }

  public enum ActionType
  {
     ENTRY, EXIT;
  }

  public long[] getTimeWithMaximumPeople(List data)
  {
      long[] result = { 0, 0, 0 };

      if (data == null || data.size() == 0)
        return result;

      Collections.sort(data, new MallTimeWithMaximumPeople.DataComparision());

      int dataSize = data.size();
      int count = 0, maxCount = 0;

      for (int i = 0; i  maxCount)
        {
            maxCount = count;
            result[0] = currentData.getEpochTime();

            if (i  data = new ArrayList();
      EntryData data1 = instance.new EntryData(100, 0, ActionType.ENTRY);
      EntryData data3 = instance.new EntryData(1, 1, ActionType.EXIT);
      EntryData data2 = instance.new EntryData(20, 2, ActionType.EXIT);
      data.add(data1); data.add(data2); data.add(data3);

      long[] result = instance.getTimeWithMaximumPeople(data);
  }
}

Don't abuse inner classes

Both classes DataComparision (beware of the typo here), and EntryData are inner classes of MallTimeWithMaximumPeople. However, those two classes do not access the fields of their enclosing instance. The Java tutorials have:


Use a non-static nested class (or inner class) if you require access to an enclosing instance's non-public fields and methods. Use a static nested class if you don't require this access.

In this case, consider:

• Using a static nested class instead. This means declaring both classes static.

• Using a top-level class for each of those.

Possible bug

The DataComparision comparator has a subtle corner-case. Currently, it relies on the return value of

data1.epochTime - data2.epochTime

to determine if data1 is greater than, equal to, or less than data2. However, consider the example case of data1.epochTime = Integer.MIN_VALUE and data2.epochTime = Integer.MAX_VALUE:

System.out.println(Integer.MIN_VALUE - Integer.MAX_VALUE); // prints 1

This actually prints a positive number due to integer overflow, so the code would consider the event data1 to be after data2. Of course, this supposes negative values are accepted, which in your case, is probably not wanted (although it is not validated).

Even if it doesn't concern you, it would be best to fix this by just calling Integer.compare instead of relying on the result of this subtraction.

Epoch time is a long

Unless you want your program to crash in 2038, store the number of seconds since the Epoch in a long, and not in an int. The maximum integer value, which is Integer.MAX_VALUE can only represent, in seconds, a date up to January 2038. If you want to be convinced of it, print the following:

System.out.println(new SimpleDateFormat("dd/MM/yyyy HH:mm:ss").format(new Date(1000l * Integer.MAX_VALUE)));

Also, if on Java 8, consider using an Instant instead of directly dealing with long values.

Namings

The method getTimeWithMaximumPeople doesn't actually return the time with the maximum people, despite its name. It also returns additional information, namely, the next Epoch time in the data where that maximum was reached and the maximum count.

You have multiple approach to tackle this:

• Only return the time, since this is what is strictly wanted.

• Rename that method, and possibly wrap the 3 information in an object, instead of an array. Having an object would make it easier to code since the client can directly invoke descriptive methods like getMaximumCount() on it, instead of accessing a value in an array, which obscures the intent.

Other comments

-

data.size() == 0

is better expressed with data.isEmpty().

-
Use curly braces, even for small if / else statements.

if (currentData.getActionType() == ActionType.ENTRY)
    count += currentData.getCount();
else
    count -= currentData.getCount();

It is a best-practice to explicitely add the curly braces, as they prevent future possible issues.

StackExchange Code Review Q#141037, answer score: 6