interview question, friends, movies and likes

Question

I was recently asked to implement an interface for a job interview. the class has methods to add customers and movies, the customers can watch or like movies and add friends. there are methods to get recommendations for users. All public methods in SocialMoviesImpl were defined by the interface, so I could not change them

the company decided not to continue with the hiring process, I would like some feedback in what I implemented. I used hashmaps to store the information since it is fast to access

import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;


public class SocialMoviesImpl implements SocialMovies {

Map<Integer,String> movies;
Map<Integer,Customer> customers;

public SocialMoviesImpl(){
    this.movies= new HashMap<Integer,String>(); 
    this.customers= new HashMap<Integer,Customer>();


}

/**
 * add a movie, runtime complexitiy is O(1) //hashmap
 * @param movieId the id of the movie
 * 
 */
public void addMovie(int movieId, String title) {
    this.movies.put(movieId,title);

}
/**
 * gets a movie, O(1)
 */
public String lookupMovie(int movieId) {
    return this.movies.get(movieId);
}

/**
 * adds a new customer, O(1)
 */
public void addCustomer(int customerId, String name) {
    this.customers.put(customerId, new Customer(customerId, name));
}

/**
 * obtains the customer, O(1)
 */
public String lookupCustomer(int customerId) {
    Customer cust= this.customers.get(customerId);
    return cust!=null?cust.getName():null;
}

/**
 * O(4), 3 searches and 1 add
 */
public void addLikedMovie(int customerId, int movieId) {
    if(this.customers.containsKey(customerId) && this.movies.containsKey(movieId)){
        this.customers.get(customerId).addLike(movieId);
    }else{
        throw new IllegalArgumentException("movie or client does not exists");
    }

}
/**
 * O(4)
 */
public void addWatchedMovie(int customerId, int movieId) {
    if(this.customers.containsKey(customerId) && this.movies.containsKey(movieId)){
        this.customers.get(customerId).addWatch(movieId);
    }else{
        throw new IllegalArgumentException("movie or client does not exists");
    }

}

/**
 * O(6)
 */
public void addFriend(int customerId1, int customerId2) {
    if(this.customers.containsKey(customerId1) && this.customers.containsKey(customerId2)){
        //we add a key to the customers friend entry
        this.customers.get(customerId1).addFriend(customerId2);
        this.customers.get(customerId2).addFriend(customerId1);
    }else{
        throw new IllegalArgumentException("the customerId or movieId provided does not exists");
    }
}

/**
 * returns any movie that has been liked by a friend.
 * 
 * 
 */
public Collection<Integer> getRecommendationsFromFriends(int customerId) {
    return getFriendRecommendations(customerId, 1);
}

/**
 * returns every movie that has been liked by a defined number of friends.
 * O(n^2)
 * 
 */
public Collection<Integer> getFriendRecommendations(int customerId,
        int minimumCommonFriends) {
    if(minimumCommonFriends<1){
        //negative doesnt make sense and zero will mean every movie
        throw new IllegalArgumentException("please provide a positive non-zero number of minimum common friends");
    }

    //iterate through all friends and their movies
    Map<Integer,Integer> recommends= new HashMap<Integer,Integer>();
    Collection<Integer> friends=this.customers.get(customerId).getFriends();
    for(Integer friendId:friends){
        Customer friend= this.customers.get(friendId);
        for(Integer movie: friend.getLikes()){
            Integer ammount=recommends.get(movie);
            ammount=ammount!=null?ammount+1:1; //increment or set to 1 if first time
            recommends.put(movie,ammount); //we dont care about who is recommending it
        }

    }

    //now, filter only those recommended by at least the treshold
    Set<Integer> keys=recommends.keySet(); 
    Set<Integer> toRemove= new HashSet<Integer>(); //we cannot remove during iteration
    for(Integer movie: keys){
        Integer commonFriends= recommends.get(movie); //ammount of friends who liked it
        if(commonFriends<minimumCommonFriends){
            toRemove.add(movie);
        }
    }
    keys.removeAll(toRemove);
    return recommends.keySet();

}

/**
 * class that holds information about the customer (liked, watched, etc).
 * @author santiago
 *
 */
class Customer{

    private int customerId;
    private String name;

    private Set<Integer> likes;
    private Set<Integer> watched;
    private Set<Integer> friends;

    public Customer(int id, String name){
        this.customerId=id;
        this.name=name;
        this.likes= new HashSet<Integer>();
        this.watched= new HashSet<Integer>();
        this.friends= new HashSet<Integer>();
    }

