Simple online auction system in Java

Question

I recently applied for a Java developer position and I was asked to completed a coding task.

After review of the coding task, the company said they do not want to take me forward to interview. They refused to give feedback so I have no idea of what went wrong.

I'd love some insights on what I did wrong.

The task:

You have been tasked with building part of a simple online auction system which will allow users to bid on items for sale.

Provide a bid tracker interface and concrete implementation with the following functionality:

• Record a user's bid on an item
• Get the current winning bid for an item
• Get all the bids for an item
• Get all the items on which a user has bid

You are not required to implement a GUI (or CLI) or persistent storage. You may use any appropriate libraries to help, but do not include the jars or class files in your submission.

The code:

The interface:

package com.nbentayou.app.service;

import com.nbentayou.app.exception.InvalidBidException;
import com.nbentayou.app.model.Bid;
import com.nbentayou.app.model.Item;
import com.nbentayou.app.model.User;

import java.util.List;
import java.util.Optional;
import java.util.Set;

/**
 * The interface for a Bid Tracker.
 * This interface exposes methods allowing {@link User}s to post {@link Bid}s on {@link Item}s
 * and query the current state of the auction.
 */
public interface BidTracker {

  /**
   * Records a bid for a given item.
   * @param bid the bid to record
   * @param item the item to bid on
   * @throws InvalidBidException when the bid is invalid
   */
  void recordUserBidOnItem(Bid bid, Item item) throws InvalidBidException;

  /**
   * @param item the item
   * @return the current winning bid (last valid bid), as an {@link Optional}, for the given item
   */
  Optional<Bid> getCurrentWinningBidForItem(Item item);

  /**
   * @param item the item
   * @return the list of all bids made for the given item
   */
  List<Bid> getAllBidsForItem(Item item);

  /**
   * @param user the user to get the list of items for
   * @return the list of all items bid on by the given user
   */
  Set<Item> getAllItemsWithBidFromUser(User user);

}

The implementation

package com.nbentayou.app.service;

import com.nbentayou.app.exception.InvalidBidException;
import com.nbentayou.app.model.Bid;
import com.nbentayou.app.model.Item;
import com.nbentayou.app.model.User;

import java.util.*;
import java.util.concurrent.ConcurrentHashMap;
import java.util.stream.Collectors;

/**
 * An implementation of the {@link BidTracker} interface with an in-memory data storage.
 *
 * Some limitations of the current implementation are:
 * - It stores the data about the auction in a ConcurrentHashMap. Other implementations could query a persistent
 * data storage (Database, flat files, ..)
 * - It assumes all the Users given as parameters are allowed to place a bid.
 * - It assumes all the Items given as parameters can be bid on.
 */
public class BidTrackerImpl implements BidTracker {

  private final Map<Item, List<Bid>> auctionBoard;

  public BidTrackerImpl() {
    auctionBoard = new ConcurrentHashMap<>();
  }

  /**
   * Getter for the auction board.
   * It returns a copy of the auction board to ensure that no external object can modify it.
   * @return a copy of the auctionBoard
   */
  public Map<Item, List<Bid>> getCurrentAuctionBoardCopy() {
    return new HashMap<>(auctionBoard);
  }


  @Override
  public void recordUserBidOnItem(Bid bid, Item item) throws InvalidBidException {
    checkForNull(bid);
    checkForNull(item);
    recordUserBidOnItemSync(bid,item);
  }

  // synchronized method ensuring that only one bid is processed at a time.
  private synchronized void recordUserBidOnItemSync(Bid bid, Item item) throws InvalidBidException {
    checkBidIsHighEnough(bid, item);
    addBidOnItem(item, bid);
  }

  private void checkBidIsHighEnough(Bid bid, Item item) throws InvalidBidException {
    Optional<Bid> currentWinningBid = getCurrentWinningBidForItem(item);
    if(currentWinningBid.isPresent() && bid.getValue() <= currentWinningBid.get().getValue()) {
      throw new InvalidBidException(String.format(
          "A bid of £%s on item %s is too low. It should be more than the current winning bid: £%s)",
          bid.getValue(),
          item,
          currentWinningBid.get().getValue()));
    }
  }

