2
\$\begingroup\$

I did an exercise of a train travel planner. But it seems like a lot of code to do a simple thing? Is there a better way of writing the JavaScript?

Overview: The planner needs to be able to find connecting station between origin and destination, then print out the traveling line. Also state how many stops there are.

Please see this JS fiddle containing my code for this exercise.

HTML:

<form id="trip-planner-form">
  <label for="origin">Origin</label>
  <select name="origin" id="origin"></select>
  <label for="destination">Destination</label>
  <select name="destination" id="destination"></select>
  <button type="submit">Check</button>
</form>

<div id="planner-result"></div>

JS Code:

const data = [{
  id: 'alamein',
  label: 'Alamein',
  stops: [
    'Flinders Street',
    'Richmond',
    'East Richmond',
    'Burnley',
    'Hawthorn',
    'Glenferrie'
  ]
}, {
  id: 'glen-waverly',
  label: 'Glen Waverly',
  stops: [
    'Flagstaff',
    'Melbourne Central',
    'Parliament',
    'Richmond',
    'Kooyong and Tooronga'
  ]
}, {
  id: 'sandringham',
  label: 'Sandringham',
  stops: [
    'Southern Cross',
    'Richmond',
    'South Yarra',
    'Prahran',
    'Windsor'
  ]
}];

const form_submit = document.getElementById('trip-planner-form');
const planner_result = document.getElementById('planner-result');
const origin_select = document.getElementById('origin');
const destination_select = document.getElementById('destination');


DOMReady();


function DOMReady() {
  console.log('dom ready');
  loadStops(getStops(data));
  set_planner_form_events();
}

function getStops(lines) {
  return lines.reduce((accum, current) => {
      return accum.concat(current.stops);
    }, [])
    // remove duplicate
    .filter((f, index, arr) => {
      return arr.indexOf(f) === index;
    }).sort();
}

function loadStops(stops) {

  stops.forEach(stop => {
    var stop_option = document.createElement('option');
    stop_option.value = stop;
    stop_option.textContent = stop;
    // Append
    origin_select.appendChild(stop_option);
    destination_select.appendChild(stop_option.cloneNode(true));
  });
}

function set_planner_form_events() {
  form_submit.addEventListener('submit', event => {
    event.preventDefault();
    print_planner_result();
  });
}


function print_planner_result() {
  var origin = origin_select.value;
  var destination = destination_select.value;
  var result = '';

  if (origin === destination) {
    result = 'You selected the same station.';
  } else {
    let traveling_stops = get_traveling_stops(origin, destination);

    console.log(traveling_stops);

    if (traveling_stops) {
      result = `<p>${traveling_stops.concat().join(' --> ')}</p>`;
      result += `<p>total: ${traveling_stops.length} stops </p>`;
    } else {
      result = 'Something went wrong.'
    }

  }

  planner_result.innerHTML = result;
}


function get_traveling_stops(origin, destination) {
  // Find origin and destination index
  var origin_line_index = data.findIndex(d => d.stops.indexOf(origin) > -1);
  var dest_Line_index = data.findIndex(d => d.stops.indexOf(destination) > -1);

  if (origin_line_index === -1 || dest_Line_index === -1) {
    return false;
  }

  // If in same line
  if (origin_line_index === dest_Line_index) {
    let the_stops = data[origin_line_index].stops;
    let origin_stop_index = the_stops.indexOf(origin);
    let dest_stop_index = the_stops.indexOf(destination);

    if (origin_stop_index < dest_stop_index) {
      return the_stops.slice(
        origin_stop_index,
        dest_stop_index + 1,
      );
    } else {
      return the_stops.slice(
        dest_stop_index,
        origin_stop_index,
      ).reverse();
    }

  }

  // Not in same line
  if (origin_line_index !== dest_Line_index) {
    let connecting_stop = get_connecting_station(
      data[origin_line_index],
      data[dest_Line_index]
    );

    let origin_line_stops = data[origin_line_index].stops;
    let dest_line_stops = data[dest_Line_index].stops;
    let origin_stop_index = origin_line_stops.indexOf(origin);
    let origin_connect_index = origin_line_stops.indexOf(connecting_stop);
    let dest_stop_index = dest_line_stops.indexOf(destination);
    let dest_connect_index = dest_line_stops.indexOf(connecting_stop);
    let stops1, stops2;

    if (origin_stop_index < origin_connect_index) {
      stops1 = origin_line_stops.slice(
        origin_stop_index,
        origin_connect_index + 1
      );
    } else {
      stops1 = origin_line_stops.slice(
        origin_connect_index,
        origin_stop_index
      );
      stops1.push(origin);
      stops1.reverse();
    }

    if (dest_stop_index < dest_connect_index) {
      stops2 = dest_line_stops.slice(
        dest_stop_index,
        dest_connect_index + 1
      ).reverse();
      stops2.shift();
    } else {
      stops2 = dest_line_stops.slice(
        dest_connect_index + 1,
        dest_stop_index
      ).reverse();
      stops2.push(destination);
    }


    return stops1.concat(stops2);
  }


}


