Sort and describe itinerary segments

Question

Given some incoming some data in JSON format, representing unsorted train, bus, and flight tickets:

[{'from':'A', to:'B',...}, {'from':'C', to:'D',...}, {'from':'B', to:'C',...}];

… I must sort tickets (A-B-B-C-C-D) and generate text (description) with all information about each trip (number of flight, gate, seat - it comes with JSON objects). But with some requirements:

• it must be "API format". extensible code (should provide way to add information about different type of transport).
• clean and easy code.

I want to be honest with you, imagine that I'm junior in your team, and that I write for real project.

Here is my solution.

var tickets = [
    {
        from: "London",
        to: "Paris",
        transportType: "train",
        transport: {
            "number": "54S",
            "seat": "23"
        }
    },
    {
        from: "Berlin",
        to: "Akrich",
        transportType: "bus",
        transport: {
            "number": "SR465",
            "seat": "56",
            "gate": "2A",
        }
    },
    {
        from: "Paris",
        to: "Berlin",
        transportType: "flight",
        transport: {
            "number": "SR465",
            "seat": "56",
            "gate": "2A",
            "baggage": "will be automatically transferred from your last leg"
        }
    },
    {
        from: "Moscow",
        to: "London",
        transportType: "flight",
        transport: {
            "number": "N554",
            "seat": "4A",
            "gate": "22",
            "baggage": "drop at ticket counter 344"
        }
    }
];


function TripSorter ( tickets ) {
    this.from = {};
    this.to = {};

    this.path = [];
    this.tickets = [];

    if ( tickets ) {
        this.importTickets( tickets );
    }
}

// copy data in this.tickets. 
// create "from" ( contains only "from" points of route ) and "to" objects (hashMap);
TripSorter.prototype.importTickets = function ( tickets ) {
    var self = this;
    self.tickets = [];

    if ( tickets instanceof Array ) {
        self.tickets = tickets;

        tickets.forEach( function( e, i ) {

           if ( !e.from || e.from === ' ' ) {
               console.error('card.from - is missing on ' + JSON.stringify(e, undefined, 2));
           } else if ( !e.to || e.to === ' ' ) {
               console.error('card.to - is missing on ' + JSON.stringify(e, undefined, 2));
           } else {
               self.from[e.from] = i;
               self.to[e.to] = i;
           }
            
        });       

    }
};

// start of trip - unique key in obj from compared to obj to.
TripSorter.prototype.getStartTrip = function () {
    var i = 0,
        start;

    for (  ; i < this.tickets.length; i++ ) {
        if( this.to[this.tickets[i].from] == null ) {
            start = this.tickets[i].from;
            break;
        }
    }

    return start;
};

// obj "from" and "to" contains equal value for different keys
// ticket {from: A, to: B} => var from = { A : 0 }, to = { B : 0};
TripSorter.prototype.buildTrip = function () {
    var start = this.getStartTrip(),
        point = this.from[start],
        next,
        i = 0;

    for (  ; i < this.tickets.length; i++ ) {
        if ( !this.tickets[point] ) continue;
        next = this.tickets[point].to;
        this.path.push(this.tickets[this.to[next]]);
        point = this.from[next];
    }
};

TripSorter.prototype.getTrip = function () {
    var text = [];
    this.path.forEach( function( card ) {
        switch (card.transportType) {
            case 'flight':
                text.push( new Flight(card).message );
            break;
            case 'train':
                text.push( new Train(card).message );
            break;
            case 'bus':
                text.push( new Bus(card).message);
            break;
            default: break;
        };
    });
    return text;
};

var trip = new TripSorter(tickets);
trip.buildTrip();
var route = trip.getTrip();

console.log(route);

//Generate trip description. 
function Transport (data) {
    this.from = data.from;
    this.to = data.to;
    this.type = data.transportType;

    this.place = 'from ' + this.from + ' to ' + this.to; // ?

    this.seat = (data.transport.seat) ? ' Seat ' + data.transport.seat + '.' : ' No seat assignment.';
    this.gate = (data.transport.gate) ? ' Geat ' + data.transport.gate + '.' : '';

    this.number = (data.transport.number) ? ' ' + data.transport.number + ' ' : ' ';
    this.baggage = ' Baggage' + data.transport.baggage || '';
}    

function Train(data) {
    Transport.call(this, data);
    this.message = 'Take train' + this.number + this.place + '.' + this.seat;
    return this.m;
}
Train.prototype = Object.create(Transport.prototype);
Train.prototype.constructor = Train;

function Flight(data) {
    Transport.call(this, data);

    this.number = 'take flight' + this.number; // ?
    this.place = 'From ' + this.from + ',' + this.number + 'to ' + this.to + '.'; // ?

    this.message = this.place + this.gate + this.seat + this.baggage;
    return this.m;
}
Flight.prototype = Object.create(Transport.prototype);
Flight.prototype.constructor = Flight;

function Bus(data) {
    Transport.call(this, data);
    this.message = 'Take the bus' + this.number  + this.place + '.' + this.seat;
    return this.m;
}
Bus.prototype = Object.create(Transport.prototype);
Bus.prototype.constructor = Bus;