  @Override
  public Optional<Bid> getCurrentWinningBidForItem(Item item) {
    LinkedList<Bid> bids = new LinkedList<>(getAllBidsForItem(item));
    return bids.isEmpty() ? Optional.empty() : Optional.of(bids.getLast());
  }

  @Override
  public List<Bid> getAllBidsForItem(Item item) {
    checkForNull(item);
    return auctionBoard.getOrDefault(item, new ArrayList<>());
  }

  @Override
  public Set<Item> getAllItemsWithBidFromUser(User user) {
    return auctionBoard.entrySet().stream()
        .filter(entry -> containsBidFromUser(entry.getValue(), user))
        .map(Map.Entry::getKey)
        .collect(Collectors.toSet());
  }


  /* Utility methods */

  private boolean containsBidFromUser(List<Bid> bidsList, User user) {
    return bidsList.stream().anyMatch(bid -> bid.isFromUser(user));
  }

  private void addBidOnItem(Item item, Bid bid) {
    List<Bid> bidsOnItem = auctionBoard.getOrDefault(item, new ArrayList<>());
    bidsOnItem.add(bid);
    auctionBoard.put(item, bidsOnItem);
  }

  private void checkForNull(Item item) {
    if(item == null)
      throw new IllegalArgumentException("Item can't be null");
  }

  private void checkForNull(Bid bid) {
    if(bid == null)
      throw new IllegalArgumentException("Bid can't be null");
    if(bid.getUser() == null)
      throw new IllegalArgumentException("Bid's user can't be null");
  }
}

Bid POJO

package com.nbentayou.app.model;

import java.util.Objects;

/**
 * The representation of an auction bid from a user
 */
public class Bid {
  private final User user;
  private final int value;

  public Bid(User user, int value) {
    this.user = user;
    this.value = value;
  }

  public User getUser() {
    return user;
  }

  public int getValue() {
    return value;
  }

  public boolean isFromUser(User user) {
    return this.user.equals(user);
  }

  @Override
  public String toString() {
    return String.format("{ user: %s, value: %s }", user, value);
  }

  @Override
  public boolean equals(Object o) {
    boolean equals = false;
    if (o == this) {
      equals = true;
    } else if (o instanceof Bid) {
      Bid bid = (Bid) o;
      equals = Objects.equals(user, bid.user) && value == bid.value;
    }
    return equals;
  }

  @Override
  public int hashCode() {
    return Objects.hash(user, value);
  }

}

Item POJO

package com.nbentayou.app.model;

import java.util.Objects;

/**
 * The representation of an item of the auction
 */
public class Item {
  private final String id;
  private final String name;
  private final String description;

  public Item(String id, String name, String description) {
    this.id = id;
    this.name = name;
    this.description = description;
  }

  @Override
  public String toString() {
    return String.format("{ id: %s, name: %s, description: %s }", id, name, description);
  }

  @Override
  public boolean equals(Object o) {
    boolean equals = false;
    if (o == this) {
      equals = true;
    } else if (o instanceof Item) {
      Item item = (Item) o;
      equals = Objects.equals(id, item.id) && Objects.equals(name, item.name) && Objects.equals(description, item.description);
    }
    return equals;
  }

  @Override
  public int hashCode() {
    return Objects.hash(id, name, description);
  }
}

User POJO

package com.nbentayou.app.model;

import java.util.Objects;

/**
 * The representation of a user of the auction
 */
public class User {
  private final String id;
  private final String name;

  public User(String id, String name) {
    this.id = id;
    this.name = name;
  }

  @Override
  public String toString() {
    return String.format("{ id: %s, name: %s }", id, name);
  }


  @Override
  public boolean equals(Object o) {
    boolean equals = false;
    if (o == this) {
      equals = true;
    } else if (o instanceof User) {
      User user = (User) o;
      equals = Objects.equals(id, user.id) && Objects.equals(name, user.name);
    }
    return equals;
  }

  @Override
  public int hashCode() {
    return Objects.hash(id, name);
  }
}

InvalidBidException

package com.nbentayou.app.exception;

/**
 * A Generic {@link Exception} thrown when trying to make an invalid bid.
 * Could be for multiple reasons: The bid is too low, the item is not in the auction anymore, etc..
 * The reason should be explained in the message or this Exception could be subclassed by finer grain Exceptions.
 */