function get_connecting_station(line1, line2) {

  for (var i = 0; i < line1.stops.length; i++) {
    let stop_index = line2.stops.indexOf(line1.stops[i]);
    if (stop_index > -1) {
      return line1.stops[i];
    }
  }

  return false;
}
\$\endgroup\$
3
  • \$\begingroup\$ I don't think your planner works as intended. Given lines A-B, B-C, C-D, D-E, the plan for traveling from A to E is "A --> D --> E"? \$\endgroup\$
    – le_m
    Jan 4, 2018 at 14:28
  • \$\begingroup\$ Did you assume that all destinations are at maximum one change of line away from any origin? Also, could it be that for Burnley -> Windsor, the stops Prahran and South Yarra are switched in the resulting travel plan? \$\endgroup\$
    – le_m
    Jan 4, 2018 at 14:36
  • \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to give it a different title if there is something more appropriate. \$\endgroup\$ Jan 4, 2018 at 17:02

1 Answer 1

-2
\$\begingroup\$

Simple solution

The problem is trivial as there is only one hub station. This means that there is only one root between any two stations.

You need only find which line each station is on. If not the same line then find the root on starting line to the hub and from the destination station hub to the destination station.

If the problem involved a configuration that allowed more than one root between stations it becomes much more involved but your code does not show that is an issue.

Simplify using functions.

The reason why your code seams long is because you have not compartmentalised functionality in the form of functions.

You can break it down to

  • Finding a line for a station
  • Getting stops between stations on a line
  • Finding a root between stations
  • Displaying a root.

By breaking the problem into smaller parts you reduce the overall complexity and make it easier to read and maintain.

function findLineForStation(station) {
    return data.find(line => line.stops.includes(station));
}

function getStops(line, fromStation ,toStation, skipFirst = false) {
    const idxFrom = line.stops.indexOf(fromStation);
    const idxTo = line.stops.indexOf(toStation);
    const count = Math.abs(idxTo - idxFrom);
    const dir = Math.sign(idxTo - idxFrom);
    const stops = [];
    var i;
    for (i = skipFirst ? 1 : 0; i <= count; i ++) {
        stops.push(line.stops[idxFrom + i * dir]);
    }
    return stops;
}
function findRoot(fromStation, toStation) {
    const hubStation = "Richmond";
    const fromLine = findLineForStation(fromStation);
    const toLine = findLineForStation(toStation);
    if (fromStation === hubStation) { fromLine = toLine }
    if (fromLine === toLine) {
        return getStops(fromLine, fromStation, toStation);
    }
    return [
        ...getStops(fromLine, fromStation, hubStation),
        ...getStops(toLine, hubStation, toStation, true)
    ];
}
function displayRoot() {
    root.textContent = findRoot(originElId.value, destinationElId.value).join(" => ");
}

originElId.addEventListener("change",displayRoot);
destinationElId.addEventListener("change",displayRoot);
const data = [{
  id: 'alamein',
  label: 'Alamein',
  stops: ['Flinders Street', 'Richmond', 'East Richmond', 'Burnley', 'Hawthorn', 'Glenferrie']
}, {
  id: 'glen-waverly',
  label: 'Glen Waverly',
  stops: ['Flagstaff', 'Melbourne Central', 'Parliament', 'Richmond', 'Kooyong and Tooronga']
}, {
  id: 'sandringham',
  label: 'Sandringham',
  stops: ['Southern Cross', 'Richmond', 'South Yarra', 'Prahran', 'Windsor']
}];
From <select name="origin" id="originElId"><option value="Burnley">Burnley</option><option value="East Richmond">East Richmond</option><option value="Flagstaff">Flagstaff</option><option value="Flinders Street">Flinders Street</option><option value="Glenferrie">Glenferrie</option><option value="Hawthorn">Hawthorn</option><option value="Kooyong and Tooronga">Kooyong and Tooronga</option><option value="Melbourne Central">Melbourne Central</option><option value="Parliament">Parliament</option><option value="Prahran">Prahran</option><option value="Richmond">Richmond</option><option value="South Yarra">South Yarra</option><option value="Southern Cross">Southern Cross</option><option value="Windsor">Windsor</option></select><br>
Destination <select name="destination" id="destinationElId"><option value="Burnley">Burnley</option><option value="East Richmond">East Richmond</option><option value="Flagstaff">Flagstaff</option><option value="Flinders Street">Flinders Street</option><option value="Glenferrie">Glenferrie</option><option value="Hawthorn">Hawthorn</option><option value="Kooyong and Tooronga">Kooyong and Tooronga</option><option value="Melbourne Central">Melbourne Central</option><option value="Parliament">Parliament</option><option value="Prahran">Prahran</option><option value="Richmond">Richmond</option><option value="South Yarra">South Yarra</option><option value="Southern Cross">Southern Cross</option><option value="Windsor">Windsor</option></select>
<div id="root"></div>

\$\endgroup\$
1
  • \$\begingroup\$ thanks a lot! This is pretty awesome. IDK why it was downvoted. I liked the use of Math.sign to solve the directional problem and use of Array.find() and Array.include() ... I should've used those! Im just gonna leave open for a bit to see if there are more different solutions :) \$\endgroup\$
    – Ray Tsai
    Jan 4, 2018 at 22:41

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.