NotSoPoorGroup.java

package com.examplefoobar.utils.improve;

import java.io.*;
import java.util.List;
import java.util.Set;
import java.util.concurrent.*;
import java.util.concurrent.atomic.AtomicBoolean;
import javax.annotation.concurrent.ThreadSafe;

import static java.util.Objects.*;

/**
 * A thread-safe container that stores a group ID and members.
 *
 * It can be added <tt>Member</tt> and return a member list as String.
 * Also, it can start and stop a background task that writes a member list to
 * specified files.
 *
 * This class is called a lot, so we need improve it.
 *
 * HuyLe: There are may be still some code smells left in the refactor code.
 * But the biggest there is: overly commented. It is done on purpose to explain
 * the rationale behinds these refactors.
 */
@ThreadSafe
public class NotSoPoorGroup {

  // Instance fields should has private access by default.
  // It's not likely that groupId will ever change in the course of
  // NotSoPoorGroup life, it should be final.
  private final String groupId;
  // Since NotSoPoorGroup is thread-safe, it's members should be backed by a
  // concurrent data struct instead of the concrete HashSet
  private final Set<Member> members;
  // Like above, shouldStop flag should be modified atomically
  private final AtomicBoolean shouldStop = new AtomicBoolean();

  // We may perhaps allow to change the executor at runtime later
  private Executor executor;

  /**
   * Create a {@code NotSoPoorGroup} with the given parameters
   *
   * @param groupId the group identity
   * @param executor allow to plugin Executor for file IO
   *                 Example: One can has a shared ThreadPoolExecutor that
   *                 dedicated for IO operations with large number of threads
   *                 (as opposed to computation-intensive thread pool with a
   *                 few numbers of threads to reduce overhead of preemptive
   *                 threads, and leverage more of CPU cycles)
   */
  public NotSoPoorGroup(String groupId, Executor executor) {
    this.groupId = groupId;
    // As mentioned, members should be concurrently read/write-able
    this.members = ConcurrentHashMap.newKeySet();
    this.executor = executor;
  }

  /**
   * Create a {@code NotSoPoorGroup} with groupId and a default single thread
   * executor
   *
   * @param groupId the group identity
   */
  public NotSoPoorGroup(String groupId) {
    this(groupId, Executors.newSingleThreadExecutor(r -> {
      Thread t = Executors.defaultThreadFactory().newThread(r);
      // Since this is a background job.
      t.setDaemon(true);
      return t;
    }));

    // A hiccup of owning an executor instead of spawn a new thread each time
    // (or was supplied an executor from the caller) is it requires explicitly
    // shutdown for non-daemon thread. On the other hand it may be early
    // terminated for daemon thread.
    // The hook below helps executor to automatically shutdown gracefully when
    // application is complete.
    ExecutorService executor = (ExecutorService) this.executor;
    Runtime.getRuntime().addShutdownHook(new Thread(() -> {
      executor.shutdown();
      try {
        // Give the tasks a chance to complete at maximum 10 seconds
        if (!executor.awaitTermination(10000, TimeUnit.SECONDS)) {
          List<Runnable> remains = executor.shutdownNow();
          System.err.println(
                  "PoorGroup's executor while have not yet completed " +
                  remains.size() + " tasks.");
          executor.shutdownNow();
        }
      } catch (InterruptedException e) {
        e.printStackTrace();
      }
    }));

  }

  /**
   * Add a member if doesn't exists a semantically equal member (same memberId)
   * to the container
   *
   * @param member the member to add
   */
  public void addMember(Member member) {
    // Either member is concurrently modifiable or addMember is synchronized
    members.add(member);
  }