public class InvalidBidException extends Exception {

  public InvalidBidException(String message) {
    super(message);
  }
}

Unit tests

package com.nbentayou.app.service;

import com.nbentayou.app.exception.InvalidBidException;
import com.nbentayou.app.model.Bid;
import com.nbentayou.app.model.Item;
import com.nbentayou.app.model.User;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;

import java.util.*;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

import static org.junit.Assert.*;

public class BidTrackerImplTest {

  private final User user1 = new User("u1", "Nicolas Bentayou");
  private final User user2 = new User("u2", "Randolph Carter");
  private final User user3 = new User("u3", "Herbert West");
  private final Item item1 = new Item("i1", "item1", "Brilliant!");
  private final Item item2 = new Item("i2", "item2", "Brilliant!");
  private final Item item3 = new Item("i3", "item3", "Brilliant!");

  private BidTrackerImpl bidTracker;

  @Rule
  public final ExpectedException thrown = ExpectedException.none();

  @Before
  public void initAuctionRoom() {
    bidTracker = new BidTrackerImpl();
  }

  @Test
  public void recordUserBidOnItem_shouldAddAFirstBidToItem_whenBidIsValid() throws InvalidBidException {
    Bid bid = new Bid(user1, 10);

    bidTracker.recordUserBidOnItem(bid, item1);

    List<Bid> actualBidListOnItem1 = bidTracker.getCurrentAuctionBoardCopy().get(item1);
    List<Bid> expectedBidListOnItem1 = Collections.singletonList(bid);
    assertEquals(expectedBidListOnItem1, actualBidListOnItem1);
  }

  @Test
  public void recordUserBidOnItem_shouldAddSeveralBidsToItem_whenBidsAreValid() throws InvalidBidException {
    bidTracker.recordUserBidOnItem(new Bid(user1, 10), item1);
    bidTracker.recordUserBidOnItem(new Bid(user2, 20), item1);
    bidTracker.recordUserBidOnItem(new Bid(user1, 30), item1);

    List<Bid> actualBidsListOnItem1 = bidTracker.getCurrentAuctionBoardCopy().get(item1);
    List<Bid> expectedBidsListOnItem1 = Arrays.asList(
        new Bid(user1, 10),
        new Bid(user2, 20),
        new Bid(user1, 30));
    assertEquals(expectedBidsListOnItem1, actualBidsListOnItem1);
  }

  @Test
  public void recordUserBidOnItem_shouldThrowIllegalArgumentException_whenItemIsNull() throws InvalidBidException {
    thrown.expect(IllegalArgumentException.class);
    thrown.expectMessage("Item can't be null");

    bidTracker.recordUserBidOnItem(new Bid(user1, 10), null);
  }

  @Test
  public void recordUserBidOnItem_shouldThrowIllegalArgumentException_whenBidIsNull() throws InvalidBidException {
    thrown.expect(IllegalArgumentException.class);
    thrown.expectMessage("Bid can't be null");

    bidTracker.recordUserBidOnItem(null, item1);
  }

  @Test
  public void recordUserBidOnItem_shouldThrowIllegalArgumentException_whenUserIsNull() throws InvalidBidException {
    thrown.expect(IllegalArgumentException.class);
    thrown.expectMessage("Bid's user can't be null");

    bidTracker.recordUserBidOnItem(new Bid(null, 10), item1);
  }

  @Test
  public void recordUserBidOnItem_shouldThrowInvalidBidException_whenBidIsLowerThanCurrentlyWinningBid() throws InvalidBidException {
    thrown.expect(InvalidBidException.class);
    thrown.expectMessage("A bid of £5 on item { id: i1, name: item1, description: Brilliant! } is too low. It should be more than the current winning bid: £10)");

    bidTracker.recordUserBidOnItem(new Bid(user1, 10), item1);

    Bid lowBid = new Bid(user2, 5);
    bidTracker.recordUserBidOnItem(lowBid, item1);
  }

  @Test
  public void recordUserBidOnItem_shouldThrowInvalidBidException_whenBidIsSameAsCurrentlyWinningBid() throws InvalidBidException {
    thrown.expect(InvalidBidException.class);
    thrown.expectMessage("A bid of £10 on item { id: i1, name: item1, description: Brilliant! } is too low. It should be more than the current winning bid: £10)");

    bidTracker.recordUserBidOnItem(new Bid(user1, 10), item1);

    Bid sameBid = new Bid(user2, 10);
    bidTracker.recordUserBidOnItem(sameBid, item1);
  }