Is it ok that I used:

• function constructor and Prototype instead of usual function ?
• different types of array loop — for and forEach in one constructor?

How I should organize code inside of this file (where to place "transport constructor" and where "TripSorter")?

Is it correct to use Transport constructor and inherit from it or I must used switch(type of transport) - case (message + )? What way is easy to extend?

I must add some functionality to print generated text to html - is it ok that I put it inside of TripSorter prototype or its better to use different function?

I used console.error to handle error of ticket format — is it correct way?

Are the comments normal?

Answer 1

Organization

How I should organize code inside of this file (where to place "transport constructor" and where "TripSorter")?

Declare things before using them. Declare dependencies first.

• Function declarations and class definitions (constructor functions) go to the top, example data and code to the bottom.
• TripSorter instantiates and holds Transport instances, so declare Transport first.

Inheritance

Is it correct to use Transport constructor and inherit from it or I must used switch(type of transport) - case (message + )? What way is easy to extend?

Inheritance is an extensible approach. However, it requires some overhead and you need to declare some kind of factory function - here: getTrip. This function features a switch statement. In JavaScript, it is often advised to replace switch statements with maps, usually object literals.

Also, your Transport objects currently don't have any methods. They even don't hold any meaningful data apart from one message attribute. Once the constructor function returns an instance, you just use it to retrieve the message.

Additionally, by pushing subclass functionality which is shared by some but not all subclasses - e.g. initialization of gate and baggage - upwards to the parent Transport constructor, you attempt to reduce code repetition at the cost of clear semantics and maintainable code:

let train = new Train({
  from: "Beijing",
  to: "Tianjin",
  transport: {number: "CR120"}
});
// Why do trains have a gate and baggage attribute?
console.log(train); // Object { type: undefined, gate: "", baggage: "...", ... }

You redundantly store type information in the type attribute. However, your Transport instances already have type information attached to them:

train instanceof Train // true

I suggest you ditch inheritance and the additional overhead by directly mapping transport types to functions (which are objects, too) which return descriptions:

const descriptors = {
  "walk": (from, to) => `Walk from ${from} to ${to}.`,
  ...
}
descriptors["walk"]("Berlin", "Hamburg"); // Walk from Berlin to Hamburg.

Constructor functions

Don't return anything from a constructor function. You currently return this.m in Train, Flight and so on. Luckily, this.m is undefined - and when returning undefined from a constructor function, the new operator returns the newly created object instance.

API design

Your TripSorter is stateful and its methods must be called in a correct order. Calling importTickets(tickets) after already having supplied tickets to the constructor results in unexpected behavior. Calling getTrip before buildTrip is meaningless. Et cetera. I don't see any good reason to split one indivisible 'sort tickets' functionality into several public methods.

Print results

I must add some functionality to print generated text to html - is it ok that I put it inside of TripSorter prototype or its better to use different function?

So far, you defined some models and business logic. You should not mix those with presentation logic - this should be part of your 'view' layer. By keeping those parts of your application separate, you can test and reuse them much easier.

Error handling

I used console.error to handle error of ticket format — is it correct way?

Your TripSorter.prototype.importTicket methods logs errors to the console when the from or to attribute of a ticket is missing. However, it still resumes operations and returns the first connected sub-trip, ignoring other connected segments.

Also, you do explicitly check for single-space strings but still accept multi-space strings:

if ( !e.from || e.from === ' ' ) { ... }

You also don't check for cycles and you don't validate the transportType and transport attribute. In some cases, you simply ignore the incorrect ticket and continue silently, sometimes you throw a TypeError and sometimes, you log an error to the console. All in all, your input validation and error handling feels incomplete and random.

I suggest you write robust code that expects valid tickets but still returns consistent results for 'invalid' attributes. Your TripSorter cannot possibly validate from and to attributes, as the knowledge about valid city names is part of another business unit in your code. You could / should therefore validate tickets beforehand and e.g. encapsulate them in a class that doesn't allow construction of invalid tickets. Fail early and document clearly how and when you fail:

// @throws {Error} when itinerary is ambiguous or circular.

Comments

Are the comments normal?

Your comments mainly focus on implementation details:

// start of trip - unique key in obj from compared to obj to.
TripSorter.prototype.getStartTrip = function () { ... }

However, I recommend more 'documentation style' comments that tell the users of your functions, classes and modules about their purpose, the expected inputs and return values. Following an established format such as ESDoc that is understood by many also allows you to auto-generate your documentation:

/**
 * Get the city where the trip starts.
 *
 * @return {String} city.
 */
TripSorter.prototype.getStartTrip = function () { ... }

Local variables

Declare and initialize loop iterators within the loop initializer. Keep variables as local as possible. let is more local than var. Instead of

var foo,
    i = 0;

for (  ; i < length; i++ ) {
  foo = ...
}

I suggest you write

for ( let i = 0; i < length; i++ ) {
  let foo = ...
}

Functional vs. declarative

