Code Review

Code Review

MTA challenge

In this series, I will be taking a project, or a few exercises I have completed the previous week and reviewing the code. I will be trying to look for improvements in efficiency, tidiness and readability. Today's code will be for a weekend homework task called MTA. This challenge has been mentioned since day one of the course, so I took a very basic approach and stuck with the fundamentals to get a working project. In this, I am going to look at the program function by function and see if it can be streamlined using more efficient methods and fix a few flaws I see in the code.

The task was as below, and my initial solution is found on Gist:

MTA Lab
Objectives:
Apply your knowledge of Javascript to solve a real-world problem.
Get really good at array manipulation.
Activity
Create a program that models a simple subway system.

The program takes the line and stop that a user is getting on at and the line and stop that the user is getting off at and prints the journey and the total number of stops for the trip in the console:

planTrip('N', 'Times Square', '6', '33rd'); // This is only a suggested function name and signature.

- console.log() shows output similar to this:
- "You must travel through the following stops on the N line: 34th, 28th, 23rd, Union Square."
- "Change at Union Square."
- "Your journey continues through the following stops: 23rd, 28th, 33rd."
- "7 stops in total."
There are 3 subway lines:
The N line has the following stops: Times Square, 34th, 28th, 23rd, Union Square, and 8th
The L line has the following stops: 8th, 6th, Union Square, 3rd, and 1st
The 6 line has the following stops: Grand Central, 33rd, 28th, 23rd, Union Square, and Astor Place.
All 3 subway lines intersect at Union Square, but there are no other intersection points. (For example, this means the 28th stop on the N line is different than the 28th street stop on the 6 line, so you'll have to differentiate this when you name your stops in the arrays.)
Tell the user the number of stops AND the stops IN ORDER that they will pass through or change at.
Hints:
Work out how you would do it on paper first! Then start to explain that process in Javascript.
Get the program to work for a single line before trying to tackle multiple lines.
Don't worry about prompting the user for input. Hard code some values to get it working. You can use prompt() later to make it more interactive.
Consider diagramming the lines by sketching out the subway lines and their stops and intersection.
The key to the lab is finding the index positions of each stop. (hint: indexOf())
Make sure the stops that are the same for different lines have different names (i.e. 23rd on the N and on the 6 need to be differentiated)

First thing I did was use a factory to create 3 arrays of objects for the lines.

function stationFactory(line,name){
    return {
        name: name,
        line:line
    }
}

At this stage I am happy with this fulfilling it's purpose without any need to be changed, but depending on later edits this may need to be tweaked to solve some other problems in the code. This brings me to the first major problem I have with this solution. The code is very brittle.

function tripadvisor(onLine,onStop,offLine,offStop){
    const on = findLine(onLine,onStop) // returns array with line and array index for stop
    const off = findLine(offLine,offStop)// returns array with line and array index for stop    
    if(onLine===offLine){
        const stops = legBuilder(on, off) //returns array of stops
        displayStops(stops);
    }else{
        const legOneOff = findLine(onLine,'Union Square') // returns array with line and array index for stop
        const legTwoOn = findLine(offLine,'Union Square') // returns array with line and array index for stop
        // console.log(legOneOff)
        const stops1 = legBuilder(on, legOneOff);//returns array of stops
        stops1[stops1.length-1]+=` ** change to ${offLine} line here**`; //directions to change and what line to change to
        const stops2 = legBuilder(legTwoOn, off);//returns array of stops
        const stops = stops1.concat(stops2) // joins arrays
        displayStops(stops);
    }
}

This is the main body of my solution. It takes the supplied inputs, find their locations in the arrays and decides whether or not it will be a single leg trip, and then displays the trip information. The brittleness comes from how I handled the change, I have hardwired in the Union Square change. To fix this I will adjust the line arrays to include a boolean for any stations that are an intersection point, and the lines they intersect with.

function stationFactory(line,name,intersection = false,changeLine ){
    return {
        name: name,
        line:line,
        intersection: intersection,
        changeLines: changeLine
    }
}

    stationFactory('N','Union Square', true, ['L','six']),

The flag will automatically be set to false to save editing a lot of inputs, and changeLine is now an optional array of other lines that intersect it. In the main body I made the following change:

    const legOneOff = findChange(on,offLine);
    //    const legOneOff = findLine(onLine,'Union Square') // returns array with line and array index for stop

This now calls a new function: function findChange(on,offLine){ const line = on[0]; const index = line.findIndex(object => { // this section finds the array //index of the first object with a true intersection value return object.intersection === true; }); const change=line[index].changeLines; console.log(change.includes(offLine)); if(line[index].changeLines.includes(offLine)){ //checks you can change //to the correct line const changeInfo= [on[0],index]; const changeName = on[0][changeInfo[1]].name;//collects the station // name for input as the on value for the second leg console.log(changeName); changeInfo[2]=changeName; return changeInfo; } } In this new function I am using an indexOf() method with an arrow function to locate an intersection and return the line array, index number for the change and the station name. This makes a brittle piece of code more flexible.

The findLine() function ran a simple switch function below.

function findLine(line,stop){ // selects the appropriate array
    let station
    switch (line){
        case 'N': 
            station = findStop(N,stop);
            break;
        case 'L':
            station = findStop(L,stop);
            break;
        case '6':
            station = findStop(six,stop);
            break;
    }
    return station
}

This can be simplified further by removing the need for the station variable at all, instead directly returning the findStop() functions as below:

function findLine(line,stop){ // selects the appropriate array
    switch (line){
        case 'N': 
           return findStop(N,stop);
        case 'L':
            return findStop(L,stop);
        case '6':
            return findStop(six,stop);
    }
}

The findStop() function is being used to find and return the index number of the stop. At the start of writing this blog I had intended to test the indexOf() method to see if I could streamline this code. After using it in findChange() I am happy to leave this as is. Arrow functions have not been covered in the course yet, and my current method seems much simpler. The suggestion in the task to use indexOf() may have been assuming that we would store the lines as arrays of strings, not objects, or I simply don't enough understanding of arrow functions to use them effectively yet.

function findStop(line,stop){
    for (let i=0;i<line.length;i++){
        if(line[i].name===stop){
            const station = [line, i];
            // console.log('stop found')
            return station;
        }
    }
}

I will address the following three functions together:

function legBuilder(on,off){ // makes info more easily accessible and chooses a direction
    const arr = on[0];
    const start = on[1];
    const finish = off[1];
    // console.log(arr,start,finish)
    if(on>off){
        stops = backwards(arr,start,finish);
    }else{
        stops = forwards(arr,start,finish);
    }
    return stops;
}

function backwards(arr,on,off){ //loops through the line from point to point
    let stops = []
    for( let i = on-1;i>=off;i--){
        stops.push(arr[i].name);
    }
    return stops;
}

function forwards(arr,on,off){ //loops through the line from point to point
    let stops = []
    for( let i = on+1;i<=off;i++){
        stops.push(arr[i].name);
    }
    return stops;
}

Here I addressed forward and backward separately, I had hoped to utilise the reverse() method instead of requiring 3 seperate functions however, after some experimentation I found that reversing arrays messed around with the indexes I had already calculated. In the second iteration of this task later in the course I will keep this in mind from the start and hopefully design the solution to handle this better.

The only other functions in this solution are to display the result, which has little to discuss. The second being 4 prompt()'s that are used to take input. I have not handled data verification for these prompts, and assume that the user is able to input the stations correctly. Rather than implement data verification I would design a button based UI to remove user error but this is beyond the current scope of the task.

Overall thoughts

Decisions I made early, such as making stations objects, have limited the methods I could utilise to easily streamline the solution. On the next iteration I will start with the intention of utilising indexOf() and reverse() to cut down on some code, or maybe I will have learned new and better ways to handle the data by that stage. I am happy with the final solution I have, it works, and gives the required information.