  @Test
  public void recordUserBidOnItem_shouldAddSeveralBidsToItem_whenSomeBidsAreInvalid() throws InvalidBidException {
    bidTracker.recordUserBidOnItem(new Bid(user1, 10), item1);
    bidTracker.recordUserBidOnItem(new Bid(user2, 20), item1);
    try { // invalid bid
      bidTracker.recordUserBidOnItem(new Bid(user3, 15), item1);
    } catch(InvalidBidException e) { /* Silencing the exception as it is irrelevant for this test */ }
    bidTracker.recordUserBidOnItem(new Bid(user1, 30), item1);

    List<Bid> bidsListOnItem1 = bidTracker.getCurrentAuctionBoardCopy().get(item1);

    List<Bid> expectedBidsOnItem1 = Arrays.asList(
        new Bid(user1, 10),
        new Bid(user2, 20),
        new Bid(user1, 30));
    assertEquals(expectedBidsOnItem1, bidsListOnItem1);
  }

  @Test
  public void recordUserBidOnItem_shouldOnlyRecordValidBids_inAMultithreadedEnvironment() {
    AtomicInteger invalidBidsCount = new AtomicInteger(0);

    // Make 10000 bids on 4 different threads.
    int totalNbBids = 10000;
    ExecutorService executor = Executors.newFixedThreadPool(4);
    IntStream.range(0, totalNbBids).forEach(
        i -> executor.submit(
            () -> {
              try {
                bidTracker.recordUserBidOnItem(new Bid(user1, i), item1);
              } catch(InvalidBidException e) {
                invalidBidsCount.incrementAndGet();
              }
            }
        )
    );
    shutDownExecutor(executor);

    List<Bid> actualBidsMade = bidTracker.getCurrentAuctionBoardCopy().get(item1);

    // asserting that all bids were processed
    assertEquals(totalNbBids, actualBidsMade.size() + invalidBidsCount.get());
    // asserting that the accepted bids for the item are all ordered by increasing value
    assertEquals(actualBidsMade, sortBidListByValue(actualBidsMade));
    // asserting that the last bid is for 9999
    Bid lastBidMade = actualBidsMade.get(actualBidsMade.size() - 1);
    assertEquals(totalNbBids - 1, lastBidMade.getValue());
  }

  private void shutDownExecutor(ExecutorService executor) {
    try {
      executor.awaitTermination(2, TimeUnit.SECONDS);
    } catch (InterruptedException e) {
      System.err.println("tasks interrupted");
    } finally {
      executor.shutdownNow();
    }
  }

  private List<Bid> sortBidListByValue(List<Bid> bidList) {
    return bidList.stream()
        .sorted(Comparator.comparing(Bid::getValue))
        .collect(Collectors.toList());
  }

  @Test
  public void getCurrentWinningBidForItem_shouldThrowIllegalArgumentException_whenItemIsNull() {
    thrown.expect(IllegalArgumentException.class);
    thrown.expectMessage("Item can't be null");

    bidTracker.getCurrentWinningBidForItem(null);
  }

  @Test
  public void getCurrentWinningBidForItem_shouldReturnEmptyOptional_whenItemHasNoBid() {
    Optional<Bid> bid = bidTracker.getCurrentWinningBidForItem(item1);
    assertEquals(Optional.empty(), bid);
  }

  @Test
  public void getCurrentWinningBidForItem_shouldReturnOptionalWithAValue_whenItemHasBids() throws InvalidBidException {
    bidTracker.recordUserBidOnItem(new Bid(user1, 10), item1);
    bidTracker.recordUserBidOnItem(new Bid(user2, 20), item1);

    Optional<Bid> bid = bidTracker.getCurrentWinningBidForItem(item1);

    assertTrue(bid.isPresent());
    assertEquals(bid.get(), new Bid(user2, 20));
  }

  @Test
  public void getAllBidsForItem_shouldThrowIllegalArgumentException_whenItemIsNull() {
    thrown.expect(IllegalArgumentException.class);
    thrown.expectMessage("Item can't be null");

    bidTracker.getAllBidsForItem(null);
  }

