6
\$\begingroup\$

enter image description here

I am simulating a simple bus network (as shown in the picture) using event simulation. There are two buses running (starting at 1 and 4) and the travel time between nodes is 10 units. For now, I used the switch-cases to manage the event. However, if I need to expand the network, it might be very confusing to manage all the events with switch-cases technique.

Is there any solution to improve this code?

#include <iostream>
#include <math.h>
#include <stdlib.h>
#include <queue>
#include <time.h>
#include <functional>
#include <vector>
#include <algorithm>


using namespace std;

double Clock, Travel_time_bw_block;


class Event
{


public:

enum EvtType {arrival_1, arrival_2, arrival_3, arrival_4, arrival_5, arrival_6};             

    Event(EvtType type = arrival_1, int bus_no = 0, double etime = 0.0):
_type(type),_bus_no(bus_no), _etime(etime) {}

    EvtType get_type() const
    {
        return _type;
    }
    double get_bus_no() const
    {
        return _bus_no;
    }
    double get_time() const
    {
        return _etime;
    }


protected:
    EvtType _type;
    int _bus_no;
    double _etime;
};



struct EventLess
{
    bool operator()(const Event& lhs, const Event& rhs) const
    {
        return (lhs.get_time() > rhs.get_time());
    }
};


priority_queue <Event, vector<Event>, EventLess> FutureEventList;


void Initialization()
{
    // initialize global variables
    Clock = 0.0;

    Travel_time_bw_block = 10.0;

    Event evt1(Event::arrival_1, 1, 0.0);
    Event evt2(Event::arrival_4, 2, 0.0);


    FutureEventList.push(evt1);
    FutureEventList.push(evt2);
}

int main(){

    int Simulation_Time = 200;
    Initialization();

    while(Clock < Simulation_Time){

        Event evt = FutureEventList.top();

        FutureEventList.pop();
        Clock = evt.get_time();

        switch(evt.get_type()){

            case Event::arrival_1:
            {
                cout << "bus " << evt.get_bus_no() << " arriving block 1 at: " << Clock << endl;
                Event enter_1 = Event(Event::arrival_2, evt.get_bus_no(), Clock + Travel_time_bw_block);
                FutureEventList.push(enter_1);
                break;
            }
            case Event::arrival_2:
            {
                cout << "bus " << evt.get_bus_no() << " arriving block 2 at: " << Clock << endl;
                Event enter_2 = Event(Event::arrival_3, evt.get_bus_no(), Clock + Travel_time_bw_block);
                FutureEventList.push(enter_2);
                break;
            }
            case Event::arrival_3:
            {
                cout << "bus " << evt.get_bus_no() << " arriving block 3 at: " << Clock << endl;
                Event enter_3 = Event(Event::arrival_4, evt.get_bus_no(), Clock + Travel_time_bw_block);
                FutureEventList.push(enter_3);
                break;
            }

            case Event::arrival_4:
            {
                cout << "bus " << evt.get_bus_no() << " arriving block 4 at: " << Clock << endl;
                Event enter_4 = Event(Event::arrival_5, evt.get_bus_no(), Clock + Travel_time_bw_block);
                FutureEventList.push(enter_4);
                break;
            }

            case Event::arrival_5:
            {
                cout << "bus " << evt.get_bus_no() << " arriving block 5 at: " << Clock << endl;
                Event enter_5 = Event(Event::arrival_6, evt.get_bus_no(), Clock + Travel_time_bw_block);
                FutureEventList.push(enter_5);
                break;
            }

            case Event::arrival_6:
            {
                cout << "bus " << evt.get_bus_no() << " arriving block 6 at: " << Clock << endl;
                Event enter_6 = Event(Event::arrival_1, evt.get_bus_no(), Clock + Travel_time_bw_block);
                FutureEventList.push(enter_6);
                break;
            }

        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ If you have a long switch statement, replace it with a std::map. Simply you can reduce the size of your code. And your code will be more cleaner. \$\endgroup\$ – Tharindu Kumara Dec 20 '16 at 17:54
1
\$\begingroup\$

Here are some observations that may help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Avoid the use of global variables

I see that Clock is used only within main (and Initialization) but it's declared as s global variable. It's generally better to explicitly pass variables your function will need or declare them within the appropriately smallest possible scope rather than using the vague implicit linkage of a global variable. I'd recommend gathering these into a BusSim class instead.

Use standard templates

The priority queue is currently declared like this:

priority_queue <Event, vector<Event>, EventLess> FutureEventList;

This is very strange for several reasons. First EventLess is defined as a struct:

struct EventLess
{
    bool operator()(const Event& lhs, const Event& rhs) const
    {
        return (lhs.get_time() > rhs.get_time());
    }
};

Second, it has "less" in the name but uses ">" in the code. It would be an understatement to describe this as counterintuitive! Instead, I'd recommend creating a more typical operator function for the Event class:

bool operator>(const Event& lhs, const Event& rhs) 
{
    return lhs.get_time() > rhs.get_time();
}

Then declare the priority_queue like this:

std::priority_queue <Event, std::vector<Event>, std::greater<Event> > FutureEventList;

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Use only required #includes

The code has a number of #includes that are not needed. This clutters the code and makes it more difficult to read and understand. Only include files that are actually needed. In this code, I believe these are the only ones required:

#include <iostream>
#include <queue>
#include <functional>
#include <vector>

Rethink your class design

Your comment about this design being confusing to manage is likely to be true the way it's currently written. Instead, I think I'd design things a little differently. In particular, I think I'd have a Bus object for each bus. Each Bus would contain its own collection of BusStops. Then I would have the whole thing encapsulated in a BusSim class. If that is done, the main could look like this:

int main(){
    BusSim sim(200);
    sim();
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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