Help fine tuning my code :)

[marcoakis]

Hello there!

I'm new programming in C++, have been learning it for 2-3 months.

Now, this uni assignment is working properly, but I'd like to read your advice on how to improve it.

All comments are appreciated.

Thanks!

#include <iostream>
#include <fstream>
#include <string>
using namespace std;


const int MAX_ARRAY_SIZE = 50;

struct Property {
    string address;
    string sub;
    int price;
};

void readFile(Property properties[], int &numOfProperties);
void listAll(Property properties[], int items);
void search(Property properties[], int items);
void addProperty(Property properties[], int &items);
void saveProperties(Property properties[], int items);
void listPropertiesSorted(Property properties[], int items);

int main()
{
    int numOfProperties = 0;
    int userChoice;
    Property properties[MAX_ARRAY_SIZE];
    /readFile(properties, numOfProperties);

    do {
	cout << "CPProperty - Menu:" << endl;
	cout << "1 - List all properties" << endl;
	cout << "2 - Property search" << endl;
	cout << "3 - Add a property" << endl;
	cout << "4 - Save property list" << endl;
	cout << "5 - Exit" << endl;
	cout << "6 - List properties sorted" << endl << endl;
	cout << "Please enter option: ";
	cin >> userChoice;
	cout << endl;

	switch (userChoice) {
	case 1:
	    listAll(properties, numOfProperties);
	    break;
	case 2:
	    search(properties, numOfProperties);
	    break;
	case 3:
	    addProperty(properties, numOfProperties);
	    break;
	case 4:
	    saveProperties(properties, numOfProperties);
	    break;
	case 5:
	    cout << "Thank you for using CPProperty" << endl;
	    break;
	case 6:
	    listPropertiesSorted(properties, numOfProperties);
	    break;
	default:
	    cout << "Illegal menu option" << endl;
	    break;
	}
    } while (userChoice != 5);

    system("pause");
    return 0;
}

void readFile(Property properties[], int &numOfProperties)
{
    ifstream fileIn;
    string fileName;
    string address, sub;
    string Endline;
    cout << "Wellcome to CPProperty" << endl;
    cout << "Enter property filename : ";
    cin >> fileName;

    numOfProperties = 0;
    fileIn.open(fileName.c_str(), ios::in);
    if (fileIn.is_open() == false)
	cout << "File \"" << fileName << "\" does not exist" << endl << endl;
    else {
	while (!fileIn.eof() && numOfProperties < MAX_ARRAY_SIZE) {

	    getline(fileIn, properties[numOfProperties].address);
	    getline(fileIn, properties[numOfProperties].sub);
	    fileIn >> properties[numOfProperties].price;
	    fileIn.ignore(INT_MAX, '\n');
	    numOfProperties++;
	}
	cout << "test ... numOfProperties = " << numOfProperties << endl;

	fileIn.close();
    }
    cout << endl;
}
void listAll(Property properties[], int items)
{
    if (items == 0)
	cout << "No properties in database" << endl;
    else
	for (int i = 0; i < items; i++)
	    cout << "$" << properties[i].price << " " << properties[i].address << ", " << properties[i].sub << endl;	//display the list
    cout << endl;
}

void search(Property properties[], int items)
{
    if (items == 0)
	cout << "No properties in database" << endl << endl;
    else {
	int priceLimit;
	cout << "Please enter price limit: $";
	cin >> priceLimit;
	cout << endl;

	for (int i = 0; i < items; i++)
	    if (properties[i].price <= priceLimit)
		cout << "$" << properties[i].price << " " << properties[i].address << ", " << properties[i].sub << endl;
	cout << endl;
    }
}

void addProperty(Property properties[], int &items)
{
    if (items < MAX_ARRAY_SIZE) {
	do {
	    cout << "Enter address: ";
	    getline(cin, properties[items].address, '\n');

	    if (properties[items].address == "")
		cout << "Invalid address." << endl;

	} while (properties[items].address == "");

	do {
	    cout << "Enter suburb: ";
	    getline(cin, properties[items].sub, '\n');

	    if (properties[items].sub == "")
		cout << "Invalid suburb." << endl;

	} while (properties[items].sub == "");

	do {
	    cout << "Enter price: $";
	    cin >> properties[items].price;

	    if (properties[items].price <= 0)
		cout << "Invalid price." << endl;

	} while (properties[items].price <= 0);

	cout << endl << "Property: " << properties[items].address << ", " << properties[items].sub << ", listed at $" << properties[items].price << " added." << endl << endl;

	items++;
    } else
	cout << "No room for more properties." << endl << endl;
}

void saveProperties(Property properties[], int items)
{
    ofstream fileOut;
    string fileName;
    cout << "Enter filename :";
    cin >> fileName;
    fileOut.open(fileName.c_str(), ios::out);
    if (fileOut.is_open() == false)
	cout << "The disk is full or write-protectde" << endl;
    else {
	for (int i = 0; i < items; i++)
	    fileOut << properties[i].address << endl << properties[i].sub << endl << properties[i].price << endl;
	fileOut.close();

	cout << "Properties details saved to file \"" << fileName << "\"" << endl << endl;
    }
}

void listPropertiesSorted(Property properties[], int items)
{
    if (items == 0)		//check for empty database
	cout << "No properties in database" << endl;
    else {
	int x = 0;
	int maxSub = items - 1;
	int lastSwap = 0;
	char swap = 'Y';
	Property temp;
	Property properties_copy[MAX_ARRAY_SIZE];
	for (int i = 0; i < items; i++) {
	    properties_copy[i].address = properties[i].address;
	    properties_copy[i].sub = properties[i].sub;
	    properties_copy[i].price = properties[i].price;
	}
	while (swap == 'Y') {
	    swap = 'N';
	    x = 0;

	    while (x < maxSub) {
		if (properties_copy[x].price > properties_copy[x + 1].price) {
		    temp.address = properties_copy[x].address;
		    temp.sub = properties_copy[x].sub;
		    temp.price = properties_copy[x].price;
		    properties_copy[x].address = properties_copy[x + 1].address;
		    properties_copy[x].sub = properties_copy[x + 1].sub;
		    properties_copy[x].price = properties_copy[x + 1].price;
		    properties_copy[x + 1].address = temp.address;
		    properties_copy[x + 1].sub = temp.sub;
		    properties_copy[x + 1].price = temp.price;
		    swap = 'Y';
		    lastSwap = x;
		}
		x++;
	    }
	    maxSub = lastSwap;
	}
	for (int i = 0; i < items; i++)
	    cout << "$" << properties_copy[i].price << " " << properties_copy[i].address << ", " << properties_copy[i].sub << endl;
	cout << endl;
    }
}

[NathanOliver]

in your sort function you should be able to do

// ...
while (x < maxSub) 
{
    if (properties_copy[x].price > properties_copy[x + 1].price) 
    {
        temp = properties_copy[x];
        properties_copy[x] = properties_copy[x + 1];
        properties_copy[x + 1] = temp;
        //...
    }
//...

[mrnutty]

No that you have hardcoded the work, I would use stl to lessen the work I have to do. That means, more efficient code, safer, readability, and shorter code.

For example, your listPropertiesSorted could be something like this :

bool propertiesCompare(const Property& lhs, const Property& rhs){
 /* compare properties here */
}
void printProperty(const Property& p){
 /* print property code here */
}
void listPropertiesSorted(Property properties[],const int items){
 std::sort(properties,properties + items, propertiesCompare);
 std::for_each(properties,properties + items, printProperty);
}