  @Test
  public void getAllBidsForItem_shouldReturnEmptyList_whenItemHasNoBid() {
    List<Bid> bids = bidTracker.getAllBidsForItem(item1);
    assertTrue(bids.isEmpty());
  }

  @Test
  public void getAllBidsForItem_shouldReturnTheCorrectListOfBids_whenItemHasBids() throws InvalidBidException {
    bidTracker.recordUserBidOnItem(new Bid(user1, 10), item1);
    bidTracker.recordUserBidOnItem(new Bid(user2, 20), item1);

    List<Bid> actualBids = bidTracker.getAllBidsForItem(item1);

    List<Bid> expectedBids = Arrays.asList(
        new Bid(user1, 10),
        new Bid(user2, 20));
    assertEquals(expectedBids, actualBids);
  }



  @Test
  public void getAllItemsWithBidFromUser_shouldReturnEmptySet_whenUserIsNull() {
    Set<Item> items = bidTracker.getAllItemsWithBidFromUser(null);
    assertTrue(items.isEmpty());
  }

  @Test
  public void getAllItemsWithBidFromUser_shouldReturnEmptySet_whenUserHasNoBid() {
    Set<Item> items = bidTracker.getAllItemsWithBidFromUser(user1);
    assertTrue(items.isEmpty());
  }

  @Test
  public void getAllItemsWithBidFromUser_shouldReturnCorrectItemSet_whenUserHasBids() throws InvalidBidException {
    bidTracker.recordUserBidOnItem(new Bid(user1, 10), item1); // bid on item1
    bidTracker.recordUserBidOnItem(new Bid(user2, 20), item1);
    bidTracker.recordUserBidOnItem(new Bid(user1, 30), item1); // second bid on item1
    bidTracker.recordUserBidOnItem(new Bid(user2, 10), item2);
    bidTracker.recordUserBidOnItem(new Bid(user3, 20), item2);
    bidTracker.recordUserBidOnItem(new Bid(user3, 10), item3);
    bidTracker.recordUserBidOnItem(new Bid(user1, 20), item3); // bid on item3

    Set<Item> itemList = bidTracker.getAllItemsWithBidFromUser(user1);

    Set<Item> expectedItemList = new HashSet<>(Arrays.asList(item1, item3));
    assertEquals(expectedItemList, itemList);
  }
}

Answer 1

  @Override
  public void recordUserBidOnItem(Bid bid, Item item) throws InvalidBidException {
    checkForNull(bid);
    checkForNull(item);
    recordUserBidOnItemSync(bid,item);
  }

Did you use your auto-formatter? There's a missing space between the comma and item here.

  private void addBidOnItem(Item item, Bid bid) {
    List<Bid> bidsOnItem = auctionBoard.getOrDefault(item, new ArrayList<>());
    bidsOnItem.add(bid);
    auctionBoard.put(item, bidsOnItem);
  }

This could be rewritten to use computeIfAbsent:

auctionBoard.computeIfAbsent(item, ignored -> new ArrayList<>()).add(bid);

Only slightly shorter, but it does help.

  @Override
  public Optional<Bid> getCurrentWinningBidForItem(Item item) {
    LinkedList<Bid> bids = new LinkedList<>(getAllBidsForItem(item));
    return bids.isEmpty() ? Optional.empty() : Optional.of(bids.getLast());
  }

Here there's no real reason to wrap the bids in a linked list.

  @Override
  public Optional<Bid> getCurrentWinningBidForItem(Item item) {
    List<Bid> bids = getAllBidsForItem(item);
    return bids.isEmpty() ? Optional.empty() : Optional.of(bids.get(bids.size() - 1));
  }

You could maybe turn that into a helper function, getLastOfList.

---

  private void checkForNull(Item item) {
    if(item == null)
      throw new IllegalArgumentException("Item can't be null");
  }

  private void checkForNull(Bid bid) {
    if(bid == null)
      throw new IllegalArgumentException("Bid can't be null");
    if(bid.getUser() == null)
      throw new IllegalArgumentException("Bid's user can't be null");
  }

To me, these two methods hurt the most.

