Eliminate Redundant Code

[GhettoBurger]

I apologize if the title seems a bit to vague, could't think of title that would work with the problem at hand.

I'm currently creating a Railway system as part of my assignment, one of the requirements is to allow staff members to set opening and closing hours of railway.

Throughout the program I ask the staff to select a railway, wheather its for adding new stations,deleting new station or anything else, in this case its for setting operation hours. So I created a function that would constantly call for the selection instead of rewriting it several times. (code below)

#include "stdafx.h"
#include "Railway.h"
#include "Staff.h"


using namespace std;

Railway::Railway()
{
	cout << "Please select one of the following options.\n";
	cout << "1) Selangor Railway \n2) Malacca Railway \n3) Negeri Sembilan Railway \n4) Exit";

	int chooseRailway;
	cin >> chooseRailway;

	switch (chooseRailway)
	{
	case 1:
		SelangorRailway();
		break;

	case 2:
		MalaccaRailway();
		break;

	case 3:
		NegeriSembilanRailway();
		break;

	default:
		//TODO ExitSystem
		break;
	}
}

void Railway::SelangorRailway()
{
	cout << "You have selected \"Selangor Railway.\"\n";
}

void Railway::MalaccaRailway()
{
	cout << "You have selected \"Malacca Railway.\"\n";
}

void Railway::NegeriSembilanRailway()
{
	cout << "You have select \"Negeri Sembilan Railway.\"\n";
}

I would like to somehow link it to another cpp file I have. (code below)

void Staff::ManageOperationHours()
{
	cout << "You have selected \"Manage Operation Hours.\"\n\n";
	Railway();
	
	cout << "Input operation hours.\n\n";
	
	ofstream OutputFile;
	OutputFile.open("OperationHours.txt");

	string OpeningHours, ClosingHourse;
	
	cout << "Enter the Opening Time of the Station: ";
	cin >> OpeningHours;
	OutputFile << OpeningHours << endl;

	cout << "Enter the Closing Time of the Station: ";
	cin >> ClosingHourse;
	OutputFile << ClosingHourse << endl;

	OutputFile.close();
	cout << "Succesfully Registered!\n";
	
}

In order to create something like the code below (code below does not work, its simply used to give idea as to what I am trying to achieve).


if(SelangorRailway == chosen)
{
// execute the code above, into its own txt file selangr.txt
}
else if (MalaccaRailway == chosen)
{
// execute the code above, into its own txt file selangr.txt
}
else if (NegeriSembilanRailway == chosen)
{
// execute the code above, into its own txt file selangr.txt
}

Ultimately, I would like to use the same code but I dont think that will be possible seeing that the different railways will have different txt files.

[jonnin]

You have a design problem, I think.
It looks to me like railway class should represent ONE railway, and you need either 3 variables of its type or a vector(3) and an enum maybe to store it all.

I would do it this way..

enum
{
SelangorRailway ,
MalaccaRailway ,
NegeriSembilanRailway,
max_railway
};

main
...

vector<railway> railz(max_railway); //you can insert more rails just by adding to the enum!!!
...
railz[SelangorRailway].ManageOperationHours();

^^^^ and here is the key, each railway calls its own version of that function, using its own class object members, one of which might be the file name, etc, so each call would write to its own file, etc.

Does this make sense?

You can fix what you have, by adding parameters to ManageOperationHours, but that is still a mess because the parameters have to be outside the class because the class is trying to represent all 3 instances of data at once.

[GhettoBurger]

Yes, its makes sense to a degree. I'm attempting to rewrite the code the way you suggested, I've never used enums before, did some research on its and its a pretty valuable tool. But I have a problem specifically with implementing this part.

railz[SelangorRailway].ManageOperationHours();


The method I have used to call function from other cpp files thus far is to call a constructor (not sure if the phrasing for the above sentence is correct code below will help understand my point).

My main simple has one line of code.

int main()
{
Login();
}

Once Login has been called it execute the code below.

using namespace std;

Login::Login()
{
	cout << "Welcome, please select one of the following options." << endl;
	cout << "1) Staff \n2) User \n3) Exit" << endl;

	int choose;
	cin >> choose;

	if (choose == 1)
	{
		 isLogin(); 
	}	
	else if (choose == 2)
	{
		cout << "hey"; // TODO redirect to User HomeScreen
	}
	else if (choose == 3)
	{
		exit(0);
	}
	else
	{
		cout << "Invalid choice, se";  // TODO set option to try again 
	}
}


void Login::isLogin()
{
	
	string username;
	string password;


	cout << "Enter Username: ";
	cin >> username;
	cout << "Enter Password: ";
	cin >> password;


	if (username == "admin" && password == "admin")
	{
		cout << "Login Successful\n\n";

		Staff();
		
	}
	else if (username != "admin" || password != "admin")
	{
		cout << "Username or Password is incorrect, please try again.\n\n"; // TODO add option to try again 

	}
}

