Trip Advisor - Train routes

Question

Gives advice to the user on which trains to take to reach a destination

 public class TripAdvisor {
        private Station source;
        private Station destination;
        private double cost;
        /**
         * 
         * @param path the shortest path returned by the shortestPath method implementation
         */
        TripAdvisor(ArrayList<Station> path)
        {
            Lane color=null;
            ArrayList<Station> changeStation =new ArrayList<Station>();//used to identify when the user needs to change lines
            ArrayList<Object> trains=new ArrayList<>();//list of trains the user need to take to reach a station
            for(int i=0;i<(path.size()-1);i++)
            {

                Station current=path.get(i);
                Station next=path.get(i+1);
                for (Connection e : current.adjacencies)
                {
                    if(e.getTarget().equals(next))
                    {
                        Lane train=e.getLaneColor();
                        trains.add(train);
                        if(!e.getLaneColor().equals(color)&&(color!=null))
                        {
                            changeStation.add(current);
                        }
                    }
                    color=e.getLaneColor();
                }
            }

            System.out.println("trains"+ "$" +trains);
            if(trains.size()==1)
            {
                int q;
                System.out.println("Take a "+ trains.get(0) +"liner from" + path.get(0) +"to reach" + path.get( q=path.size()));
            }
            else
            {
                System.out.println("Take a "+ trains.get(0) +"liner from" + path.get(0)+ "Get down at" + changeStation.get(0));

            }
        }

    }
    /**
     * 
     * @author PrasannaAarthiB
     * This has all the lines in the metro.They are represented by different colors
     */
    public enum Lane{
        RED,BLUE,GREEN,YELLOW,BLACK
    }

Answer 1

First of all, you have a possible bug in your code.

If trains is empty you will get a problem at the printing of the results.

• Don't code that much inside the constructor. The constructor should be used for initializing only.

• Be consitent in the way you create objects

  ArrayList<Station> changeStation =new ArrayList<Station>();
  ArrayList<Object> trains=new ArrayList<>();  
  

• If you need comments to describe a variable, you have named the variable poorly

• Let your code breathe, use some spacing

  for(int i=0;i<(path.size()-1);i++)  
  

  this would be easier to read like

  for (int i = 0 ; i < path.size() - 1; i++)  
  

• use methods for calculations. You should add a method which calculates the shortest path and another one that would be responsible for the output of these calculated values.

• code against interfaces instead of implementations so

  ArrayList<Station> changeStation =new ArrayList<Station>();
  ArrayList<Object> trains=new ArrayList<>();  
  

  should be

  List<Station> changeStation =new ArrayList<>();
  List<Lane> trains=new ArrayList<>();  
  

• Return from methods what the methodnames implies and name the "receiving" variable accordingly

  Lane train=e.getLaneColor();  
  

  So the enum Lane should be become LaneColor and train should become e.g. currentLaneColor

Answer 2

Constructor

You seem to have everything in your constructor. This isn't really great practice. You use your constructor to initialize all of your variables, so you'd have something like:

TripAdvisor()
{
    source = new Station();
    destination = new Station();
    cost = 0;
}

TripAdvisor(Station source, Station dest)
{
    this.source = source;
    this.destination = dest;
    cost = 0; //This isn't necessary, just nice to explicitly state 
}

After this, you'd split your method up into different calls. You might have a void Print() method that prints out the path, as well as an overridden string toString() method, possibly?

"Calculate" methods

On top of this, you could have an ArrayList<Station> calculatePath() and an ArrayList<Station> calculatePath(Station source, Station dest) which runs through your algorithm and returns the path that should be taken. Note that you'll have to check whether the stations have been set in the no-args method.

Unused (but useful) variables

Although, having written all of that, you don't actually appear to be using source or destination, so it's possible that a further re-design is required, but I can't help with that right now, as I don't know what you're doing outside of this class. However, I can say that if you're representing the network like you are, giving this class a copy of the network as a private variable (passed in in the constructor, maybe) and then giving it a source and destination is a good way to design a class which is performing an algorithm.

Patterns?

This sort of implementation means that when you want a new route planning you don't have to create a new instance of your TripAdvisor class, you can just re-use your old one and save on memory. This is reasonably close to a singleton pattern, so you might want to do some research on best practices for them, and good ways to use them.

Answer 3

The purpose of a constructor is to create an object. What is the purpose of a TripAdvisor object here? It seems it has no purpose as an object. You "create" a TripAdvisor object only to get something printed on the output. Constructor is not the right element for this, what you need is a method, let's call it printShortestPath.

---

The source, destination and cost member fields are unused. Remove them.

---

Use interface types like List<> as opposed to ArrayList<> in variable and method parameter declarations, unless you have a specific need for an implementation type, like this:

public static void printShortestPath(List<Station> path) {
    List<Station> changeStation = new ArrayList<>();

---

This variable is quite confusing:

Lane color = null;

Is lane a color? Further down you also have a Lane train. Any of these would be better options:

Lane lane = null;
LaneColor laneColor = null;
Train train = null;

---

Instead of ArrayList<Object> trains, you should make use of Java's type system and declare like this instead:

List<Lane> trains = new ArrayList<>();

However, as the earlier point with Lane color, this is confusing. If this is a list of lanes, then it would be better to name it lanes.

I think you need to rethink your objects and their relationships. The fact that it's hard to find the right names to describe how the program should work indicates that maybe there is a flaw in the design. When all classes and object names become natural, the code will become a lot easier to understand.

---

for (Connection e : current.adjacencies) {
    if (e.getTarget().equals(next)) {
        Lane train = e.getLaneColor();
        trains.add(train);
        if (!e.getLaneColor().equals(color) && (color != null)) {
            changeStation.add(current);
        }
    }
    color = e.getLaneColor();
}

Several points to improve here:

• connection would have been a much better name for a Connection object instead of e
• It's a bit hard to follow what happens to the color variable during this loop. Probably the code can be reorganized in a way that's more natural and easier to follow
• The inconsistent naming of Lane vs color vs train is a major burden on the reader's mind. You really need to have something more consistent, more clear and natural
• The parentheses around (color != null) are unnecessary