To start with, these methods shouldn't have been implemented by you. Use Objects.requireNotNull:

  @Override
  public List<Bid> getAllBidsForItem(Item item) {
    checkForNull(item);
    return auctionBoard.getOrDefault(item, new ArrayList<>());
  }

would become

  @Override
  public List<Bid> getAllBidsForItem(Item item) {
    Objects.requireNotNull(item);
    return auctionBoard.getOrDefault(item, new ArrayList<>());
  }

or

  @Override
  public List<Bid> getAllBidsForItem(Item item) {
    return auctionBoard.getOrDefault(Objects.requireNotNull(item), new ArrayList<>());
  }

possibly with custom message.

Second, but that's just personal preference, although that could make you a bad fit,

  private void checkForNull(Bid bid) {
    if(bid == null)
      throw new IllegalArgumentException("Bid can't be null");
    if(bid.getUser() == null)
      throw new IllegalArgumentException("Bid's user can't be null");
  }

this right here, with the if statements without braces? That's bad form. At least, to me it is.

And then of course there's the overloading for checkForNull where it takes either Item or Bid. Even although the two are completely unrelated. That's weird.

I'd personally have named them validateItem and validateBid. You could have put various business logic in there. Maybe you can't have negative bids? I dunno. It's a bit of a shame they gave you no feedback, if I was interviewing you I'd be interested in pressing you on these points.

---

 * The reason should be explained in the message or this Exception could be subclassed by finer grain Exceptions.

You've got a typo there, should be "finer grained Exceptions." ... it's a minor issue, though.

---

/**
 * The interface for a Bid Tracker.
 * This interface exposes methods allowing {@link User}s to post {@link Bid}s on {@link Item}s
 * and query the current state of the auction.
 */
public interface BidTracker {

  /**
   * Records a bid for a given item.
   * @param bid the bid to record
   * @param item the item to bid on
   * @throws InvalidBidException when the bid is invalid
   */
  void recordUserBidOnItem(Bid bid, Item item) throws InvalidBidException;

  /**
   * @param item the item
   * @return the current winning bid (last valid bid), as an {@link Optional}, for the given item
   */
  Optional<Bid> getCurrentWinningBidForItem(Item item);

  /**
   * @param item the item
   * @return the list of all bids made for the given item
   */
  List<Bid> getAllBidsForItem(Item item);

  /**
   * @param user the user to get the list of items for
   * @return the list of all items bid on by the given user
   */
  Set<Item> getAllItemsWithBidFromUser(User user);

}

Ooh, documentation.

How much of it is useful?

/**
 * The interface for a Bid Tracker.
 * This interface exposes methods allowing {@link User}s to post {@link Bid}s on {@link Item}s
 * and query the current state of the auction.
 */
public interface BidTracker {

That first sentence is useless. public interface BidTracker - "The interface for a Bid Tracker." Autocomplete in IDEs tends to give you the first sentence and you've just wasted it. The second sentence is better and actually explains what a BidTracker ... Service... actually does.

  /**
   * Records a bid for a given item.
   * @param bid the bid to record
   * @param item the item to bid on
   * @throws InvalidBidException when the bid is invalid
   */
  void recordUserBidOnItem(Bid bid, Item item) throws InvalidBidException;

The method recordUserBidOnItem Records a bid for a given item. Duhhh. It's redundant.

The bid is the bid to record (you know, "recordUserBid"), then item is the item to bid on ("OnItem"). This is all just duplication. Also, it throws InvalidBidException when the bid is invalid.

... All of that information you have put in the documentation was literally IN the method signature!

Look at it!

void recordUserBidOnItem(Bid bid, Item item) throws InvalidBidException;

It records a user's bid on an item. You have to provide the bid and the item. Also, it might throw InvalidBidException (presumably because the bid is invalid). And it returns void, so I guess the recording is "saving" or something.

There was no new information in the javadoc, as a result the javadoc is just meaningless noise. I don't blame you, from the problem brief you have no real business rules to describe here, but still. Don't put javadoc on methods just to tick a box.

  /**
   * @param item the item
   * @return the current winning bid (last valid bid), as an {@link Optional}, for the given item
   */
  Optional<Bid> getCurrentWinningBidForItem(Item item);

And here you do it again! Although I must admit - there's one nugget of information here. A "currentWinningBid" is the "last valid bid" for a given item. But the rest is noise. It can go.