    public String getName(){
        return this.name;
    }

    public Set<Integer> getLikes() {
        return likes;
    }


    public Set<Integer> getWatched() {
        return watched;
    }


    public Set<Integer> getFriends() {
        return friends;
    }


    public void addFriend(int friend){
        this.friends.add(friend);
    }

    public void addWatch(int movie){
        this.watched.add(movie);

    }

    public void addLike(int movie){
        this.likes.add(movie);
    }

}

}

this is the provided interface

import java.util.Collection;

// Maintains a network of movies and customers
// All methods should return an empty collection, -1, or null on failure, as appropriate <---*** I just noted this and feel very very bad.
public interface SocialMovies {

// Defines a movie ID to title mapping in the system
void addMovie(int movieId, String title);

// Returns the title of the given movie
String lookupMovie(int movieId);

// Defines a customer ID to name mapping in the system
void addCustomer(int customerId, String name);

// Returns the name of the given customer
String lookupCustomer(int customerId);


// Record that a movie was "liked" by the given customer
void addLikedMovie(int customerId, int movieId);

// Record that a movies has been watched by the given customer
void addWatchedMovie(int customerId, int movieId);

// Associate two customers as being friends
void addFriend(int customerId1, int customerId2);


// Returns the IDs of movies that:
// - Have not been watched by the given customer
// - Have been "liked" by at least one of the given customer's friends
Collection<Integer> getRecommendationsFromFriends(int customerId);

// Returns the IDs of customers that have at least <minimumCommonFriends> in common
// with the given customer
Collection<Integer> getFriendRecommendations(int customerId, int minimumCommonFriends);

}

Answer 1

Your getRecommendationsFromFriends and getFriendRecommendations implementations are incorrect as the documentation shows:

• getRecommendationsFromFriends: return movie id's
• getFriendRecommendations: return customer id's

This is the biggest issue with your implementation.

getRecommendationsFromFriends is fairly straight-forward:

public Collection<Integer> getRecommendationsFromFriends(int customerId) {
    /* 1. Validate Customer
     * 2. Collect the id's for movies his friends like
     * 3. Remove the customer's watched movies from the friend likes
     * 4. Return the collection (a set is good here, maybe even a TreeSet for sorting
     */
}

2 and 3 could be combined (a helper method in Customer could be handy)

GetFriendRecommendations is slightly more complicated, but not unreasonable for an interview. (keep in mind there could be many many customers!). Remember to always read (and re-read!) the documentation to make sure you have a firm grasp on what the interface is supposed to provide.

Answer 2

My thoughts in no particular order:

• You can remove most of the "this." in your code. That looks terrible to me, sorry.

• addMovie and lookupMovie should probably be part of a separate class like MovieDatabase

• Customer.addWatch() has a misleading name, not consistent with SocialMovies.addWatchedMovie

• addCustomer does not verify that the id is already used

• lookupMovie has no exception handling

• lookupCustomer has no exception handling

• lookupCustomer returns the name, would it not be better to return the customer object?

• addLikedMovie & addWatchedMovie: I would separate the 2 exceptions, so that it is clear which id does not exist.

• Should a method that throws IllegalArgumentException have that in the method declaration?

• addLikedMovie & addWatchedMovie: Do not follow the DRY principle

• should addLikeMovie also call addWatchedMovie ?

• The error message in addFriend is wrong, copy pasting is not your friend

• Ammount is not spelled correctly

• Why is one method called getRecommendationsFromFriends and the other one getFriendRecommendations? Confusing.

• Are you sure that getFriendRecommendations is O(n^2)?

• //we cannot remove during iteration <- Are you sure, how about calling remove() on an iterator?

Answer 3

Don't feel bad about your code, I've seen worse, you'll be fine, asking this here is a huge step forward.

Here are the things that were most critical in my view

1. Division of Responsibility

I would have a class for each model, and each model can do CRUD (create read update delete) on it, e.g. a Movie class and a Person class ( and Customer extends person)

I think not having a Movie class was one of the major issues.

2. Better references

Instead of this

  private Set<Integer> likes;
  private Set<Integer> watched;
  private Set<Integer> friends;

I would do this

  private Set<Movie> likes;
  private Set<Movie> watched;
  private Set<Customer> friends;

3. Other small things

you say:

we cannot remove during iteration

Well it's true in this form of iteration, but in this case you can simply use an Iterator, which has a remove method that allows to remove items while iterating.