Better don't mix functional style iterations

 this.path.forEach( function( card ) { ... }

and for-loops

 for (  ; i < this.tickets.length; i++ ) { ... }

needlessly, unless it improves readability.

Naming

Name your generic function arguments:

 tickets.forEach( function( e, i ) { ... } );       // what is e?
 tickets.forEach( function( element, i ) { ... } ); // it is an element...
 tickets.forEach( function( ticket, i ) { ... } );  // it is a ticket!

Avoid misleading names - is this really a 'place'?

this.place = 'From ' + this.from + ',' + this.number + 'to ' + this.to + '.';

Most of your constructors feature a single generic, nondescript data argument. Instead of

function Transport (data) {
   this.from = data.from;
   this.to = data.to;
   ...
}

I suggest the self-documenting

function Transport ({from, to} = data) {
    this.from = from;
    this.to = to;
    ...
}

or simply

function Transport (from, to) {
    this.from = from;
    this.to = to;
    ...
}

Strict type checking

Don't check argument types:

if ( tickets instanceof Array ) { ... }

Use comments to state the expected type or interface. By removing above type check and writing

for (let ticket of tickets) { ... }

you can handle any iterable tickets argument. It also doesn't fail silently but throws a TypeError: ... is not iterable if needed.

Weak comparison

Be cautious of weak comparisons:

if( this.to[this.tickets[i].from] == null )

Accessing an inexistent key from this.to actually returns undefined. And luckily for you, undefined == null evaluates to true.

Other issues

I don't think this is correct as the + operator has higher precedence than ||:

this.baggage = ' Baggage' + data.transport.baggage || '';

Suggested alternative implementation

Applying above suggestions to your code:

/**
 * Return tickets in connecting order with successively matching
 * to - from attributes.
 *
 * @param {Object[]} tickets - unordered tickets.
 * @return {Object[]} connecting tickets.
 * @throws {Error} when itinerary is ambiguous or circular.
 */
function findItinerary(tickets) {
  let from = new Map(tickets.map(ticket => [ticket.from, ticket]));
  let to = new Map(tickets.map(ticket => [ticket.to, ticket]));
  let start = tickets.find(ticket => !to.has(ticket.from));
  
  if (!start) {
    throw Error(`Circular itinerary`);
  }
  
  let itinerary = [];
  for (let ticket = start; ticket; ticket = from.get(ticket.to)) {
    itinerary.push(ticket);
  }
  
  if (itinerary.length < tickets.length) {
    throw Error(`Ambiguous itinerary`);
  }
  
  return itinerary;
}

/**
 * Map tickets to descriptions returned by matching descriptor functions
 * based on the ticket transport type.
 *
 * @param {Object[]} tickets - connecting tickets.
 * @param {Object} descriptors - mapping transport types to descriptor functions.
 * @return {String[]} descriptions.
 * @throws {Error} when a transport type has no matching descriptor.
 */
function describeItinerary(tickets, descriptors) {
  return tickets.map(ticket => {
    let descriptor = descriptors[ticket.transportType];
    if (descriptor) {
      return descriptor(ticket.from, ticket.to, ticket.transport);
    } else {
      throw Error(`Missing descriptor for transport type ${ticket.transportType}`);
    }
  });
}

// Extensible ticket descriptors:
const descriptors = {
  "flight": function(from, to, {number, gate, seat, baggage} = transport) {
    return `From ${from}, take flight ${number} to ${to}.`
      + `${gate ? ` Gate ${gate}.` : ``}`
      + `${seat ? ` Seat ${seat}`  : ` No seat assignment`}.`   
      + ` Baggage ${baggage}`;
  },
  "bus": function(from, to, {number, seat, gate} = transport) {
    return `Take the bus ${number} from ${from} to ${to}.`
      + `${gate ? ` Gate ${gate}.` : ``}`
      + `${seat ? ` Seat ${seat}`  : ` No seat assignment`}.`;
  },
  "train": function(from, to, {number, seat} = transport) {
    return `Take train ${number} from ${from} to ${to}.`
      + `${seat ? ` Seat ${seat}`  : ` No seat assignment`}.`;
  }
};

// Example tickets:
let tickets = [
  {
    from: "London",
    to: "Paris",
    transportType: "train",
    transport: {
      "number": "54S",
      "seat": "23"
    }
  },
  {
    from: "Berlin",
    to: "Akrich",
    transportType: "bus",
    transport: {
      "number": "SR465",
      "seat": "56",
      "gate": "2A"
    }
  },
  {
    from: "Paris",
    to: "Berlin",
    transportType: "flight",
    transport: {
      "number": "SR465",
      "seat": "56",
      "gate": "2A",
      "baggage": "will be automatically transferred from your last leg"
    }
  },
  {
    from: "Moscow",
    to: "London",
    transportType: "flight",
    transport: {
      "number": "N554",
      "seat": "4A",
      "gate": "22",
      "baggage": "drop at ticket counter 344"
    }
  }
];

// Find itinerary and describe it:
let itinerary = findItinerary(tickets);
let description = describeItinerary(itinerary, descriptors);

console.log(description);