The function isLogin then calls another constructor Railway::Railway. (Railway is the code above)

Staff::Staff()
{
	cout << "Welcome, to proceed select one of the following options.\n";
	cout << "1) Manage Railway Operation Hours \n2) Manage Station\n3) Manage Ticket Prices \n4) Scedule & Monitor Routes \n5) Return \n";

	int select;
	cin >> select;

	switch (select)
	{
	case 1:
		ManageOperationHours();
		break;

	case 2:
		ManageStations();
		break;

	case 3:
		ManageTickets();
		break;

	case 4:
		ManageScheduleAndRoutes();
		break;
		
	default:
		//Login();
		break;
	}
}

void Staff::ManageOperationHours()
{
	cout << "You have selected \"Manage Operation Hours.\"\n\n";
	Railway();
	
	cout << "Input operation hours.\n\n";
	
	ofstream OutputFile;
	OutputFile.open("OperationHours.txt");

	string OpeningHours, ClosingHourse;
	
	cout << "Enter the Opening Time of the Station: ";
	cin >> OpeningHours;
	OutputFile << OpeningHours << endl;

	cout << "Enter the Closing Time of the Station: ";
	cin >> ClosingHourse;
	OutputFile << ClosingHourse << endl;

	OutputFile.close();
	cout << "Succesfully Registered!\n";
	
}

I've created multiple cpp files and header files to make the code as clear as possible and I feel that I've somehow crippled myself and cant shake this feeling that further down the line I'll run into plenty of problem using this approach. Additionally, I cant rely on high level coding techniques as I am still a beginner and due to the fact that submission is in a week. (Felt that I should write that).

So going back to the main problem I cannot call .ManageOperationHours from Staff.cpp or Main.cpp. For some reason I can only call function that have no return type (constructors), I dont quite understand why this is.

[jonnin]

are you saying that your constructor, which calls ManageOperationHours, is not working?

Or are you trying to call ManageOperationHours directly somewhere *outside* the class?

It can only be called from inside the staff class, like it is in your constructor, or from a variable instance:

staff x;
x.ManageOperationHours(); //legal in main, or outside the class.

And, it can only be called outside the class if it is made
public:

it is private and untouchable by default.

[dhayden]

It's very good that you've recognized that there's something wrong with your design. Believe it or not, that's one sign of a good programmer.

It's not clear to me that you need a Railway class at all, but let's assume that you do for now. I'd use an enum like jonnin, but I'd create just one Railway object. A railway object stores the type of railway that it is. Then it's easy to add a method that returns the name of the output file that it should use.

Right now you write the opening/closing hours directly to a file. Should they be members of the railway class instead?

Separate your user interface code from your computation code. That means a separate function to prompt for the type of railway, followed by a single line of code to create the object.

This shows what I mean. Note that I've changed some method names just to make this sample compile.

#include <iostream>
#include <string>
#include <fstream>


enum RailwayType {
    Selangor,
    Malacca,
    NegeriSembilan,
    max_railway			// MUST BE LAST
};

class Railway {
public:
    Railway(RailwayType t) {
	type = t;
    }

    RailwayType type;
    std::string outFile() {
	switch (type) {
	case Selangor:
	    return "selangr.txt";
	case Malacca:
	    return "macala.txt";
	case NegeriSembilan:
	    return "negerisembilan.txt";
	default:
	    std::cout << "bad value " << int(type) << "in Railway::outfile()\n";
	    return "";
	}
    }
};
    

using std::cout;
using std::cin;
using std::string;
using std::ofstream;
using std::endl;


void ManageOperationHours(Railway &rail)
{
	cout << "You have selected \"Manage Operation Hours.\"\n\n";
	
	// Get the input hours
	cout << "Input operation hours.\n\n";
	
	string OpeningHours, ClosingHourse;
	cout << "Enter the Opening Time of the Station: ";
	cin >> OpeningHours;

	cout << "Enter the Closing Time of the Station: ";
	cin >> ClosingHourse;

	// Write them out
	ofstream OutputFile;
	OutputFile.open(rail.outFile());
	
	OutputFile << OpeningHours << endl;
	OutputFile << ClosingHourse << endl;

	OutputFile.close();
	cout << "Succesfully Registered!\n";
}

int main()
{
	cout << "Please select one of the following options.\n";
	cout << "1) Selangor Railway \n2) Malacca Railway \n3) Negeri Sembilan Railway \n4) Exit";

	unsigned chooseRailway;
	cin >> chooseRailway;

	if (chooseRailway >= int(max_railway)) {
	    cout << "Railway must be between 0 and " << int(max_railway)-1 << '\n';
	    return 0;
	}

	Railway rail(RailwayType(chooseRailway));
}

If possible, post the full assignment and your full code. If it's too big them feel free to PM it to me, but I'll only be able to help for a few more hours today. I can pick it up again tomorrow.