Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

}

share|improve this question
    
I like this question, but unfortunately, I can't help answer it. I think I'd do it very differently, but I can't tell how much of that is Java idiom (vs the C# I use) and how much would actually be improvements. Do you really need to indicate the complexity of each function, though? That seems overkill to me. –  Bobson Feb 12 '13 at 19:17
    
yes, they ask to provide complexity for every method –  santiagozky Feb 12 '13 at 20:33
    
Just to be sure, you know that both getRecommendationsFromFriends and getFriendRecommendations are wrongly implemented? –  konijn Feb 12 '13 at 21:12
    
@santiagozky - My understanding of complexity was that there is no such thing as O(2), O(4), etc. See wikipedia - O(1) is just an indication of constant time. At best, it would be 4*O(1) - four constant-time operations. –  Bobson Feb 12 '13 at 21:32
    
Just as an hint regarding the complexity of HashMap operations: What is the time complexity of HashMap.containsKey() in java? –  mnhg Feb 13 '13 at 8:43

3 Answers 3

up vote 4 down vote accepted

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.

share|improve this answer
    
thanks for the feedback. I was a bit on a hurry and I didnt take time enough to understand what I was ask to do. I hope I learnt a lesson, it could have been a great job opportunity –  santiagozky Feb 13 '13 at 8:47
1  
The good thing is you are looking for feedback and trying to improve. Keep on coding! –  E-Man Feb 13 '13 at 13:06

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?

share|improve this answer
    
thanks for the feedback. maybe I should have make clear that all the methods were defined in an interface provided by them, so I had no control over them. –  santiagozky Feb 12 '13 at 20:32
    
I was wondering about that, do you still have the interface, it might make this easier to review. –  konijn Feb 12 '13 at 20:33
    
yes, I just added, along with a note in the comments about some very clear instructions I shamefully ignored :( –  santiagozky Feb 12 '13 at 20:38
    
@santiagozky - Well, that explains getRecommendationsFromFriends vs getFriendRecommendations... I disapprove of their interface. It should be getMovieRecommendationsFromFriends, since otherwise it could be returning friends which your friends recommended to you. –  Bobson Feb 12 '13 at 21:35

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.

share|improve this answer
    
Indeed. I think the main point of improvement is the fact that there is almost no domain model here. –  Joeri Hendrickx Feb 13 '13 at 13:14

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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