0
\$\begingroup\$

I've been working on an agent-based model as part of my PhD and I've hit a roadblock. I started learning Java by doing this project and I definitely started to run before I could walk. The problem is I'm too far along in my PhD to give up on this par. Also, the work is heavily based on an example that comes with the agent-based modelling software (called Mason) and is a bit hacky. I want to start fresh, but I'm running out of time and I've reached the point where I think the work I need to do is beyond my skills.

I would love some brutal feedback and guidance. I'm really struggling to re-engage with the model and finish my PhD. I need some constructive criticism and feedback to get me back in the groove.

The model reads a number of GIS shapefiles and displays a road network, and two Environment Agency flood maps and a bespoke Open Source Vulnerability Index (OSVI) as simple polygons. The model reads in a .CSV and generates a predetermined number of agents with set characteristics. The agents are placed on the road network and are located at set start point. Each agent is assigned a goal location, and a random speed. Once the model is started, the agents move from A to B, then they change direction and head back to their start position. The process repeats until the user quits.

The main code is below. If theres anything else you need, let me know. The whole thing has become quite convoluted and is spread across multiple files.

MK_7.java

package sim;

import java.io.BufferedReader;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.URL;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Random;
import com.linuxense.javadbf.*;
import java.io.*;

import sim.engine.SimState;
import sim.field.geo.GeomVectorField;
import sim.io.geo.ShapeFileImporter;
import sim.util.Bag;
import sim.util.geo.GeomPlanarGraph;
import sim.util.geo.GeomPlanarGraphEdge;
import sim.util.geo.MasonGeometry;

import com.vividsolutions.jts.geom.Coordinate;
import com.vividsolutions.jts.geom.Envelope;
import com.vividsolutions.jts.geom.GeometryFactory;
import com.vividsolutions.jts.geom.Point;
import com.vividsolutions.jts.planargraph.Node;

import ec.util.MersenneTwisterFast;

public class MK_7 extends SimState  {

    //////////////////////////////////////////////////////////////////////////////
    ///////////////////////////// MODEL PARAMETERS ///////////////////////////////
    //////////////////////////////////////////////////////////////////////////////

    private static final long serialVersionUID = -4554882816749973618L;

    ///////////////////////////// Containers /////////////////////////////////////
    public GeomVectorField roads = new GeomVectorField();
    public GeomVectorField world = new GeomVectorField();
    public GeomVectorField flood3 = new GeomVectorField();
    public GeomVectorField flood2 = new GeomVectorField();
    //public GeomVectorField HouseholdsFZ = new GeomVectorField();
    //public GeomVectorField Households = new GeomVectorField();
    public GeomVectorField agents = new GeomVectorField();
    //public GeomVectorField ngoagents = new GeomVectorField();

    ///////////////////////////// Network ////////////////////////////////////////
    public GeomPlanarGraph network = new GeomPlanarGraph();
    // Stores road network connections
    public GeomVectorField junctions = new GeomVectorField();
    // Stores nodes for road intersections
    HashMap<Integer, GeomPlanarGraphEdge> idsToEdges =
        new HashMap<Integer, GeomPlanarGraphEdge>();
    public HashMap<GeomPlanarGraphEdge, ArrayList<agents.MainAgent>> edgeTraffic =
        new HashMap<GeomPlanarGraphEdge, ArrayList<agents.MainAgent>>();
    public GeomVectorField mainagents = new GeomVectorField();

    // Model ArrayLists for agents and OSVI Polygons
    ArrayList<agents.MainAgent> agentList = new ArrayList<agents.MainAgent>();
    ArrayList<Polygon> polys = new ArrayList<Polygon>();
    ArrayList<String> csvData = new ArrayList<String>();

    // Here we force the agents to go to or from work at any time
    public boolean goToWork = true;
    public boolean getGoToWork()    {
        return goToWork;
    }

    //////////////////////////////////////////////////////////////////////////////
    /////////////////////////// BEGIN FUNCTIONS //////////////////////////////////
    //////////////////////////////////////////////////////////////////////////////

    /**
     * //////////////////////// Model Constructor ////////////////////////////////
     * Model Constructor
     */
    public MK_7(long seed)  {
        super(seed);
        random = new MersenneTwisterFast(12345);
    }

    /**
     * //////////////////////// OSVI Polygon Setup ///////////////////////////////
     * Polygon Setup
     */
    void setup()
    {
        // copy over the geometries into a list of Polygons
        Bag ps = world.getGeometries();
        polys.addAll(ps);
        }