  /**
   * @param item the item
   * @return the list of all bids made for the given item
   */
  List<Bid> getAllBidsForItem(Item item);

Useless.

  /**
   * @param user the user to get the list of items for
   * @return the list of all items bid on by the given user
   */
  Set<Item> getAllItemsWithBidFromUser(User user);

That's no List, that's a Set! So why are you calling it a list?

Also, the rest of the documentation here is useless too.

This entire interface would have been better off without method level javadoc.

---

The thing is, as a junior backend developer, yeah, you'd be pretty workable. Depending on how badly I needed people, I'd hire you, if the personality was a match. I don't know what kind of job you applied to. For a higher experienced job, I'd expect better.

On the other hand, I don't know how much time you spent on this. An hour? Three hours? A day? If it was a day I'd think it to be a bit much but if you did this in an hour then I'd say it'd just need a bit of polishing. I mean, I did kinda tear into it, but most of my comments were minor. I'm not really spotting any bugs either.

I think the most important thing you should take away from this review is that you should have sensible javadoc. And, where possible, provide an explanation from your point of view when you're presenting your work. Why did you make certain choices? For instance, why does a BidTracker accept random items to be bidded on, items it has never seen before? Maybe you take it as a given that you can just let a BidTracker work like that for demonstration purposes. Maybe they see it as a law of Demeter violation.

Answer 2

I agree with Pimgd's answer. The code itself looks fine and there really isn't any good reason why they wouldn't at least take you in for an actual interview.

The only feeling I get from your solution is that it may be over-designed for their tastes.

What if they were expecting the user and item to just be simple Strings and the "bid" just a float? The interface could then look something like:

public interface Auction {

    public void placeBid(float bid, String user, String item) throws InvalidBidException;

    public float getCurrentWinningBidForItem(String item);

    public List<float> getAllBidsForItem(String item);

    public List<String> getAllItemsWithBidFromUser(String user);

}

For the implementation you could go in a few directions. My initial thought would be to create a POJO class Bid like this:

class Bid {
    private final String name;
    private final String item;
    private final float price;

    public Bid(String name, String item, float price) {
        this.name = name;
        this.item = item;
        this.price = price;
    }

    public String getName() {
        return name;
    }

    public String getItem() {
        return item;
    }

    public float getPrice() {
        return price;
    }
}

As for the actual implementation of the Auction we could then use a List<Bid> to store all the given bids and use a stream to filter out the result. For example:

private final List<Bid> placedBids = new CopyOnWriteArrayList<>();

public float getCurrentWinningBid(String item){
    return placedBids.stream()
            .filter(bid->item.equals(bid.getName()))
            .map(bid -> bid.getPrice())
            .max(Float::compare)
            .orElse(0f);
}

Which results in a lot less code to look at in the submissions they got. If they got a submission like this and were content about that candidate they might have gotten lazy and refused all others (without being able to give a proper reason why as you noticed).

---

Don't let this failed job attempt discourage you though. Like Pimgd already mentioned the things that you can still improve upon are easy to teach you once you get started in a company so those shouldn't really be a reason not to give you a proper interview. Just search for a company that appreciates your thorough approach more.

Answer 3

All the calls to the bidding system were synchronous (i.e. in the test cases). Nothing was built to demonstrate the asynchronous nature of an auction site.

Overall, what was developed was the underlying backend structure of the bidding system - which is good and needed, but doesn't actually represent what a bidding site is.

Answer 4

Don't get disheartened by the rejection, it most likely isn't about you. Your solution works but as someone posted above said might be little over-engineered. My simple solution would be like below:

Map<String, List<Bids>> userBids = new HashMap<String,List<Bids>>();

Given a user id - get all bids user has bid for - \$O(1)\$

Map<String, PriorityQueue<Bid>> itemBids = new HashMap<String, PriorityQueue<Bid>>();

Given an item id - get all Bids for the item - \$O(1)\$- return everything in the Queue

• Also return Bid with highest value (winning bid) for the item in \$O(1)\$ time

  itemBids.get('itemId').peek() // winning bid
  

• Record Bid - add to userBids and itemBids maps

  class Bid implements Comparator<Integer> {
      int amt;
  
      String user;
  
