Java coding challenge

DisappointingGroup.java

package com.examplefoobar.utils;

import javax.annotation.concurrent.ThreadSafe;
import java.io.Closeable;
import java.io.FileWriter;
import java.util.Collections;
import java.util.HashSet;
import java.util.Objects;
import java.util.concurrent.atomic.AtomicBoolean;

/**
 * FIXME: This class was written by a really junior software engineer.
 *        It seems there are a lot of rooms for improvement in this class.
 *        Please give us your feedback about this class (with suggestions if possible.)
 */

/**
 * A thread-safe container that stores a group ID and members.
 * <p>
 * This class can store <tt>Member</tt> and/or <tt>AdminMember</tt>.
 * Also, it can start and stop a background task that writes a member list to specified 2 files.
 * <p>
 * This class is called many times, so it should have a good performance.
 * <p>
 * Example usage:
 * <p>
 * DisappointingGroup group = new DisappointingGroup("group-001");
 * <p>
 * group.addMember(new DisappointingGroup.Member("member-100", 42));
 * group.addMember(new DisappointingGroup.AdminMember("admin-999", 30));
 * group.addMember(new DisappointingGroup.Member("member-321", 15));
 * <p>
 * group.startLoggingMemberList10Times("/tmp/output.primary", "/tmp/output.secondary");
 */
@ThreadSafe
public class DisappointingGroup implements Closeable {

    // synchronize required the filed is final
    // refer thread-safe collection & make it final
    private final HashSet<Member> members;

    private final AtomicBoolean isRunning;

    private final AtomicBoolean shouldStop;

    private final String groupId;


    public DisappointingGroup(String groupId) {
        this.groupId = groupId;
        // use thread-safe collection
        this.members = Collections.synchronizedSet(new HashSet<>());
        this.isRunning = new AtomicBoolean(false);
        this.shouldStop = new AtomicBoolean(false);
    }

    public void addMember(Member member) {
        synchronized (members) {
            members.add(member);
        }
        // members.add(member);
    }

    @Override
    public void close() {
        // remove un-throw exception, & format
    }

    /**
     * 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.
     * <p>
     * Only one thread is allowed to run at once
     * - When this method is called and another thread is running, the method call should just return w/o starting any thread
     * - When this method is called and another thread is already finished, the method call should start a new thread
     */
    public void startLoggingMemberList10Times(final String outputFilePrimary, final String outputFileSecondary) {
        // Only one thread is allowed to run at once
        if (isRunning.getAndSet(true)) {
            return;
        }
        // isRunning = true;

        // replace anonymous new Runnable() with lambda
        new Thread(() -> {
            int i = 0;
            while (!shouldStop.get()) {
                if (i++ >= 10)
                    break;

                //                FileWriter writer0 = null;
                //                FileWriter writer1 = null;
                // try with resource
                try (FileWriter writer0 = new FileWriter(outputFilePrimary);
                     FileWriter writer1 = new FileWriter(outputFileSecondary)) {
                    String membersStr = getMembersAsStringFlooringAge();

                    writer0.append(membersStr);

                    writer1.append(membersStr);
                } catch (Exception e) {
                    // should define custom exception instead throw generic one
                    throw new RuntimeException(
                            "Unexpected error occurred. Please check these file names. outputFilePrimary="
                                    + outputFilePrimary + ", outputFileSecondary=" + outputFileSecondary);
                }
//                finally {

//                comment out as use try with resource, it auto close
//                    try {
//                        if (writer0 != null)
//                            writer0.close();
//
//                        if (writer1 != null)
//                            writer1.close();
//                    } catch (Exception ignored) {
//                        // Do nothing since there isn't anything we can do here, right?
//                    }

//                }

                try {
                    Thread.sleep(1000);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        }).start();
    }

    private String getMembersAsStringFlooringAge() {
        // refer StringBuilder than String concat as memory space more efficient
        StringBuilder buf = new StringBuilder();
        synchronized (members) {
            for (Member member : members) {
                // Floor the age: e.g. 37 -> 30
                Integer flooredAge = (member.getAge() / 10) * 10;
                String decoratedMemberId = getDecoratedMemberId(member);
                buf.append(String.format("memberId=%s, age=%d¥n", decoratedMemberId, flooredAge));
            }
        }
        return buf.toString();
    }

    private String getDecoratedMemberId(Member member) {
        // if (member instanceof Member) {
        //     return member.getMemberId() + "(normal)";
        // }
        // else if (member instanceof AdminMember) {
        //     return member.getMemberId() + "(admin)";
        // }
        // return null;

        //refer new introduce variable admin
        return member.isAdmin() ? member.getMemberId() + "(admin)" : member.getMemberId() + "(normal)";
    }

    /**
     * Stop the background task started by <tt>startLoggingMemberList10Times()</tt>
     */
    public void stopPrintingMemberList() {
        shouldStop.getAndSet(true);
    }

    class Member {

        //admin does not have any specific logic, no need separate object
        private boolean admin;

        private int age;

        //make it private, encapsulation
        private String memberId;

        Member(String memberId, int age) {
            this.memberId = memberId;
            this.age = age;
        }

        public Member setIsAdmin(boolean admin) {
            this.admin = admin;
            return this;
        }

        public int getAge() {
            return age;
        }

        public Member setAge(int age) {
            this.age = age;

            //apply builder pattern
            return this;
        }

        public String getMemberId() {
            return memberId;
        }

        public Member setMemberId(String memberId) {
            this.memberId = memberId;
            return this;
        }

        // the equals method is override, then hashcode method should override as well
        @Override
        public int hashCode() {
            return memberId.hashCode();
        }

        @Override
        public boolean equals(Object o) {
            // If `memberId` matches the other's one, they should be treated as the same `Member` objects.

            // add more conditions
            if (o == this) {
                return true;
            }

            if (!(o instanceof Member)) {
                return false;
            }

            Member member = (Member) o;
            // refer null-safe method instead do ==
            return Objects.equals(this.memberId, member.memberId);
        }

        public boolean isAdmin() {
            return this.admin;
        }
    }

    // Deprecated this class due to it does not have any specific logic
    @Deprecated
    class AdminMember extends Member {

        AdminMember(String memberId, int age) {
            super(memberId, age);
        }
    }
}