4
\$\begingroup\$

Here is the link to git repo of my application I am currently implementing just to have better understanding of OOP as well as learning Concurrency (Atleast a try :D)

What is the whole simulation intends to do?

This application is intended to provide classes or interfaces to create

-> a Room in which persons can enter, objects to be interacted are placed.

-> Person who can enter a room and interact with any random object person wants to.

-> Interactable objects which can be interacted. A single object can be interacted by multiple persons at one time and some can only be interacted by one person at a time.

The code I would like to code review is allowing a person to enter in the room. So, this is what my code personEnters currently is doing.

It checks, if room is not already full, and if a person wants to SIT or STAND, if person wants to SIT, it checks for Interactable, which has SIT interaction and get the one which is free and no one using. If it gets, person SITs otherwise, person STANDs.

Until the very end, I was not returning the Person from personEnters until I came to work with multi threading on it. So, I had to return a Person object since Person is Runnable and I can start a thread on it.

simulation

More specifically this function:

public synchronized Person personEnters (final Person person)
        throws RoomFullException, InteractionNotPossibleException, PersonNotInRoomException, NoCurrentInteraction {
        Objects.requireNonNull(person);
        if (persons.size() >= capacity) {
            throw new RoomFullException("Room is full.");
        }
        if (person.getCurrentPosition() == Person.Position.SIT && getPersonsByPosition(Person.Position.SIT).size() >=
                                                                  sitCapacity) {
            System.out.println("No more SIT objects. So, " + person + " is going to stand.");
            person.changePosition(Person.Position.STAND);
        }
        System.out.println(person + " enters room at: " + new Date().toString());
        this.persons.add(person);
        person.setRoom(this);
        if (person.getCurrentPosition() == Person.Position.SIT) {
            final Interactable chair = getRandomFreeObjectByInteraction(EnumSet.of(Interactions.SIT));
            if (chair == null) {
                System.out.println("Something is wrong.");
            } else {
                person.tryOccupying(chair);
            }
        }
        return person;
    }

and this is the test case I have implemented to support that multi-threading is working fine.

@Test
    public void multiplePersonsEntersRoomAndExitsAsSoonAsTimeGetsOver ()
        throws SitFullException, PersonNotInRoomException, InteractionNotPossibleException, NoCurrentInteraction,
               RoomFullException, InterruptedException {
        final Room  room  = new Room("Room1", 2, 1);
        final Chair chair = new Chair("Chair");
        room.putObject(chair);
        final Person person1 = new Person("Adeel");
        final Person person2 = new Person("Jamil");
        Thread       t1      = new Thread(room.personEnters(person1));
        Thread.sleep(1000);
        Thread t2 = new Thread(room.personEnters(person2));
        t1.start();
        t2.start();
        while (t1.isAlive() || t2.isAlive()) {}
        assertThat(room.hasPerson(person1)).isFalse();
    }

And here is the Person class:

package com.ssc.simulation.entity;

import com.ssc.simulation.action.Interactable;
import com.ssc.simulation.action.Interactions;
import com.ssc.simulation.exception.InteractionNotPossibleException;
import com.ssc.simulation.exception.NoCurrentInteraction;
import com.ssc.simulation.exception.PersonNotInRoomException;
import com.ssc.simulation.exception.RoomEmptyException;

import java.util.Objects;

/**
 * author:  shakeel
 * date:    April 15, 2016.
 */
public class Person implements Runnable {
    /**
     * Person's name
     */
    private String       name;
    /**
     * {@link Room} the person is currently in
     */
    private Room         room;
    /**
     * Current Person's {@link Position}
     */
    private Position     position;
    /**
     * Current {@link Interactable}, person is interacting with
     */
    private Interactable currentObject;

    public Person (final String name) {
        this(name, null);
    }

    /**
     * Every person is {@code SIT} greedy by default.
     *
     * @param name of this person
     * @param room this person is currently in or {@code null}
     */
    public Person (final String name, final Room room) {
        this.name = name;
        this.room = room;
        position = Position.SIT;
    }

    /**
     * Returns current {@code Interactable}, this person is interacting with.
     *
     * @return {@link Interactable}
     */
    public Interactable getCurrentObject () {
        return currentObject;
    }

