-1
\$\begingroup\$

I made a very basic social network Java program. I was wondering if my design could be improved. I have 2 classes (+ 1 driver) : Person and Graph. I would like now to add a new method hasAccount(Person person) in the Graph class to check if a Person is on the graph. However, so far I don't have any data structure that stores every Person object added on the Graph. What would be the most efficient way to do it?

    public class Driver {
      public static void main(String[] args) {
        Graph facebook = new Graph();
        Person sara = new Person("sara");
        Person robert = new Person("robert");
        Person alex = new Person("alex");
        facebook.newFriendship(sara, robert);
        facebook.newFriendship(alex, sara);
        facebook.printFriends(sara);
        facebook.removeFriendship(sara, alex);
        facebook.printFriends(sara);
      }
    }

/*Class to represent friendships*/

    public class Graph {

        public void newFriendship(Person from, Person to) throws IllegalArgumentException{
            if(from == null || to == null){
                throw new IllegalArgumentException("One of the account does not exits");
            }
            from.addFriend(to);
            to.addFriend(from);
        }

        public void removeFriendship(Person from, Person to) throws IllegalArgumentException{
            if(from == null || to == null){
                throw new IllegalArgumentException("One of the account does not exits");}
            from.removeFriend(to);
            to.removeFriend(from);
        }

        public void printFriends(Person user){
            System.out.println(user.getFriendList());
        }
    }

/*Class to represent a User*/
    public class Person {
      private String name;
      private List<Person> friends;

      Person(String name){
        this.name = name;
        friends = new LinkedList();
      }

      public void addFriend(Person newFriend){
        this.friends.add(newFriend);
      }

      public List getFriendList(){
        List<String> friendList = new ArrayList<>();
        for(Person friend : friends){
          friendList.add(friend.name);
        }
        return friendList;
      }

      public void removeFriend(Person toremove){
        friends.remove(toremove);
      }
    }
\$\endgroup\$
  • 5
    \$\begingroup\$ We can review your current code if it works, with your feature in mind. But this is code review, we don't do feature-requests. \$\endgroup\$ – Mast Jan 21 at 21:08
  • 2
    \$\begingroup\$ (Graph does not represent friendships - and what business would it have to? The "driver" seems to be using Graph as a go-between. There is no way to add Person objects on the Graph. Under what circumstances do you think storage of person related data anywhere else (than with the person) tolerable?) \$\endgroup\$ – greybeard Jan 21 at 22:36
3
\$\begingroup\$

Graph is a strange name for something designed to manipulate friendships. As indicated by @greybeard in the comments, it doesn't really represent anything at the moment.

print

I hate seeing methods like printFriends on classes. They very rarely belong. If you want to have the class provide a method/toString implementation that can be used by it's controller to write to the console / log / outputStream then that's one thing. However, hardcoding System.out.println into classes really does limit their usefulness.

control + error checking

Is it valid to call addFriendShip twice for the same friends? To Remove it twice? What are you expecting in this scenario:

facebook.newFriendship(sara, robert);
facebook.newFriendship(robert, sara);

Sara ends up with two friends, both of whom are Robert. This seems wrong.

Friendship is a two way street

This is nitpicky, but newFriendShip tells both supplied Persons that they have a new friend. Having the parameter names from and to suggests a one directional, rather than two way relationship.

List

getFriendList returns a list of Strings, unless there's a good reason not to, I'd set the return type appropriately, rather than just returning a basic List. You should also consider using the Stream libraries, they can make your code more concise:

public List<String> getFriendList(){
    return friends.stream().map(f->f.name).collect(Collectors.toList());
}

Graph

There are various structures that Graph could use to keep track of what Persons it knows about. However, there isn't really enough information at the moment to suggest the correct method. Some things to consider include, does Graph need to support adding and removing? If a Person is added, does the graph need to keep track of all that Persons friends as well? How about their friends and so on? What operations are you going to want Graph to support that will use the Persons it knows about?

\$\endgroup\$

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