Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

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

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

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

explained why > in "less" is a bad idea
Source Link
Edward
  • 66.6k
  • 4
  • 118
  • 282

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:

Second, it has "less" in the name but uses ">" in the code! Instead, I'd recommend creating a more typical operator function for the Event class:

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:

Source Link
Edward
  • 66.6k
  • 4
  • 118
  • 282

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! 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();
}