    /**
     * This function returns {@code true} or {@code false} based on if this person is able to successfully wait and
     * occupy the {@link Interactable} object. If this person already is interacting with some other {@link
     * Interactable}, throw {@link InteractionNotPossibleException}, otherwise wait for given {@code seconds} and try
     * {@code maxTries} number of times in given {@code seconds} to occupy the given {@code object}.
     *
     * @param object   {@link Interactable}
     * @param seconds  number of seconds to wait for occupation of the {@code object}
     * @param maxTries number of maximum tries in given {@code seconds}
     *
     * @return {@code true} or {@code false}
     *
     * @throws PersonNotInRoomException        if this person is not in the {@link Person#room}
     * @throws InteractionNotPossibleException if due to some reason interacting with {@code object} is not possible
     */
    public boolean waitToOccupy (final Interactable object, final long seconds, final int maxTries)
        throws PersonNotInRoomException, InteractionNotPossibleException {
        if (this.currentObject != null) {
            throw new InteractionNotPossibleException(this + " already is interacting with " +
                                                      "" + currentObject);
        }
        boolean flag     = false;
        int     noOfSecs = 0;
        int     secs     = (int) seconds / maxTries;
        int     trie     = 0; // Intentional typo since try is a keyword :(
        while (!flag && noOfSecs < seconds) {
            trie++;
            System.out.println("Trying for " + trie + " time.");
            try {
                Thread.sleep(secs * 1000);
                noOfSecs += secs;
                synchronized (object) {
                    flag = tryOccupying(object);
                }
            } catch (PersonNotInRoomException e) {
                throw new PersonNotInRoomException("Person is not in room.");
            } catch (InteractionNotPossibleException e) {
                if (!e.getMessage().equalsIgnoreCase(object + " is already in use by some other " +
                                                     "person.")) {
                    if (e.getMessage().equalsIgnoreCase(this + " already is interacting with " +
                                                        "" + currentObject)) {
                        throw new InteractionNotPossibleException(e.getMessage());
                    }
                    throw new InteractionNotPossibleException(
                        "There is something wrong to wait for this object to acquire.");
                }
            } catch (InterruptedException e) {
                System.out.println("Leaving Wait state due to below interrupt.");
                e.printStackTrace();
            }
        }
        return flag;
    }

    /**
     * This function tries to occupy the {@code object} and returns {@code true} if successfully occupies it or
     * {@code false} if due to any reason, fails to occupy.
     *
     * @param object {@link Interactable}
     *
     * @return {@code true} or {@code false}
     *
     * @throws PersonNotInRoomException        if this person is not in the room
     * @throws InteractionNotPossibleException if someone is already interacting with {@code object}
     */
    public boolean tryOccupying (final Interactable object)
        throws PersonNotInRoomException, InteractionNotPossibleException {
        if (this.currentObject != null) {
            throw new InteractionNotPossibleException(this + " already is interacting with " +
                                                      "" + currentObject);
        }
        synchronized (object) {
            boolean isOccupied = false;
            if (room == null || !room.hasPerson(this)) {
                throw new PersonNotInRoomException("Person is not in any room.");
            }
            if (!room.hasObject(object)) {
                throw new InteractionNotPossibleException(object + " does not belong to " + room);
            }
            if (currentObject == null) {
                isOccupied = object.tryOccupiedBy(this, Interactions.SIT);
                System.out.println(this + " has occupied " + object);
                this.currentObject = object;
            }
            return isOccupied;
        }
    }

    /**
     * Returns current {@link Position} of this person. e.g. {@link Person.Position#SIT} or {@link
     * Person.Position#STAND}
     *
     * @return {@link Position}
     */
    public Position getCurrentPosition () {
        return position;
    }

    /**
     * This function tries to change the person's {@link Position}. If this person was already sitting e.g. {@link
     * Position#SIT} and tries to stand {@link Position#STAND}, this function makes sure to release the {@link
     * Interactable} {@code SIT} object.
     *
     * @param position {@link Position}
     *
     * @throws NoCurrentInteraction if this person is not currently interacting with the {@link Interactable} that
     *                              has {@code SIT} interaction.
     */
    public void changePosition (final Position position)
        throws NoCurrentInteraction {
        Objects.requireNonNull(position);
        if (position == Position.STAND) {
            if (currentObject != null) releaseObject(currentObject);
        }
        this.position = position;
    }

    /**
     * Release the {@code object} if it is being interacted by this person.
     *
     * @param object {@link Interactable}
     *
     * @throws NoCurrentInteraction if given {@code object} is not being interacted by this person.
     */
    public void releaseObject (final Interactable object)
        throws NoCurrentInteraction {
        synchronized (object) {
            currentObject.unOccupiesMe(this);
            currentObject = null;
        }
    }

