4
\$\begingroup\$

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<EntryData>
    {
        @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<EntryData> 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 < dataSize; i++)
      {
        EntryData currentData = data.get(i);

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

        if (i < dataSize - 1 && currentData.getEpochTime() == data.get(i + 1).getEpochTime())
            continue;

        if (count > maxCount)
        {
            maxCount = count;
            result[0] = currentData.getEpochTime();

            if (i < dataSize - 1)
                result[1] = data.get(i + 1).getEpochTime();
            else
                result[1] = data.get(i).getEpochTime();

            result[2] = maxCount;
        }
    }

    return result;
  }

  public static void main(String args[])
  {
      MallTimeWithMaximumPeople instance = new MallTimeWithMaximumPeople();

      List<EntryData> data = new ArrayList<EntryData>();
      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);
  }
}
\$\endgroup\$
  • 4
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Sep 10 '16 at 21:15
6
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

This seems a bit unnecessary:

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);

You could just put each line inside one data.add() without creating a variable for each one. There might also be a cleaner way of doing this, but I'm not quite sure.

Also, in general, you shouldn't have multiple statements on one line:

data.add(data1); data.add(data2); data.add(data3);

Although they're similar, this isn't so easy to read and could result in much longer horizontal space if more variables are later added. Just keep one statement per line.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.