      String item;
  
      // constructor
  
      public int compare(Bid b1, Bid b2) {
          return b2.amt - b1.amt; // decending order
      }
  }
  

Answer 5

Most people are talking about code style, optimization, and documentation, but to me those things aren't typically that vital. I know people will get on my case about that, but here's my reasoning.

Although those things are VERY important, none of your code is so poorly optimized that it makes a serious impact on performance. Other things would definitely be a larger bottleneck in your code.

Your code style and documentation while not the most helpful or perfect weren't really that bad. If I was a co-worker looking at the code, I definitely wouldn't have been that upset with it. Just make sure you run your auto-formatter more.

However, your naming and general code design were not the greatest. As a co-worker, I definitely would have gotten frustrated at times.

Naming

These are grouped by what exactly you are naming.

Method Names

Interface Methods

void recordUserBidOnItem(Bid bid, Item item) throws InvalidBidException;

Optional<Bid> getCurrentWinningBidForItem(Item item);

List<Bid> getAllBidsForItem(Item item);

Set<Item> getAllItemsWithBidFromUser(User user);

Each of these method names are incredibly verbose for no real reason. Here's how I would recommend naming them:

void recordBidForItem(Bid bid, Item item) throws InvalidBidException;

Can someone other than a User submit a Bid? If not, then having the word "user" as part of the method name is redundant and confusing.

Optional<Bid> getCurrentWinningBidForItem(Item item);

Why specify both "current" and "winning"? If someone told you "I have the current bid for an auction" or "I have the winning bid for an auction", you would understand those as the same thing.

List<Bid> getBidsForItem(Item item);

Set<Item> getItemsWithBidFromUser(User user);

Typically you should try to avoid the word "all" unless it really is all of something. If you have the word "for" or "of" after "all", typically your naming is not right. The main reason for that is if you later decided to add a filter option and wanted to overload that method, it would no longer be an accurate method name.

Other Methods

private void checkBidIsHighEnough(Bid bid, Item item) throws InvalidBidException

When I see a method starting with check, I assume it will return a boolean with whether or not the condition is met. For methods that throw an exception, I typically see things like validate, ensure, or require. For this, try ensureBidHigher.

Variable names

There are a couple things that are problematic.

LinkedList<Bid> bids = new LinkedList<>(getAllBidsForItem(item));

When you name lists, try to avoid just adding the letter 's' to the end. Appending "List" at the end makes it easier to notice at a quick glance what a variable is. This one is more of a personal preference, but I know that most people at my company agree. I notice you actually do this in the containsBidFromUser method. You can ignore my advice on this one or listen to it, but above all, and I'm sure everyone else here agrees, BE CONSISTENT.

Optional<Bid> currentWinningBid = getCurrentWinningBidForItem(item);

Just name it currentBid or winningBid. See my notes above for reasoning.

@Override
public Set<Item> getAllItemsWithBidFromUser(User user) {
  return auctionBoard.entrySet().stream()
      .filter(entry -> containsBidFromUser(entry.getValue(), user))
      .map(Map.Entry::getKey)
      .collect(Collectors.toSet());
}

Don't name lambda parameters after their data type if that type is not descriptive. If you aren't the one who wrote the code, would you know what the type of entry is without scrolling up to auctionBoard? My advice is to name it something like bid.

Class Structure

Here's my biggest gripe with your code. One of the primary purposes for classes is separation of concerns. A class should only contain functionality that is specific to that class.

Besides that, you overuse checked exceptions. Here's an excerpt from a StackOverflow post: checked exceptions should only be used when predictable, but preventable, errors that are reasonable to recover from.

Class Member Ordering

This is really a pet peeve of mine. Class members should always have a logical order. What order you use is completely up to the maintainer, BUT you definitely need to have an order. Public and private methods/fields should NOT be mixed together arbitrarily. Here is my personal preference for class members. Feel free to use it or not, but definitely fix the random jumble:

• private static final fields
• protected static final fields
• public static final fields
• private static fields
• protected static fields
• public static fields
• static initializer block
• private final fields
• protected final fields
• public final fields
• private fields
• protected fields
• public fields
• initializer block
• constructors
• public methods
• private methods
• public static methods
• private static methods
• enums
• interfaces
• static classes
• classes