    public String getName () {
        return name;
    }

    /**
     * Sets the {@link Room} {@code room} for this person only if this person has already entered {@code room}
     *
     * @param room of {@link Room}
     */
    void setRoom (final Room room) {
        if (room.getPersons().contains(this)) {
            this.room = room;
        }
    }

    /**
     * Un sets the room for this person whether person was in room or not.
     */
    void unsetRoom () {
        this.room = null;
        position = null;
    }

    @Override
    public String toString () {
        return name;
    }

    /**
     * When an object implementing interface <code>Runnable</code> is used
     * to create a thread, starting the thread causes the object's
     * <code>run</code> method to be called in that separately executing
     * thread.
     * <p>
     * The general contract of the method <code>run</code> is that it may
     * take any action whatsoever.
     *
     * @see Thread#run()
     */
    @Override
    public void run () {
        long currentMilliseconds = 0;
        while (currentMilliseconds <= room.getDuration()) {
            try {
                Thread.sleep(1000);
                currentMilliseconds += 1000;
            } catch (InterruptedException e) {
                System.out.println(this + " is interrupted");
            }
        }
        leaveRoom();
    }

    /**
     * Leaves the room
     */
    private void leaveRoom () {
        try {
            room.personExits(this);
        } catch (RoomEmptyException e) {
            e.printStackTrace();
        } catch (PersonNotInRoomException e) {
            e.printStackTrace();
        } catch (NoCurrentInteraction noCurrentInteraction) {
            noCurrentInteraction.printStackTrace();
        }
        this.room = null;
        position = null;
    }

    /**
     * Person can be in any of the given two positions.
     */
    protected enum Position {
        SIT, STAND
    }
}

I want to know the problems that I might not see at the moment but can occur in future? Also, is there much better way to improve this piece of code with respect to multi-threading?

If someone here can help me cloning or downloading the whole project and suggest about the overall architecture of application and multi-threading, I will be very very thankful. But since codereview is only to review some particular piece of code, so here it is, I want to review.

\$\endgroup\$
0

1 Answer 1

3
\$\begingroup\$

Exceptions and Responsibilities

throws RoomFullException, InteractionNotPossibleException, PersonNotInRoomException, NoCurrentInteraction {

Wow that's a lot of exceptions. Let's talk about the purpose of exceptions for a moment, because that definitely wasn't the point of exceptions....

Exceptions are exceptional circumstances in your program. Things that shouldn't happen but for whatever reason do. Things like files going missing, not having an internet connection, not having an object to call a method on, ...

Exceptions are not things that are expected. Things like: The user didn't add required data, The data the user tries to access was deleted, ...

Sometimes it makes sense to handle expected circumstances through exceptions. But that's rather seldom.

Ideally you'd check whether the room is full before trying to enter it, You'd check whether the object can be interacted with before trying to do so, ...

In general your code is rather clean and obvious in what it does, but your method does a lot of things. It checks whether the room is full, checks whether there's Objects the Person can sit on, and if there are makes the person try to occupy a chair.
Also it gives the Room a reference to the Person and the Person a reference to the Room.

That's too many things for a single method. Split them up into sub-methods. Consider something like:

public synchronized void Person personEnters(final Person person) {
    // omitted throws clauses for brevity
    checkRoomCapacity();
    checkSittingPossible(person);
    enterRoom(person);
    if (person.getCurrentPosition == Position.SIT) {
       tryOccupyChair(person);
    }
    return person;
}

This makes your method significantly more abstracted. Also it allows you to synchronize with finer graining, if possible. In this case here it doesn't but for other methods it might.


Nitpicks

  • You violate line-lenghts in some places. Once in the throws-clause, once in your check for free seats.
  • You abuse System.out as a logger. Don't do that, it's significantly slower and more inconvenient than proper logging (think about finding all errors or warnings or other logging analysis ideas). As I understand it's a toy project, so for now it's not important, but it's something you should not get into the habit of doing.
  • System.out.println("Something is wrong."); is a really unhelpful error message. First off you could miss it, secondly it doesn't tell you anything.
  • I'm not sold on returning the person from the method. That makes little sense to me. I'd expect this method to be a void method, especially since you're doing all your error- and precondition-checking through exceptions.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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