    /**
     * //////////////////////// Model Initialisation /////////////////////////////
     * Model Initialisation
     */
    @Override
    public void start() {
        super.start();
        System.out.println("Reading shapefiles...");

        //////////////////////////////////////////////////////////////////////////
        /////////////////////////// READ IN DATA /////////////////////////////////
        //////////////////////////////////////////////////////////////////////////

        /*
        src/
            data/
                .shp, .dbf, .csv ...
        sim/
            MK_7/
                .java files
         */

        try {
            // read in the roads shapefile to create the transit network
            URL roadsFile = MK_7.class.getResource
                    ("/data/Final_ITN.shp");
            ShapeFileImporter.read(roadsFile, roads);
            System.out.println("    Roads shapefile: " +roadsFile);

            Envelope MBR = roads.getMBR();

            // read in the LSOA shapefile to create the backgrounds
            // URL areasFile = MK_4.class.getResource
            //      ("/data/Final_LSOA.shp");
            //   Bag desiredAttributes = new Bag();
            //   desiredAttributes.add("RC_RankCol");
            //
            //   try {
            //      ShapeFileImporter.read(areasFile, world, desiredAttributes);
            //   }
            //   catch (FileNotFoundException ex){
            //      }

            URL wardsFile = MK_7.class.getResource
                    ("/data/Final_LSOA.shp");
            ShapeFileImporter.read(wardsFile, world, Polygon.class);
            System.out.println("    LSOA shapefile: " +wardsFile);

            MBR.expandToInclude(world.getMBR());

            // read in the FZ3 file
            URL flood3File = MK_7.class.getResource
                    ("/data/NorfolkFZ3.shp");
            ShapeFileImporter.read(flood3File, flood3);
            System.out.println("    FZ3 shapefile: " +flood3File);

            MBR.expandToInclude(flood3.getMBR());

            // read in the FZ2 file
            URL flood2File = MK_7.class.getResource
                    ("/data/NorfolkFZ2.shp");
            ShapeFileImporter.read(flood2File, flood2);
            System.out.println("    FZ2 shapefile: " +flood2File);

            MBR.expandToInclude(flood2.getMBR());

            /*
            // read in the household files
            URL HouseholdsFZFile = MK_2.class.getResource
                    ("/data/Buildings_IN_FZ_Snapped_to_ITN.shp");
            ShapeFileImporter.read(HouseholdsFZFile, HouseholdsFZ);
            System.out.println("Households in FZ shapefile: " +HouseholdsFZFile);

            MBR.expandToInclude(HouseholdsFZ.getMBR());

            // read in the FZ2 file
            URL HouseholdsFile = MK_2.class.getResource
                    ("/data/Buildings_NOT_in_FZ_Snapped_to_ITN.shp");
            ShapeFileImporter.read(HouseholdsFile, Households);
            System.out.println("Households not in FZ shapefile: " +HouseholdsFile);
            System.out.println();

            MBR.expandToInclude(Households.getMBR());
            */

            createNetwork();
            setup();

            //////////////////////////////////////////////////////////////////////
            /////////////////////////// CLEANUP //////////////////////////////////
            //////////////////////////////////////////////////////////////////////

            // clear any existing agents from previous runs
            agents.clear();
            //ngoagents.clear();

            //////////////////////////////////////////////////////////////////////
            /////////////////////////// AGENTS ///////////////////////////////////
            //////////////////////////////////////////////////////////////////////

            // initialize agents using the following source .CSV files
            agentGoals("/data/AgentGoals.csv");
            populateAgent("/data/NorfolkITNAGENT.csv");

            System.out.println();
            System.out.println("Starting simulation...");

            // standardize the MBRs so that the visualization lines up
            // and everyone knows what the standard MBR is
            roads.setMBR(MBR);
            world.setMBR(MBR);
            flood3.setMBR(MBR);
            flood2.setMBR(MBR);
            //HouseholdsFZ.setMBR(MBR);
            //Households.setMBR(MBR);
            agents.setMBR(MBR);
            //ngoagents.setMBR(MBR);

            // Ensure that the spatial index is updated after all the agents move
            schedule.scheduleRepeating( agents.scheduleSpatialIndexUpdater(),
                    Integer.MAX_VALUE, 1.0);
            //schedule.scheduleRepeating( ngoagents.scheduleSpatialIndexUpdater(),
            //      Integer.MAX_VALUE, 1.0);


            /** Steppable that flips Agent paths once everyone reaches
             * their destinations

            Steppable flipper = new Steppable() {
                @Override
                public void step(SimState state)
                {

                    MK_7 gstate = (MK_7) state;

                    // pass to check if anyone has not yet reached work
                    //for (MainAgent a : gstate.agentList)
                    //{
                        ///if (!a.reachedDestination)
                        //{
                        //    return; // someone is still moving: let him do so
                      //  }
                    //}
                    // send everyone back in the opposite direction now
                    //boolean toWork = gstate.goToWork;
                   // gstate.goToWork = !toWork;

                    // otherwise everyone has reached their latest destination:
                    // turn them back
                    for (MainAgent a : gstate.agentList)
                        if (a.reachedDestination) {
                            a.flipPath();
                        }
                }
            };
            schedule.scheduleRepeating(flipper, 10);
             */

        } catch (FileNotFoundException e)
        {
            System.out.println("Error: missing required data file");
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    /**
     * //////////////////////// Model Finish & Cleanup ///////////////////////////
     * Finish the simulation and clean up
     */
    public void finish()    {
        super.finish();
        System.out.println();
        System.out.println("Simulation ended by user.");
        /*
        System.out.println("Attempting to export agent data...");
        try {
            ShapeFileExporter.write("agents", agents);
        } catch (Exception e)   {
            System.out.println("Export failed.");
            e.printStackTrace();
        }
        */
    }

    /**
     * //////////////////////// Create Road Network //////////////////////////////
     * Create the road network the agents will traverse
     */
    private void createNetwork()    {
        System.out.println("Creating road network..." +roads);
        System.out.println();
        network.createFromGeomField(roads);

        for (Object o : network.getEdges()) {
            GeomPlanarGraphEdge e = (GeomPlanarGraphEdge) o;

            idsToEdges.put(e.getIntegerAttribute("ROAD_ID").intValue(), e);

            e.setData(new ArrayList<agents.MainAgent>());
        }

        addIntersectionNodes(network.nodeIterator(), junctions);
    }


    /**
    * ///////////////////////// Setup agentGoals /////////////////////////////////
    * Read in the agent goals CSV
    * @param agentfilename
    * @return
    *
    */
    public ArrayList<String> agentGoals(String agentfilename) throws IOException{
        String csvGoal = null;
        BufferedReader agentGoalsBuffer = null;

        String agentFilePath = MK_7.class.getResource(agentfilename).getPath();
        FileInputStream agentfstream = new FileInputStream(agentFilePath);
        System.out.println("Reading Agent's Goals CSV file: " +agentFilePath);

        try {
            agentGoalsBuffer = new BufferedReader
                    (new InputStreamReader(agentfstream));
            agentGoalsBuffer.readLine();
            while ((csvGoal = agentGoalsBuffer.readLine()) != null) {
                String[] splitted = csvGoal.split(",");

                ArrayList<String> agentGoalsResult = 
                        new ArrayList<String>(splitted.length);
                for (String data : splitted)
                    agentGoalsResult.add(data);
                csvData.addAll(agentGoalsResult);
            }
            System.out.println("Full csvData Array: " +csvData);

        } finally {
            if (agentGoalsBuffer != null)
                agentGoalsBuffer.close();
        }
        return csvData;
    }

    /**
     * //////////////////////// Setup mainAgent //////////////////////////////////
     * Read in the population files and create appropriate populations
     * @param filename
     */
    public void populateAgent(String filename)  {
        try {
            String filePath = MK_7.class.getResource(filename).getPath();
            FileInputStream fstream = new FileInputStream(filePath);
            System.out.println();
            System.out.println("Populating model with Agents: " +filePath);

            BufferedReader d = new BufferedReader(new InputStreamReader(fstream));
            String s;

            // get rid of the header
            d.readLine();
            // read in all data
            while ((s = d.readLine()) != null)  {
                String[] bits = s.split(",");

                int pop = Integer.parseInt(bits[2]);

                //moveRate = (int)(Math.random()*70) + 1;
                //System. out.println("MoveRate = " + moveRate );
                //int mainAgentSpeed = MainAgent.MoveRate;
                //System.out.println("Main Agent speed = " +mainAgentSpeed);

                String homeTract = bits[3];
                String ROAD_ID = bits[3];

                Random randomiser = new Random();
                String random = csvData.get(new Random().nextInt(csvData.size()));
                String goalTract = random;
                System.out.println();
                System.out.println("Agent goalTract: " +goalTract);

                GeomPlanarGraphEdge startingEdge = idsToEdges.get(
                        (int) Double.parseDouble(ROAD_ID));
                GeomPlanarGraphEdge goalEdge = idsToEdges.get(
                        (int) Double.parseDouble(goalTract));
                        //reads the .CSV column
                        //goals[ random.nextInt(goals.length)]);
                        // uses the hardcoded 'goals' from above

                for (int i = 0; i < pop; i++)   {
                    //pop; i++) {   // NO IDEA IF THIS MAKES A DIFFERENCE!?!
                    agents.MainAgent a = new agents.MainAgent
                            (this, homeTract, startingEdge, goalEdge);

                    boolean successfulStart = a.start(this);
                    //System.out.println("Starting...");

                    if (!successfulStart)   {
                        System.out.println("Main agents added successfully!!");
                        continue; // DON'T ADD IT if it's bad
                    }

                    //MasonGeometry newGeometry = new MasonGeometry(a.getGeometry());
                    MasonGeometry newGeometry = a.getGeometry();
                    newGeometry.isMovable = true;
                    agents.addGeometry(newGeometry);
                    agentList.add(a);
                    schedule.scheduleRepeating(a);
                }
            }

            d.close();

        } catch (Exception e) {
                System.out.println("ERROR: issue with population file: ");
                e.printStackTrace();
        }
    }

    /**
     * //////////////////////// Network Intersections ////////////////////////////
     * adds nodes corresponding to road intersections to GeomVectorField
     *
     * @param nodeIterator Points to first node
     * @param intersections GeomVectorField containing intersection geometry
     *
     * Nodes will belong to a planar graph populated from LineString network.
     */
    private void addIntersectionNodes(Iterator<?> nodeIterator,
                                      GeomVectorField intersections)    {
        GeometryFactory fact = new GeometryFactory();
        Coordinate coord = null;
        Point point = null;
        @SuppressWarnings("unused")
        int counter = 0;

        while (nodeIterator.hasNext())  {
            Node node = (Node) nodeIterator.next();
            coord = node.getCoordinate();
            point = fact.createPoint(coord);

            junctions.addGeometry(new MasonGeometry(point));
            counter++;
        }
    }

    /**
     * //////////////////////// Main Function ////////////////////////////////////
     * Main function allows simulation to be run in stand-alone, non-GUI mode
     */
    public static void main(String[] args)  {
        doLoop(MK_7.class, args);
        System.exit(0);
    }
}

MK_7WithUI.java

package sim;

import java.awt.Color;
import java.awt.Graphics2D;

import javax.swing.JFrame;

import org.jfree.data.xy.XYSeries;

import sim.display.Console;
import sim.display.Controller;
import sim.display.Display2D;
import sim.display.GUIState;
import sim.engine.SimState;
import sim.engine.Steppable;
import sim.portrayal.DrawInfo2D;
import sim.portrayal.geo.GeomPortrayal;
import sim.portrayal.geo.GeomVectorFieldPortrayal;
import sim.util.media.chart.TimeSeriesChartGenerator;
import agents.MainAgent;

public class MK_7WithUI extends GUIState    {

    //////////////////////////////////////////////////////////////////////////////
    /////////////////////////// DISPLAY FUNCTIONS ////////////////////////////////
    //////////////////////////////////////////////////////////////////////////////

    private Display2D display;
    private JFrame displayFrame;

    //private GeomVectorFieldPortrayal lsoaPortrayal = new GeomVectorFieldPortrayal();
    GeomVectorFieldPortrayal polyPortrayal = new GeomVectorFieldPortrayal(true);
    private GeomVectorFieldPortrayal roadsPortrayal = new GeomVectorFieldPortrayal(true);
    private GeomVectorFieldPortrayal flood3Portrayal = new GeomVectorFieldPortrayal();
    private GeomVectorFieldPortrayal flood2Portrayal = new GeomVectorFieldPortrayal();
    private GeomVectorFieldPortrayal agentPortrayal = new GeomVectorFieldPortrayal();
    //SparseGridPortrayal2D agentPortrayal = new SparseGridPortrayal2D();
    TimeSeriesChartGenerator trafficChart;
    XYSeries maxSpeed;
    XYSeries avgSpeed;
    XYSeries minSpeed;

    //////////////////////////////////////////////////////////////////////////////
    /////////////////////////// BEGIN FUNCTIONS //////////////////////////////////
    //////////////////////////////////////////////////////////////////////////////

    /**
     * ///////////////////////// Default constructor /////////////////////////////
     * Default constructor
     */
    protected MK_7WithUI(SimState state)    {
            super(state);
        }

    /**
     * //////////////////////// Portrayal Setup //////////////////////////////////
     * Sets up the portrayals and charts for the simulation
     */
    private void setupPortrayals()  {
        sim.MK_7 world = (sim.MK_7) state;

        // the polygon portrayal
        polyPortrayal.setField(world.world);
        polyPortrayal.setPortrayalForAll(new PolyPortrayal());

        display.reset();

        display.repaint();
    }

        /**
         * ///////////////////////// Main Function ///////////////////////////////
         *
         * Main function to run the simulation
         * @param args
         */
        public static void main(String[] args)  {
            MK_7WithUI simple = new MK_7WithUI(
                    new sim.MK_7(System.currentTimeMillis()));
            Console c = new Console(simple);
            c.setVisible(true);
        }

        /**
         * //////////////////////// Simulation Name //////////////////////////////
         * @return name of the simulation
         */
        public static String getName()  {
            return "EngD ABM Model MK_7";
        }

        /**
         *  /////////////////////// Model Modification ///////////////////////////
         *  This must be included to have model tab, which allows mid-simulation
         *  modification of the coefficients
         */
        public Object getSimulationInspectedObject()    {
            return state;
        }  // non-volatile

        /**
         * //////////////////////// Model Setup //////////////////////////////////
         * Called when starting a new run of the simulation. Sets up the portrayals
         * and chart data.
         */
        public void start() {
            super.start();

            setupPortrayals();

            sim.MK_7 world = (sim.MK_7) state;

            maxSpeed = new XYSeries("Max Speed");
            avgSpeed = new XYSeries("Average Speed");
            minSpeed = new XYSeries("Min Speed");
            trafficChart.removeAllSeries();
            trafficChart.addSeries(maxSpeed, null);
            trafficChart.addSeries(avgSpeed, null);
            trafficChart.addSeries(minSpeed, null);

            state.schedule.scheduleRepeating(new Steppable()    {
                private static final long serialVersionUID = -3749005402522867098L;

                public void step(SimState state)    {
                    sim.MK_7 world = (sim.MK_7) state;
                    double maxS = 0, minS = 10000, avgS = 0, count = 0;
                    //////////////////////////// Main Agent //////////////////////
                    for (MainAgent a : world.agentList) {
                        if (a.reachedGoal)  {
                            continue;
                        }
                        count++;
                        double speed = Math.abs(a.speed);
                        avgS += speed;
                        if (speed > maxS)   {
                            maxS = speed;
                        }
                        if (speed < minS)   {
                            minS = speed;
                        }
                    }

                    double time = state.schedule.time();
                    avgS /= count;
                    maxSpeed.add(time, maxS, true);
                    minSpeed.add(time, minS, true);
                    avgSpeed.add(time, avgS, true);
                }
            });

            /**
             * Sets up the portrayals within the map visualization.
             */

            roadsPortrayal.setField(world.roads);
            roadsPortrayal.setPortrayalForAll(new GeomPortrayal
                    (Color.DARK_GRAY, 0.0005, false));
            polyPortrayal.setField(world.world);
            polyPortrayal.setPortrayalForAll(new PolyPortrayal());
            flood3Portrayal.setField(world.flood3);
            flood3Portrayal.setPortrayalForAll(new GeomPortrayal
                    (Color.CYAN, true));
            flood2Portrayal.setField(world.flood2);
            flood2Portrayal.setPortrayalForAll(new GeomPortrayal
                    (Color.BLUE, true));
            agentPortrayal.setField(world.agents);
            agentPortrayal.setPortrayalForAll(new GeomPortrayal
                    (Color.MAGENTA, 150, true));
            //agentPortrayal.setPortrayalForAll(new GeomPortrayal());

            display.reset();
            display.setBackdrop(Color.WHITE);
            display.repaint();

        }

        /**
         * /////////////////////// Poly Portrayal Colours ////////////////////////
         * The portrayal used to display Polygons with the appropriate color
         * */
        class PolyPortrayal extends GeomPortrayal
        {

            private static final long serialVersionUID = 1L;

            @Override
            public void draw(Object object, Graphics2D graphics, DrawInfo2D info)
            {
                Polygon poly = (Polygon) object;

                if (poly.getSoc().equals("red"))
                {
                    paint = Color.red;
                }

                else if (poly.getSoc().equals("orange"))
                {
                    paint = Color.orange;
                }

                else if (poly.getSoc().equals("yellow"))
                {
                    paint = Color.yellow;
                }

                else if (poly.getSoc().equals("green"))
                {
                    paint = Color.green;
                }
                else
                {
                    paint = Color.gray;
                }

                super.draw(object, graphics, info);
            }

        }

        /**
         * /////////////////////// Visualisation Format //////////////////////////
         * Initializes the simulation visualization. Sets up the display
         * window, the JFrames, and the chart structure.
         */
        public void init(Controller c)
        {
            super.init(c);

            /////////////////////////// MAIN DISPLAY /////////////////////////////
            // makes the displayer and visualises the maps
            display = new Display2D(1200, 600, this);
            // turn off clipping
            // display.setClipping(false);

            displayFrame = display.createFrame();
            displayFrame.setTitle("EngD ABM Model MK_7");
            c.registerFrame(displayFrame); // register the frame so it appears in

            // Put portrayals in order from bottom layer to top
            displayFrame.setVisible(true);
            display.attach(polyPortrayal, "LSOA");
            display.attach(flood2Portrayal, "FZ2 Zone");
            display.attach(flood3Portrayal, "FZ3 Zone");
            display.attach(roadsPortrayal, "Roads");
            display.attach(agentPortrayal, "Agents");

            ///////////////////////////// CHART //////////////////////////////////
            trafficChart = new TimeSeriesChartGenerator();
            trafficChart.setTitle("Traffic Stats");
            trafficChart.setYAxisLabel("Speed");
            trafficChart.setXAxisLabel("Time");
            JFrame chartFrame = trafficChart.createFrame(this);
            chartFrame.pack();
            c.registerFrame(chartFrame);

        }

        /**
         * /////////////////////// Model Finish //////////////////////////////////
         * Quits the simulation and cleans up.
         */
        public void quit()  {
            System.out.println("Model closed.");
            super.quit();

            if (displayFrame != null)   {
                displayFrame.dispose();
            }
            displayFrame = null; // let gc
            display = null; // let gc
        }
    }

MainAgent.java

package agents;

import java.awt.Color;
import java.awt.Graphics2D;
import java.util.ArrayList;

import network.AStar;
import sim.MK_7;
import sim.engine.SimState;
import sim.engine.Steppable;
import sim.portrayal.DrawInfo2D;
import sim.util.geo.GeomPlanarGraphDirectedEdge;
import sim.util.geo.GeomPlanarGraphEdge;
import sim.util.geo.MasonGeometry;
import sim.util.geo.PointMoveTo;

import com.vividsolutions.jts.geom.Coordinate;
import com.vividsolutions.jts.geom.GeometryFactory;
import com.vividsolutions.jts.geom.LineString;
import com.vividsolutions.jts.linearref.LengthIndexedLine;
import com.vividsolutions.jts.planargraph.Node;

public final class MainAgent implements Steppable   {
    private static final long serialVersionUID = -1113018274619047013L;

    //////////////////////////////////////////////////////////////////////////////
    /////////////////////////////// PARAMETERS ///////////////////////////////////
    //////////////////////////////////////////////////////////////////////////////

    MK_7 world;
    // Residence/Work Attributes

    String homeTract = "";
    String goalTract = "";
    Node homeNode = null;
    Node workNode = null;
    // point that denotes agent's position
    // private Point location;
    private MasonGeometry location; // point that denotes agent's position
    // How much to move the agent by in each step()
    private double basemoveRate = 10.0;
    private double moveRate = basemoveRate;
    //private double moveRate = 70;
    private LengthIndexedLine segment = null;
    double startIndex = 0.0; // start position of current line
    double endIndex = 0.0; // end position of current line
    double currentIndex = 0.0; // current location along line
    GeomPlanarGraphEdge currentEdge = null;
    private Color headingToHQ = Color.black;
    private Color headingToGoal = Color.red;
    int linkDirection = 1;
    public double speed = 0; // useful for graph
    ArrayList<GeomPlanarGraphDirectedEdge> pathFromHomeToWork =
        new ArrayList<GeomPlanarGraphDirectedEdge>();
    int indexOnPath = 0;
    int pathDirection = 1;
    public boolean reachedGoal = false;
    PointMoveTo pointMoveTo = new PointMoveTo();

    static private GeometryFactory fact = new GeometryFactory();

    /**
     * //////////////////////// Model Constructor ////////////////////////////////
     * Constructor: specifies parameters for Agents
     * Default Wrapper Constructor: provides the default parameters
     *
     * //@param location - Coordinate indicating the initial position of the Agent
     * //@param homeNode - Coordinate indicating the Agent's home location
     * //@param workNode - Coordinate indicating the Agent's workplace
     * //@param world - reference to the containing NorfolkRouting instance
     */
    public MainAgent(MK_7 g, String homeTract, GeomPlanarGraphEdge startingEdge,
            GeomPlanarGraphEdge goalEdge)   {
       world = g;

       // set up information about where the node is and where it's going
       homeNode = startingEdge.getDirEdge(0).getFromNode();
       workNode = goalEdge.getDirEdge(0).getToNode();
       this.homeTract = homeTract;
       this.goalTract = goalTract;

       // set the location to be displayed
       //GeometryFactory fact = new GeometryFactory();

       location = new MasonGeometry(fact.createPoint(new Coordinate(10, 10))) ;

       location.isMovable = true;

       // Now set up attributes for this agent
       if (g.random.nextBoolean())  {
           location.addStringAttribute("TYPE", "4x4");
           int age = (int) (20.0 + 2.0 * g.random.nextGaussian());
           location.addIntegerAttribute("AGE", age);
       }
       else {
           location.addStringAttribute("TYPE", "Car");
           int age = (int) (40.0 + 9.0 * g.random.nextGaussian());
           location.addIntegerAttribute("AGE", age);
       }

       // Not everyone moves at the same speed
       // Assigns random speed
       //moveRate *= Math.abs(g.random.nextGaussian());
       // Assigns random speed between 0-70
       moveRate = (int)(Math.random()*70) + 1;
       System. out.println("Agent's MoveRate = " + moveRate );
       location.addDoubleAttribute("MOVE RATE", moveRate);

       Coordinate startCoord = null;
       startCoord = homeNode.getCoordinate();
       updatePosition(startCoord);
    }

    //////////////////////////////////////////////////////////////////////////////
    //////////////////////////// AGENT ATTRIBUTES ////////////////////////////////
    //////////////////////////////////////////////////////////////////////////////

    /**
     * ////////////////////////// Agent Type /////////////////////////////////////
     * @return string indicating whether Agent is a "4x4" or a "Car"
     */
    public String getType() {
        return location.getStringAttribute("TYPE");
    }

    /**
    * ////////////////////////// Agent Colour ////////////////////////////////////
    * Want to change the colour of the Agent's depending on their status:
    * "heading back to HQ" or "heading to goal"
    *
    */

    public final void draw(Object object, Graphics2D graphics, DrawInfo2D info) {
       if( reachedGoal )
           graphics.setColor( headingToGoal );
       else
           graphics.setColor( headingToHQ );

       // this code was stolen from OvalPortrayal2D
       int x = (int)(info.draw.x - info.draw.width / 20.0);
       int y = (int)(info.draw.y - info.draw.height / 20.0);
       int width = (int)(info.draw.width);
       int height = (int)(info.draw.height);
       graphics.fillOval(x,y,width, height);
    }

    //////////////////////////////////////////////////////////////////////////////
    //////////////////////////////// ROUTING /////////////////////////////////////
    //////////////////////////////////////////////////////////////////////////////

    /**
    * ////////////////////////// A* Route Initialisation /////////////////////////
    * Initialization of an Agent: find an A* path to work!
    *
    * @param state
    * @return whether or not the agent successfully found a path to work
    */
   public boolean start(MK_7 state) {
       findNewAStarPath(state);
       if (pathFromHomeToWork.isEmpty())    {
           System.out.println("Initialization of a Agent (" +homeTract
                + ") failed: it is located in a part of the network that cannot "
                + "access the given goal.");
           return false;
       } else   {
           return true;
       }
   }

   /**
    * ////////////////////////// Plot A* Route ///////////////////////////////////
    * Plots a path between the Agent's home Node and its work Node
    */
   private void findNewAStarPath(MK_7 geoTest)  {

       // get the home and work Nodes with which this Agent is associated
       Node currentJunction = geoTest.network.findNode
               (location.geometry.getCoordinate());
       Node destinationJunction = workNode;

       if (currentJunction == null) {
           return; // just a check
       }
       // find the appropriate A* path between them
       AStar pathfinder = new AStar();
       ArrayList<GeomPlanarGraphDirectedEdge> path =
           pathfinder.astarPath(currentJunction, destinationJunction);

       // if the path works, lay it in
       if (path != null && path.size() > 0) {

           // save it
           pathFromHomeToWork = path;

           // set up how to traverse this first link
           GeomPlanarGraphEdge edge =
               (GeomPlanarGraphEdge) path.get(0).getEdge();
           setupEdge(edge);

           // update the current position for this link
           updatePosition(segment.extractPoint(currentIndex));

       }
   }

   double progress(double val)  {
       double edgeLength = currentEdge.getLine().getLength();
       double traffic = world.edgeTraffic.get(currentEdge).size();
       double factor = 1000 * edgeLength / (traffic * 5);
       factor = Math.min(1, factor);
       return val * linkDirection * factor;
   }

   /**
    * ////////////////////////// Step to Move Agent //////////////////////////////
    * Called every tick by the scheduler.
    * Moves the agent along the path.
    */
   public void step(SimState state) {
       // check that we've been placed on an Edge
       if (segment == null) {
           return;
       } // check that we haven't already reached our destination
       else if (reachedGoal)    {
           System.out.println(this + " has reached its HOME");
           flipPath();
       }

       // make sure that we're heading in the right direction
       //boolean toWork = ((MK_7) state).goToWork;
       // if ((toWork && pathDirection < 0) || (!toWork && pathDirection > 0))  {
       //     flipPath();
       // }

       // move along the current segment
       speed = progress(moveRate);
       currentIndex += speed;

       // check to see if the progress has taken the current index beyond its goal
       // given the direction of movement. If so, proceed to the next edge
       if (linkDirection == 1 && currentIndex > endIndex)   {
           Coordinate currentPos = segment.extractPoint(endIndex);
           updatePosition(currentPos);
           transitionToNextEdge(currentIndex - endIndex);
       } else if (linkDirection == -1 && currentIndex < startIndex) {
           Coordinate currentPos = segment.extractPoint(startIndex);
           updatePosition(currentPos);
           transitionToNextEdge(startIndex - currentIndex);
       } else
       { // just update the position!
           Coordinate currentPos = segment.extractPoint(currentIndex);

           updatePosition(currentPos);
       }
   }

   /**
    * ////////////////////////// Flip Agent's Route //////////////////////////////
    * Flip the agent's path around
    */
   public void flipPath()   {
       reachedGoal = false;
       pathDirection = -pathDirection;
       linkDirection = -linkDirection;
   }

   /**
    * ////////////////////////// Move Agent to Next Edge /////////////////////////
    * Transition to the next edge in the path
    * @param residualMove the amount of distance the agent can still travel
    * this turn
    */
   void transitionToNextEdge(double residualMove)   {

       // update the counter for where the index on the path is
       indexOnPath += pathDirection;

       // check to make sure the Agent has not reached the end
       // of the path already
       if ((pathDirection > 0 && indexOnPath >= pathFromHomeToWork.size())
               || (pathDirection < 0 && indexOnPath < 0))
                // depends on where you're going!
       {
           System.out.println(this + " has reached its DESTINATION");
           reachedGoal = true;
           indexOnPath -= pathDirection; // make sure index is correct
           return;
       }

       // move to the next edge in the path
       GeomPlanarGraphEdge edge = (GeomPlanarGraphEdge)
               pathFromHomeToWork.get(indexOnPath).getEdge();
       setupEdge(edge);
       speed = progress(residualMove);
       currentIndex += speed;

       // check to see if the progress has taken the current index beyond its goal
       // given the direction of movement. If so, proceed to the next edge
       if (linkDirection == 1 && currentIndex > endIndex)   {
           transitionToNextEdge(currentIndex - endIndex);
       } else if (linkDirection == -1 && currentIndex < startIndex) {
           transitionToNextEdge(startIndex - currentIndex);
       }
   }

   /**
    * ////////////////////////// Agent's Route Info //////////////////////////////
    * Sets the Agent up to proceed along an Edge
    * @param edge the GeomPlanarGraphEdge to traverse next
    */
   void setupEdge(GeomPlanarGraphEdge edge) {

       // clean up on old edge
       if (currentEdge != null) {
           ArrayList<MainAgent> traffic = world.edgeTraffic.get(currentEdge);
           traffic.remove(this);
       }
       currentEdge = edge;

       // update new edge traffic
       if (world.edgeTraffic.get(currentEdge) == null)  {
           world.edgeTraffic.put(currentEdge, new ArrayList<MainAgent>());
       }
       world.edgeTraffic.get(currentEdge).add(this);

       // set up the new segment and index info
       LineString line = edge.getLine();
       segment = new LengthIndexedLine(line);
       startIndex = segment.getStartIndex();
       endIndex = segment.getEndIndex();
       linkDirection = 1;

       // check to ensure that Agent is moving in the right direction
       double distanceToStart = line.getStartPoint().distance(location.geometry),
           distanceToEnd = line.getEndPoint().distance(location.geometry);
       if (distanceToStart <= distanceToEnd)    { // closer to start
           currentIndex = startIndex;
           linkDirection = 1;
       } else if (distanceToEnd < distanceToStart)  { // closer to end
           currentIndex = endIndex;
           linkDirection = -1;
       }
   }

   /**
    * ////////////////////////// Move Agent //////////////////////////////////////
    * Move the agent to the given coordinates
    */
   public void updatePosition(Coordinate c) {
       pointMoveTo.setCoordinate(c);
       // location.geometry.apply(pointMoveTo);

       world.agents.setGeometryLocation(location, pointMoveTo);
   }

   /**
    * ////////////////////////// Agent's Location ////////////////////////////////
    * Return geometry representing agent location
    */
   public MasonGeometry getGeometry()   {
       return location;
   }
}

AStar.java

package network;

import com.vividsolutions.jts.geom.Coordinate;
import com.vividsolutions.jts.planargraph.DirectedEdgeStar;
import com.vividsolutions.jts.planargraph.Node;
import sim.util.geo.GeomPlanarGraphDirectedEdge;

import java.util.ArrayList;
import java.util.HashMap;

/**
 * AStar.java
 *
 * Copyright 2011 by Sarah Wise, Mark Coletti, Andrew Crooks, and
 * George Mason University.
 *
 * Licensed under the Academic Free License version 3.0
 *
 * See the file "LICENSE" for more information
 *
 * $Id: AStar.java 842 2012-12-18 01:09:18Z mcoletti $
 */
public class AStar {

    //////////////////////////////////////////////////////////////////////////////
    /////////////////////////////// PARAMETERS ///////////////////////////////////
    //////////////////////////////////////////////////////////////////////////////
    public ArrayList<GeomPlanarGraphDirectedEdge> astarPath(Node start, Node goal)  {
        // initial check
        if (start == null || goal == null)  {
            System.out.println("Error: invalid node provided to AStar");
        }

        // set up the containers for the result
        ArrayList<GeomPlanarGraphDirectedEdge> result =
            new ArrayList<GeomPlanarGraphDirectedEdge>();

        // containers for the metainformation about the Nodes relative to the
        // A* search
        HashMap<Node, AStarNodeWrapper> foundNodes =
            new HashMap<Node, AStarNodeWrapper>();

        AStarNodeWrapper startNode = new AStarNodeWrapper(start);
        AStarNodeWrapper goalNode = new AStarNodeWrapper(goal);
        foundNodes.put(start, startNode);
        foundNodes.put(goal, goalNode);

        startNode.gx = 0;
        startNode.hx = heuristic(start, goal);
        startNode.fx = heuristic(start, goal);

        // A* containers: nodes to be investigated, nodes that have been investigated
        ArrayList<AStarNodeWrapper> closedSet = new ArrayList<AStarNodeWrapper>(),
            openSet = new ArrayList<AStarNodeWrapper>();
        openSet.add(startNode);


        while (openSet.size() > 0)  {
            // while there are reachable nodes to investigate
            AStarNodeWrapper x = findMin(openSet);
            // find the shortest path so far
            if (x.node == goal) {
                // we have found the shortest possible path to the goal!
                // Reconstruct the path and send it back.
                return reconstructPath(goalNode);
            }
            openSet.remove(x);
            // maintain the lists
            closedSet.add(x);

            // check all the edges out from this Node
            DirectedEdgeStar des = x.node.getOutEdges();
            for (Object o : des.getEdges().toArray())   {
                GeomPlanarGraphDirectedEdge l = (GeomPlanarGraphDirectedEdge) o;
                Node next = null;
                next = l.getToNode();

                // get the A* meta information about this Node
                AStarNodeWrapper nextNode;
                if (foundNodes.containsKey(next))   {
                    nextNode = foundNodes.get(next);
                } else  {
                    nextNode = new AStarNodeWrapper(next);
                    foundNodes.put(next, nextNode);
                }

                if (closedSet.contains(nextNode))   {
                    // it has already been considered   
                    continue;
                }

                // otherwise evaluate the cost of this node/edge combo
                double tentativeCost = x.gx + length(l);
                boolean better = false;

                if (!openSet.contains(nextNode))    {
                    openSet.add(nextNode);
                    nextNode.hx = heuristic(next, goal);
                    better = true;
                } else if (tentativeCost < nextNode.gx) {
                    better = true;
                }

                // store A* information about this promising candidate node
                if (better) {
                    nextNode.cameFrom = x;
                    nextNode.edgeFrom = l;
                    nextNode.gx = tentativeCost;
                    nextNode.fx = nextNode.gx + nextNode.hx;
                }
            }
        }
        return result;
    }

    /**
     * ///////////////////////////// Path Array //////////////////////////////////
     * Takes the information about the given node n and returns the path that
     * found it.
     * @param n the end point of the path
     * @return an ArrayList of GeomPlanarGraphDirectedEdges that lead from the
     * given Node to the Node from which the serach began
     */
    ArrayList<GeomPlanarGraphDirectedEdge> reconstructPath(AStarNodeWrapper n)  {
        ArrayList<GeomPlanarGraphDirectedEdge> result =
            new ArrayList<GeomPlanarGraphDirectedEdge>();
        AStarNodeWrapper x = n;
        while (x.cameFrom != null)  {
            result.add(0, x.edgeFrom); // add this edge to the front of the list
            x = x.cameFrom;
        }

        return result;
    }

    /**
     * /////////////////////////// Euclidean Distance ////////////////////////////
     * Measure of the estimated distance between two Nodes. Extremely basic, just
     * Euclidean distance as implemented here.
     * @param x
     * @param y
     * @return notional "distance" between the given nodes.
     */
    double heuristic(Node x, Node y)    {
        Coordinate xnode = x.getCoordinate();
        Coordinate ynode = y.getCoordinate();
        return Math.sqrt(Math.pow(xnode.x - ynode.x, 2)
            + Math.pow(xnode.y - ynode.y, 2));
    }

    /**
     * //////////////////////////// Road Length //////////////////////////////////
     * @param e
     * @return The length of an edge
     */
    double length(GeomPlanarGraphDirectedEdge e)    {
        Coordinate xnode = e.getFromNode().getCoordinate();
        Coordinate ynode = e.getToNode().getCoordinate();
        return Math.sqrt(Math.pow(xnode.x - ynode.x, 2)
            + Math.pow(xnode.y - ynode.y, 2));
    }

    /**
     *  //////////////////////// Nodes to Consider ///////////////////////////////
     *  Considers the list of Nodes open for consideration and returns the node
     *  with minimum fx value
     * @param set list of open Nodes
     * @return
     */
    AStarNodeWrapper findMin(ArrayList<AStarNodeWrapper> set)   {
        double min = 100000;
        AStarNodeWrapper minNode = null;
        for (AStarNodeWrapper n : set)  {
            if (n.fx < min) {
                min = n.fx;
                minNode = n;
            }
        }
        return minNode;
    }

    /**
     * 
     * /////////////////////////// A* Node Meta Info /////////////////////////////
     * A wrapper to contain the A* meta information about the Nodes
     *
     */
    class AStarNodeWrapper  {
        // the underlying Node associated with the metainformation
        Node node;
        // the Node from which this Node was most profitably linked
        AStarNodeWrapper cameFrom;
        // the edge by which this Node was discovered
        GeomPlanarGraphDirectedEdge edgeFrom;
        double gx, hx, fx;

        public AStarNodeWrapper(Node n) {
            node = n;
            gx = 0;
            hx = 0;
            fx = 0;
            cameFrom = null;
            edgeFrom = null;
        }
    }
}

Polygon.java

package sim;

import sim.util.geo.MasonGeometry;
import java.util.ArrayList;

/**
 * Polygon.java
 *
 * Copyright 2011 by Sarah Wise, Mark Coletti, Andrew Crooks, and
 * George Mason University.
 *
 * Licensed under the Academic Free License version 3.0
 *
 * See the file "LICENSE" for more information
 *
 * $Id: Polygon.java 842 2012-12-18 01:09:18Z mcoletti $
 */
public class Polygon extends MasonGeometry  {
    String soc;

    ArrayList<Polygon> neighbors;

    public Polygon()    {
        super();
        neighbors = new ArrayList<Polygon>();
    }

    public void init()  {
        soc = getStringAttribute("RC_RankCol");
    }

    String getSoc() {
        if (soc == null)    {
            init();
        }
        return soc;
    }
}
\$\endgroup\$
  • \$\begingroup\$ I'm struggling to make sense of your code and don't have the time look in depth. However I think you may be trying to do too much. You could look at something like Processing, processing.org to provide the underpinning so you can concentrate on the agents. \$\endgroup\$ – Jackson Mar 22 '18 at 19:57
  • \$\begingroup\$ @Jackson thanks for the comment. There is a lot of code and most of it relates back to MASON so it's difficult to get to grips with straight away. And I'm sure my code needs improving in terms of style and format. Yes, as I say in my post, I know I'm trying to do too much but I'm kinda stuck at the stage where I just have to do it now! Thanks for the processing.org link. I'll check it out. Any other hints or tips or a point to other resources or places where people might be able to help would be appreciated. Thanks again. \$\endgroup\$ – KJGarbutt Mar 23 '18 at 7:50
3
\$\begingroup\$

Dude... where to start. I even have a hard time to somehow group the issues. I'll just go line by line, I guess... oh and disclaimer: If sarcasm is found, keep it. And this will be kind of a code review I'd give my co workers. Welcome to the real world, I guess.

MK_7

Quite bad naming all over the code. What is MK_7 supposed to mean? Seventh level of Mushroom Kingdom? Quite a fancy easter egg, but keep those for a start up, not while doing your PhD, man.

Comments

This is the worst:

//////////////////////////////////////////////////////////////////////////////
///////////////////////////// MODEL PARAMETERS ///////////////////////////////
//////////////////////////////////////////////////////////////////////////////

This is just useless. It's not only useless, I have no idea what I have to do with that information. And it's wrong. The first line after that comment is the serialVersionUID. And it's not parameters. Just get rid of all those comments, it's just clutter and misleading.

serialVersionUID

Disable the eclipse warning for the serialVersionUID. And delete all of those uids. When I'm pair programming with my apprentices, I forbid them to use quick fix. Because they don't know what the error/warning is, and what the quick fix does.

public variables

Don't explose implementation detail: public GeomVectorField roads = new GeomVectorField();

See information hiding

another comment

Okay, I withdraw my previous statement, THIS is the worst:

/**
 * //////////////////////// Model Constructor ////////////////////////////////
 * Model Constructor
 */

JavaDoc WITH single line comments.

Develop against interfaces

Don't declare your variables with the implementation of the datastructure, use Map instead of HashMap. It shouldn't concern you what type of Map it is. The decision what map to use is taken once.

<> / Diamond operator

instead of

ArrayList<agents.MainAgent> agentList = new ArrayList<agents.MainAgent>();

write

ArrayList<agents.MainAgent> agentList = new ArrayList<>();

Commented out code

Get a code repository. And never ever check in commented code. If there's commented code, no one will ever know if it's important. If you need the history, you got it in your scm.

setup() method

The king of the worst gets a new contender:

/**
 * //////////////////////// OSVI Polygon Setup ///////////////////////////////
 * Polygon Setup
 */
void setup()

Just name the method polygonSetup and get rid of this comment.

And this one:

    // copy over the geometries into a list of Polygons

Well, why don't you name the method copyGeomtriesIntoPolygons? Beside that:

polys.addAll(world.getGeomtries())

is fine. (Don't overdo method chaining, though)

start() method

... You call it start, but the comment says "Model Initialisation". (TWICE, btw). Which one is correct?


    /*
    src/
        data/
            .shp, .dbf, .csv ...
    sim/
        MK_7/
            .java files
     */

First: WTF? (You know, the best measurement of code goodness is WTF's/minute.)

Second: Those files shouldn't be in src/data. It's not source code.


        URL roadsFile = MK_7.class.getResource
                ("/data/Final_ITN.shp");

... so your program only does work with shipped resources and there's no way to to use other data?


ShapeFileImporter.read(roadsFile, roads);

Be more clear about what you read. And if you read something into something, make the method name clear. And why does the start() method read data? Is it because the comment (Model Initialisation) is more correct than the method name?


System.out.println("    Roads shapefile: " +roadsFile);

Use a logging API.


Envelope MBR = roads.getMBR();

Code conventions: variables must not be upper case. See java code convetions


MBR.expandToInclude(world.getMBR());

Expand what to include what? Why does the "Envelope MBR" - which you got from roads - must be expanded with "world.getMBR()"?

// read in the FZ3 file

Extract the code, which does read in the FZ3 file in a separate method and call it readFZ3File. And get rid of the comment.


setup();

So, the start()/"Model Initialisation" method does call a setup method. WTF++


agentGoals("/data/AgentGoals.csv");

Method names should have a verb in it. And about what it does: It initializes agents and reads a csv file. The correct name would be readAgentGoalsAndInitialize. And if a name has an and in it's name, it usually does too much. I should only do one thing.


System.out.println();
System.out.println("Starting simulation...");

No reason not to use \n for a line feed...


roads.setMBR(MBR);

Why do you load all your mbrs, then to so much other stuff, and then set the mbr's the other types?


System.out.println("Error: missing required data file");

and

e.printStackTrace();

1st: Use a logging API. 2nd: Be consistent.

finish()

 * //////////////////////// Model Finish & Cleanup ///////////////////////////

Liar. The setup() method does the cleanup. At least the cleanup of the agents.

agentGoals()

Use try-with-resource block.


for (String data : splitted)
    agentGoalsResult.add(data);

I recommend to use brackets. I've never seen code guide lines where no-bracket-one-liner-thingies (What's it called?) are allowed.


populateAgent()

 * //////////////////////// Setup mainAgent //////////////////////////////////

... populate or setup? ...

 * Read in the population files and create appropriate populations

... or is it readPopulationFilesAndCreatePopulations?


Don't forget to close your resources


String homeTract = bits[3];

What's a home tractor?


String ROAD_ID = bits[3];

-> it's roadId_s_. camelCase, and it's plural.


Random randomiser = new Random();

Read the constructor JavaDoc of Random. If created within the same System.currentTimeMillis(), it will use the same seed. Beside that, I've seen a random variable somewhere, why not use this one?


String random = csvData.get(new Random().nextInt(csvData.size()));

Random what?

String goalTract = random;

Why declaring random in the first place?


for (int i = 0; i < pop; i++)   {

what's a pop?


agents.MainAgent a

What's a a? Similator to a b?


continue; // DON'T ADD IT if it's bad

DON'T ADD WHAT IF WHAT IS BAD?


newGeometry.isMovable = true;

As said before: Don't expose implementation detail.


d.close();

... it was an awful lot of scrolling to figure out what d is. Guess what it was? A BufferedReader of course. It's logical.


Iterator<?> nodeIterator

In my opinition, type safety is a good thing (I hate javascript). Use Iterator<Node> as parameter type.


@SuppressWarnings("unused")

... That's another example of eclipse quick fix. Dude, if it's not used, remove it. Don't suppress the warning.


Oh and why are you declaring coord (Coordinate of what, btw?) and point (Point of what, btw?) not within the while loop?

main() method

doLoop(MK_7.class, args);

... the code of doLoop isn't posted, but passing a class looks quite suspicious to me.


System.exit(0);

Dude, that's the default exit code...

MK_7WithUI.java

Holy smokes, it doesn't stop.

Well, MK_7WithUI indicates, that the type will do to much, the word "with" usually indicates that...


/////////////////////////// DISPLAY FUNCTIONS ////////////////////////////////

Well, we call it methods in Java


 * ///////////////////////// Default constructor /////////////////////////////

Liar. It's not the default constructor. See default constructor.

setupPortrayals()

sim.MK_7 world = (sim.MK_7) state;

I don't get it: Why is a state a world?


display.reset();

setting portrayals up shouldn't reset the display

main() method

Usually, the main method is at the bottom of a type.


Console c = new Console(simple);

Hm, what Console? The console-console? Why is he passing that mk7 with ui thingie?

c.setVisible(true);

WAIT WHAT?

Well, the Console code is not posted, but if that thing actually is a JFrame or something, you're gonne make me really angry.

getSimulationInspectedObject()

*  This must be included to have model tab, which allows mid-simulation
*  modification of the coefficients

You should have seen the look on my face. Why must what be included to have what to allow something which somehow has nothing to do with the method name?

Why does it return Object?

Why is there an explicit comment about non-volatile?

Everything.. is just.. so scary

start()

     * Called when starting a new run of the simulation. Sets up the portrayals
     * and chart data.

Ooooh I somehow have the feeling this is a lying comment. And yes it is. It also does remove series from the traffic chart, does some fancy speed modification thingie and schedlung of some sort and calculation of average speeds (oh and has JAVADOC WITHIN METHODS JESUS CHRIST), and does somehow setup the gui.


double maxS = 0, minS = 10000, avgS = 0, count = 0;

Please don't do that. Declare variables line by line. Btw, what s? In maths and physics, it's usually called velocity.


if (a.reachedGoal)  {
    continue;
}

Reverse the condition and remove the continue. And I don't know,... I usually cringe when I see continue. Reminds me of my assembler lessons somehow. Don't know why, I try to forget about it.


 avgS += speed;

... Well, actually it's not the average speed. The average will be calculated after the foor loop.


double time = state.schedule.time();

Should be getTime(). Time() reads as "stop the time" or "time something".


    roadsPortrayal.setPortrayalForAll(new GeomPortrayal(Color.DARK_GRAY, 0.0005, false));

What's 0.0005? What's false?

PolyPortrayal

Is there any reason to use inner classes?

draw()

Switch statement also works on String (since JDk 1.7 I think)

MainAgent

Dude, I have to scroll to see all the variables. That type is too long. glances of and checks the scrollbar. It's WAY too long.


private double basemoveRate = 10.0;
private double moveRate = basemoveRate;

basemoveRate is never used again. Beside that, it should be a constant: private static final BASE_MOVE_RATE = 10.0;


private LengthIndexedLine segment = null;

That's like Point wire = null;

How on in earth would I know that a wire is a point, when I see it seven million lines later in the code?


static private GeometryFactory fact = new GeometryFactory();

Same here. Later:

location = new MasonGeometry(fact.createPoint(new Coordinate(10, 10))) ;

How would I know, that fact is not a fact, but a factory?


// Now set up attributes for this agent

"Once upon a time" (Ok, maybe I'm getting a bit silly now)


//////////////////////////// AGENT ATTRIBUTES ////////////////////////////////

... and the first thing after that comment is a getter ...


getType()

Why on earth it's the MainAgent's job to handle the car type anyway? And why is the car type stored in a location.

findNewAStarPath()

   if (currentJunction == null) {
       return; // just a check
   }

Yeah, just a check, but jumping out of the method.

progress()

Progessing what? What's val? What does it return? Uses CTRL+F THE VELOCITY? WHAT?

step()

currentIndex += speed;

What has an index to do with speed?


if (linkDirection == 1 && currentIndex > endIndex)   {

Why 1? Why must the currentIndex be larger than the index to perform that code?


{ // just update the position!

Were you angry writing this piece of code?

flipPath()

/**
 * ////////////////////////// Flip Agent's Route //////////////////////////////
 * Flip the agent's path around
 */

Flip path, flip route, revert pathDirection, revert linkDirection. It's like a thesaurus in here.

AStar & Polygon

Okay, I think I have to stop here now...

Guidance

Alright. It took me so long, I had to take TWO cigarette brakes.

Look, it's a quite complicated topic you got yourself into. Now, you have a complicated topic, and complicated code. It's the duty of a software developer, to write clean code, so a reader can focus on the topic. Ok?

Clean Code

Use the correct indentations. Format your code. Don't leave commented code. Get rid of "all" comments and make your code speak for itself. Use the common code conventions.

Read the book "Clean Code" by Robert C. Martin. There should be a free pdf version. I really, really recommend that book. Many of my points in this review are also found in the book and discussed in great detail.

Another thing: Good code, or clean code, is hard to measure. But if someone else opens it and thinks stuff like "Oh, that's nice", it's good. My favorite: "That code is sexy". You need to entitle yourself to that.

OOP

You have a butt ton of code spread out in a few classes. You really should read about oop principles. There's so many priniples and patterns which could be applied to your code. (Of course, OOP is not about splitting code into classes just to have smaller classes.) You might want to read about SOLID principles and about high cohesion and lose coupling. But from my experience, just reading about it is not too helpful.

Getting better

About producing "good code", I learned the must by doing pair programming and code reviews with good, passionate and experienced programmers. Yes, this site is called code review, but, you know, face to face communication is the best communication. Now, the fundamental difference between code review and pair programm - well, at least from an educational perspective - is, that a code review is more about finding bugs, smaller details like "this can be a static method", "there's a type", "I find that too complex". Pair programming on the other hand is more about "the process of thinking" and "decisions". If you do a code review, and you see, that the other dude for instance puts "x and y" in a type Point. You don't judge that. But if you do pair programming, he will tell you, that they "belong together".

Working code

If you finished your work, push it up to your scm, and you're thinking "The code works, I'm done.", I have bad news: That's only half of your work. In reality, often you will read more code than you write. If it takes you two days to read and analyze clean code, it can easily take you four days to read bad code. And in reality, that's a poop ton of money wasted.

Tests

You didn't post any unit tests, and I'm fairly certain, that there are none. You know why? Because it's not testable. I highly recommend to read books about unit testing, especially test driven development. There's people who like it, there's people who don't. But what it will do, it will produce testable design. Which is quite better than a non testable design.

Now, why are tests so important? Well, if you have horrible code, which needs refactoring, and you can not write unit tests, because it's not testable, how do you figure out, if it still works? I have seen classes with thousands of lines and dozens of inner classes. I wanted to refactor it. But I didn't. Because it was so bloody complicated, everything had dependencies to everyone, everyone to everything. And if you have code which is about money, in the sense of: If you break it, your customer will lose money, and you are not 100% sure it works, you're so freaking scared, you just leave it.

Brutally honest

If you'd present that piece of code in an interview, even fresh from university, you won't get the job. If you'd have a job and are developing in our project and would push code like that into our repository, it will be back-outed immediately. Then we'd do a lot of pair programming ;-)

Hope this helps,

slowy

\$\endgroup\$
  • \$\begingroup\$ Thank you! I'm going to go through all your comments and spend the day figuring out how to improve all of this. A lot (most!) of the code is inherited and I've just been hacking it together and I've learned Java from these MASON examples, so it looks like I've learned some really bad habits. Thanks again. \$\endgroup\$ – KJGarbutt Mar 25 '18 at 9:32
  • \$\begingroup\$ I just wrote a line by line apology/explanation/question/answer comment, but of course it's way over the character limit. @slowy, if you would like to read it, it's here. Thank you for your time and patience. It was a huge help and a great wake up call. I would love to get more help and advice from you, but appreciate if that is not possible. \$\endgroup\$ – KJGarbutt Mar 26 '18 at 14:02

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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