  /**
   * Run a background task that writes a member list to specified files 10 times
   * in background thread so that it doesn't block the caller's thread.
   *
   * @param outputFilePrimary the first output file
   * @param outputFileSecondary the second output file
   *
   * @throws UnaccessibleFileException if either one of the two files is inexist
   *                                   or not writeable
   */
  public void startLoggingMemberList10Times(String outputFilePrimary,
                                            String outputFileSecondary) {
    // Strings are already immutable, and since Java is pass by value, it is
    // safe to drop the final on the arguments without affects anything on the
    // call site. It still depends on the team's code styles on whether we
    // should final the arguments or even local variables.

    // An executor is used for scheduling the task instead of spawn a new thread
    // every times Java's thread is not green, OS thread. It is assured to be
    // expansive.
    executor.execute(() -> {
      int i = 0;
      // Writers should be shared outside the loop.
      // Since we write multiple times, it is more IO efficient to has a
      // BufferedWriter over the FileWriter
      try (Writer writer0 =
                   new BufferedWriter(new FileWriter(new File(outputFilePrimary)));
          Writer writer1 =
                   new BufferedWriter(new FileWriter(new File(outputFileSecondary)))
      ) {
        // A suffix ++ is evaluated to the previous value; which causes the
        // original code to has an off-by-one problem.
        // Also the that counter predicate can be expressed in the while loop
        // condition, so the loop exit latch can be easily read in one expression
        while (!shouldStop.get() && ++i <= 10) {
          String membersAsStringWith10xAge =
                  NotSoPoorGroup.this.getMembersAsStringWith10xAge();
          writer0.append(membersAsStringWith10xAge);
          writer1.append(membersAsStringWith10xAge);
        }
      // Narrow the scope of catching to IOException
      // When other kind of exceptions is thrown here we know an even wilder
      // thing happened and should let it handled by the upstream caller.
      } catch (IOException e) {
        // It's always better to include the original cause/exception when
        // propagating.  And when possible, throw more specific exception, so
        // the caller has a chance for branching logic on it catch clauses.
        // For example, if it is IO related, it may attempts to retry.
        throw new UnaccessibleFileException(
            "Unexpected error occurred. Please check these file names. \n"
                    + "outputFilePrimary=" + outputFilePrimary
                    + ", outputFileSecondary=" + outputFileSecondary,
                e);
      }
    });
  }

  /**
   * Stop the background task started by <tt>startPrintingMemberList()</tt>
   * Of course, <tt>startLoggingMemberList</tt> can be called again after
   * calling this method.
   */
  public void stopPrintingMemberList() {
    // As mentioned, either shouldStop is atomic or synchronized on every accesses.
    // Also, since stopPrintMemberList could be called from different threads,
    // simply marking volatile is not sufficient, there is still potential of
    // race conditions.
    shouldStop.set(true);
  }

  private String getMembersAsStringWith10xAge() {
    // Use StringBuilder, the original causes a new String to be allocated for
    // every concatenation in the below loop.
    // Also StringBuilder is sufficient since we don't concurrently operate on
    // it; StringBuffer will incur some overheads from synchronization.
    StringBuilder builder = new StringBuilder();
    for (Member member : members) {
      // Original code has several unnecessary boxing and unboxing introduced by
      // an extra temporary variable and an imperative assignment.
      builder.append(
              String.format("memberId=%s, age=%d¥n",
              member.getMemberId(), member.getAge()));
    }
    return builder.toString();
  }

  /**
   * Value object to represent a member
   */
  public static final class Member {
    // Member class should be static, otherwise a class object is allocated for
    // every instances of NotSoPoorGroup.
    // Value objects won't likely to has subclasses, let's make it final
    // Since Member is consumed through NotSoPoorGroup#addMember();
    // it should has the same public access level as NotSoPoorGroup.

    // POJO is always a promising candidate to be immutable
    private final String memberId;
    private final int age;

    /**
     * Create a new member with provided values
     *
     * @param memberId the ID of this member, this will determine the semantic
     *                 equality and identity of the member in the
     *                 {@link NotSoPoorGroup}
     * @param age member's age
     */
    public Member(String memberId, int age) {
      // For lightweight value objects like Member, usually aggressive null
      // guards to limit the edge cases is prefer.
      requireNonNull(memberId, "Member#memberId is not nullable!");
      this.memberId = memberId;
      this.age = age;
    }

    // These getters is only accessed by NotSoPoorGroup, it can be private.
    // The little we expose, the easier it is to change later.
    private String getMemberId() {
      return memberId;
    }

    private int getAge() {
      return age;
    }

    // Overridden method should be annotated @Override, let the compiler helps
    // you to detect typos. It's also help easing reader eyes navigation, IMHO.
    @Override
    public boolean equals(Object o) {
      // Should perform a type check to return false if `o` is null or not an
      // instance of Member, instead of potentially throwing a NullPointerException
      // or ClassCastException
      if (!(o instanceof Member)) {
        return false;
      }
      Member member = (Member) o;
      // The original intent seems to aim for semantic equality, the `memberId`
      // should be compared by `equals` instead of identical memory address.
      // Member#memberId is guaranteed non-null
      return this.memberId.equals(member.memberId);
    }

    // equals() and hashCode() must be implemented together by contract. For a
    // sole `equals()` implementation, we guarantee will break a  whole lot of
    // `members`'s Set operations.
    @Override
    public int hashCode() {
      // Member#memberId is guaranteed non-null
      return this.memberId.hashCode();
    }
  }

  /**
   * Marker runtime exceptions for file IO related failure
   */
  public static class UnaccessibleFileException extends RuntimeException {
    public UnaccessibleFileException(String message, Throwable cause) {
      super(message, cause);
    }
  }
}