[gareth.nic]

1st year at varsity and in our C++ class we got this small program to code. Lecturer gave one look at it and said I was totally wrong.

Here's my code:

#include <iostream>

using namespace std;

int main()
{
	int num1, num2, num3, num4;

	cout <<"Enter number 1: ";
	cin >> num1;
	cout <<"Enter number 2: ";
	cin >> num2;
	cout <<"Enter number 3: ";
	cin >> num3;
	cout <<"Enter number 4: ";
	cin >> num4;

	if ((num1 > num2) && (num1 > num3) && (num1 > num4))
	{
		cout << num1 << " is bigger." << endl;
	}
	else if ((num2 > num1) && (num2 > num3) && (num2 > num4))
	{
		cout << num2 << " is bigger." << endl;
	}
	else if ((num3 > num1) && (num3 > num2) && (num3 > num4))
	{
		cout << num3 << " is bigger." << endl;
	}
	else
	{
		cout << num4 << " is bigger" << endl;
	}

	system("PAUSE");
	return 0;
}

He says instead of all those outputs, I should have a variable that keeps track of the biggest number. Then when the application is done checking, show the output.

I tried to get my point of view across to him, he basically said "it's my way or the high way." Then he spoke of efficiency etc. But my program was shorter than his...

Which way is better for these mini programs? Straight to output or via variable?

[JackOfAllTrades]

He's right. For one thing a shorter program doesn't necessarily reflect efficiency, and writing programs with easy to follow logic is a much better choice. He's trying to set you on the path of good habits, and that's a good thing for which I applaud him.

Try to write your code with an eye toward the person who will be maintaining it in the future. I know that doesn't make sense at the university homework level, but it will when you enter the workforce.

The right solution to this problem is actually reading the data into an array, sorting it, and selecting the top array index, but I'm guessing at the point of this assignment you hadn't reached that. Bet you'll get there relatively soon.

[Bryston]

Practical C++ from O'Reilly Press has some good information about coding styles including writing code that can be maintained by others as JackOfAllTrades suggests. Highly recommend it for that chapter if nothing else.

[gareth.nic]

Thanks guys.

Good to know. I just have to adjust my entire way of coding. But as you say, it's for the good.

Was hoping to prove him wrong. For some reason this guy seems really smug when I get something wrong.

[Bench]

gareth.nic, on 18 February 2012 - 01:39 PM, said:

I tried to get my point of view across to him, he basically said "it's my way or the high way." Then he spoke of efficiency etc. But my program was shorter than his...

Most commonly the reason for writing programs in a particular way is to make the program easier to read, debug, maintain, extend and usually easier to write as well. The length/size of the program has absolutely no bearing on any of these things; A better measure would be one of complexity. I.e. if you have to sit and stare at a bit of code in order to understand what it does, then there's room to make it simpler. (in your original example, each of your "if" statements contain several different comparisons - this is a common example of complexity which can be cut down with a little careful thought)


In small programs, it can often be difficult to see how certain advices apply in general - but programming is a discipline, meaning that every single little habit you pick up while you're learning is going to make all the difference when you come to write a really big, complicated project (And it can make the difference between giving you a clear headstart or leading yourself down a dark blind alley).

So, if you're not sure why your tutor suggests doing something in a particular way, then its worth probing for an example of what can go wrong - usually the answer will come down "most programmers are likely to make mistakes when they do it like this" or "Other programmers are likely to make mistakes when they get their hands dirty with your code".
(Unfortunately sometimes tutors seem to ask their students to do really bizarre things which they wouldn't do in the real world, but hopefully your tutor isn't one of those.)

[Karel-Lodewijk]

Basically the reason this is poor code is because it doesn't scale at all. Sure you can write this for 4 numbers but try to adapt the code so it looks for the largest of 100 numbers.

So, I would start out with a constant at the top const int NR_OF_NUMBERS = 5; and write my code in such a way that if I change 5 to any other arbitrary number that it will still work.

This post has been edited by Karel-Lodewijk: 19 February 2012 - 09:01 AM

[Karel-Lodewijk]

Bench, on 18 February 2012 - 02:47 PM, said:

Most commonly the reason for writing programs in a particular way is to make the program easier to read, debug, maintain, extend and usually easier to write as well. The length/size of the program has absolutely no bearing on any of these things; A better measure would be one of complexity. I.e. if you have to sit and stare at a bit of code in order to understand what it does, then there's room to make it simpler. (in your original example, each of your "if" statements contain several different comparisons - this is a common example of complexity which can be cut down with a little careful thought)

I'll have to disagree with you there, the length/size of the program is absolutely a major contributing factor to how easy your program is to read, debug, maintain and extend.

It's not the only contributing factor and you shouldn't sacrifice clarity for brevity. But a good solution solution to a problem is mostly the obvious one, expressed in an elegant and concise way. If you need a 100 lines of code to do something you can explain in one sentence, then that's a tell tale sign that you are not doing it right and should take a step back.

This post has been edited by Karel-Lodewijk: 18 February 2012 - 12:07 PM

[Bryston]

examples from practical c++

while ('\n' != (*p++ = *q++));

or

while (1){
     *destination_ptr == *source_ptr;
     if (*destination_ptr ++ '\n')
          break;
     destination_ptr++;
     source_ptr++;
}

which would you rather see when you have to maintain the code?

[JackOfAllTrades]

while ('\n' != (*p++ = *q++));

Code like that is just generally programmer dick-waving. "Look how cool I am! I understand pointer arithmetic!" Yeah, well the junior programmer from East Asswipe who ends up maintaining that shit after you've moved on to bigger and better things is going to be cursing your name. Maybe that doesn't matter to some people, but it does to me.

Moved to the Discussion area, as this really isn't a help question.

[ishkabible]

Ya, Karel-Lodewijk said exactly what I was going to; it doesn't scale well. more often than not programs have to deal with data of arbitrary size and often those sizes are far too huge to hard code everything.

Quote

The right solution to this problem is actually reading the data into an array, sorting it, and selecting the top array index, but I'm guessing at the point of this assignment you hadn't reached that. Bet you'll get there relatively soon.

for the most part I would agree but wouldn't something like std::max_element be better than a sort? it's linear time and doesn't modify the array

This post has been edited by ishkabible: 18 February 2012 - 04:58 PM

[JackOfAllTrades]

ish, I'm sure you're right. I've spent so much time in C rather than C++ (and little time in the sorting space...I let my DBs do that work for me ), I rarely even think of all the STL has to offer.

[Karel-Lodewijk]

Bryston, on 18 February 2012 - 07:40 PM, said:

examples from practical c++

while ('\n' != (*p++ = *q++));

or

while (1){
     *destination_ptr == *source_ptr;
     if (*destination_ptr ++ '\n')
          break;
     destination_ptr++;
     source_ptr++;
}

which would you rather see when you have to maintain the code?

We can throw examples around all day. The bottom line is that if you need a lot of lines to express something you can explain very simply, then it means you probably lost track of the essence of what you are doing. You are getting bogged down with exceptions/and or duplicating a lot of code. You are probably taken a much to low level view of the matter.

Take, this snippet a few post down: http://www.dreaminco...crement-number/ . And if the person who made that post is reading this, I'm sure your still learning, I mean no offense.

It's a 360 line program and basically all he wants can be summarized in this menu.

cout << "Please enter your choice:"<<endl<<endl;
cout << "1:  Add patient"<<endl;
cout << "2:  Add critically ill patient"<<endl;
cout << "3:  Call a patient for operation"<<endl;
cout << "4:  Remove all patients from queue"<<endl;
cout << "5:  Show all patient"<<endl;

And the code is not even complete. The auto id is not working. And I'm assuming he wants critically ill patients to have precedence over a regular patient, but this is not implemented. I would expect another 100 lines before he's done.

So why is he having so much difficulties. He's writing a lot of C-style string which I'm sure caused him a fair deal of trouble but this is not the core of it. His big errors are:

• He noticed he needed a queue. He however did not defined beforehand what this queue would need to do (which operations it should support), he did not think what would be a suitable data structure (removing elements from the front of an array is the worst you can do performance wise). And he did not check whether the data structure he needed or something close to it already exists.
  
• He has no clear concept of when a patient is created, what it does during it's lifetime and where it's life ends. The program creates a lot of empty patients, for example when it does not know what to return. I wanted to retrofit an automatic id counter in his code to show the concept, I added it to the constructor and the id of the first patient I got was 31. The code actually created 30 patient objects before it even added a patient. Not 30 copies, 30 new patient objects.

As a result he is writing code, a lot of code with a lot of problems and he is adding more code to fix those problems. This is the point where he really should take a step back and re-evaluate. If you need a lot of code to do something simple you are doing it wrong.

So let's redesign it from the start. I'm probably going to be using things he is not yet familiar with but this is how I would do it.

So we start by creating a patient. Since every patient should have credentials I'll go right ahead and create a constructor for it that fills in all the right fields, No nameless/empty patients in my system. I'll also get rid of all character arrays in favor of std::string and a phone number is not an ID.

// define structure for patient information
class Patient {
  public:
    Patient(const std::string& first_name_, const std::string& last_name_, const std::string& phone_number_) 
        :first_name(first_name_)
        ,last_name(last_name_)
        ,phone_number(phone_number_) 
        ,id(id_counter++) {}
    std::string first_name;
    std::string last_name;
    std::string phone_number;
    unsigned int id;
  private:
    static unsigned int id_counter;
};

unsigned int Patient::id_counter = 1;

Now, I can only create a patient when I supply all data, and I will only do so when it checks out. Every patient will automatically get an id. I've not added getters and setters for all of patients data. My opinion is if you are going to write a function that can change and read a member, then you might as well make it public (or make it private and define friends).

Now we are probably going to want to be able to print a patient. So we'll do that next. A print function would be perfectly ok, but a better way is overloading the << operator for ostream. The advantages are that sending something directly to cout (i.e.: cout << patient) is very natural. But it also allows to send a patient to a file with the same ease.

std::ostream& operator<<(std::ostream& out, const Patient& patient) {
    out << "Patient " << patient.id << "\n";
    out << "  First name: " << patient.first_name << "\n";
    out << "  Last name: " << patient.last_name << "\n";
    out << "  Phone number: " << patient.phone_number << "\n";
    return out;
}

Now I'm going to go ahead and create a read patient function as well.

Patient read_patient() {
    std::string first_name, last_name, phone_number;
    do {
        std::cout << "Please enter a first name:";
        std::getline(std::cin, first_name);
    } while (first_name == "");
    do {
        std::cout << "Please enter a last name:";
        std::getline(std::cin, last_name);
    } while (last_name == "");
    std::cout << "Please enter a phone number:";
    std::getline(std::cin, phone_number);
    return Patient(first_name, last_name, phone_number);    
}

Some people might argue, you should make this a static function of patient, but I'm not an OO purist. Some people might argue that you could override istream >>, but then I would have to supply a default constructor for Patient and if I do a istream overload, I'd like it to be symmetric with the ostream overload so I can read/write patients to file in a hearthbeat. But this wouldn't be very user friendly. Some people say returning an object by value is a bad practice as it incurs a copy. But return type optimization is in every C/C++ compiler I know of, so this is simply not true. I apply the KISS principle here, keep it simple stupid. Notice I insist on a name but not on a phone number, I don't like phones.

Time for a little test:

    Patient patient = read_patient();
    std::cout << patient << std::endl;

And after fixing some error you'll never get to see I get:

Patient 1
  First name: John
  Last name: Doe
  Phone number: 048741245

Now we get to the interesting part. Queuing patients. Basically I want a FIFO data structure (first in first out), but I want to give preference to patients who are critically ill. My first thought was std::priority_queue, but after looking it up I found out that it is not stable when priorities are equal. So I went for std::dequeue, it is a FIFO queue but also supports insert at a given position, so I can look up the position and insert it at the right spot. I considered using std::deqeue<std::pair<Piority, Patient> >, but then realized I probably want the priority in patient anyway. So I retrofitted that.

The new patient is now:

enum Priority {CRITICAL_PRIORITY = 0, NORMAL_PRIORITY = 1};

// define structure for patient information
class Patient {
  public:
    Patient(const std::string& first_name_, const std::string& last_name_, const std::string& phone_number_, Priority priority_ = NORMAL_PRIORITY) 
        :first_name(first_name_)
        ,last_name(last_name_)
        ,phone_number(phone_number_) 
        ,priority(priority_) 
        ,id(id_counter++) {}
    std::string first_name;
    std::string last_name;
    std::string phone_number;
    Priority priority;
    unsigned int id;
  private:
    static unsigned int id_counter;
};

And the department:

class Department {
  public:
    Department(const std::string& name_) : name(name_) {}
    void add_patient(Patient patient) {
        if (queue.size() >= MAX_PATIENTS && patient.priority != CRITICAL_PRIORITY) { //we're running a humane hospital here
            std::cout << "Maximum patient capacity reached, try again later" << std::endl;
            return;
        }
        //find first patient with same priority
        std::deque<Patient>::iterator it = std::find_if(queue.begin(), queue.end(), 
            [&patient](const Patient& queue_patient) {return queue_patient.priority == patient.priority;}
        );
        //insert patient before that patient
        queue.insert(it, patient);
    }
    Patient next_patient() {
        Patient patient = queue.back();
        queue.pop_back();
        return patient;
    }
    std::string name;
    std::deque<Patient> queue;
};

std::ostream& operator<<(std::ostream& out, const Department& dept) {
    out << dept.name << std::endl;
    for (auto it = dept.queue.rbegin(); it != dept.queue.rend(); ++it) {
        out << *it << std::endl;
    }
    return out;
}

I have to admit, I used some C++11 functions here, start getting used to it:). Add_patient will add new patients, first based on their priority and further abiding by the rules of a FIFO queue. It works by looking for the first patient with the same priority and insert will add the new patient right before that one. In other words the queue will be ordered on priority first and then on order second. I also added a print function. Notice I print them in reverse order because patients with the highest priority order should be at the back now.

Again a little test:

    Patient patient1("a","a","a",NORMAL_PRIORITY);
    Patient patient2("b","b","b",NORMAL_PRIORITY);
    Patient patient3("c","c","c",CRITICAL_PRIORITY);
    Patient patient4("d","d","d",CRITICAL_PRIORITY);
    Department dept("Dep");
    dept.add_patient(patient1);
    dept.add_patient(patient2);
    dept.add_patient(patient3);
    dept.add_patient(patient4);
    std::cout << dept;

Which after fixing a few errors gives us:

Patient 3
  First name: c
  Last name: c
  Phone number: c
  Priority: CRITITCAL
Patient 4
  First name: d
  Last name: d
  Phone number: d
  Priority: CRITITCAL
Patient 1
  First name: a
  Last name: a
  Phone number: a
  Priority: NORMAL
Patient 2
  First name: b
  Last name: b
  Phone number: b
  Priority: NORMAL

Perfect critical before normal and then the order in which they were added. Now our program is basically done. Just put in the interface.

int main() {
    Department emergency_department("Emergency department");

    bool quit = false;
    while (quit == false) {
        std::cout << "Welcome to Emergency Department (ED)" << std::endl;
        std::cout << "Please enter your choice:\n" << std::endl;
        std::cout << "1:  Add patient"<<std::endl;
        std::cout << "2:  Call a patient for operation" << std::endl;
        std::cout << "3:  Remove all patients from queue" << std::endl;
        std::cout << "4:  Show all patient" << std::endl;
        std::cout << "5:  Quit\n" << std::endl;

        int choice;
        std::cin >> choice;

        if (choice == 1) {
            Patient patient = read_patient();
            emergency_department.add_patient(patient);
        } else if (choice == 2) {
            if (emergency_department.queue.empty()) {
                std::cout << "No more patients" << std::endl;   
            } else {
                Patient patient = emergency_department.next_patient();
                std::cout << patient << std::endl;
                std::cout << "This patient is now ready for operation" << std::endl;
            }
        } else if (choice == 3) {
            emergency_department.queue.clear();
            std::cout << "Cleared the patient queue" << std::endl;
        } else if (choice == 4) {
            std::cout << emergency_department << std::endl;
        } else if (choice == 5) {
            quit = true;
        }
    }
}

The final result is:

Spoiler

#include <iostream>
#include <string>
#include <queue>
#include <cctype>
#include <algorithm>

// The maximum number of patients in queue
const unsigned int MAX_PATIENTS = 30;

enum Priority {CRITICAL_PRIORITY = 0, NORMAL_PRIORITY = 1};

// define structure for patient information
class Patient {
  public:
    Patient(const std::string& first_name_, const std::string& last_name_, const std::string& phone_number_, Priority priority_ = NORMAL_PRIORITY) 
        :first_name(first_name_)
        ,last_name(last_name_)
        ,phone_number(phone_number_) 
        ,priority(priority_) 
        ,id(id_counter++) {}
    std::string first_name;
    std::string last_name;
    std::string phone_number;
    Priority priority;
    unsigned int id;
  private:
    static unsigned int id_counter;
};

unsigned int Patient::id_counter = 1;

std::ostream& operator<<(std::ostream& out, const Patient& patient) {
    out << "Patient " << patient.id << "\n";
    out << "  First name: " << patient.first_name << "\n";
    out << "  Last name: " << patient.last_name << "\n";
    out << "  Phone number: " << patient.phone_number << "\n";
    out << "  Priority: ";
    switch (patient.priority) {
        case CRITICAL_PRIORITY:
            out << "CRITITCAL";
            break;
        case NORMAL_PRIORITY:
            out << "NORMAL";
            break;
    }
    return out;
}

Patient read_patient() {
    std::string first_name, last_name, phone_number;
    char critical;
    do {
        std::cout << "Please enter a first name: ";
        std::getline(std::cin, first_name);
    } while (first_name == "");
    do {
        std::cout << "Please enter a last name: ";
        std::getline(std::cin, last_name);
    } while (last_name == "");
    std::cout << "Please enter a phone number: ";
    std::getline(std::cin, phone_number);
    do {
        std::cout << "Critical (y/n): ";
        std::cin >> critical;
    } while (toupper(critical) != 'Y' && toupper(critical) != 'N');
    Priority priority;
    if (toupper(critical) == 'Y')
        priority = CRITICAL_PRIORITY;
    else 
        priority = NORMAL_PRIORITY;
    return Patient(first_name, last_name, phone_number, priority);    
}

class Department {
  public:
    Department(const std::string& name_) : name(name_) {}
    void add_patient(Patient patient) {
        if (queue.size() >= MAX_PATIENTS && patient.priority != CRITICAL_PRIORITY) { //we're running a humane hospital here
            std::cout << "Maximum patient capacity reached, try again later" << std::endl;
            return;
        }
        std::deque<Patient>::iterator it = std::find_if(queue.begin(), queue.end(), 
            [&patient](const Patient& queue_patient) {return queue_patient.priority == patient.priority;}
        );
        queue.insert(it, patient);
    }
    Patient next_patient() {
        Patient patient = queue.back();
        queue.pop_back();
        return patient;
    }
    std::string name;
    std::deque<Patient> queue;
};

std::ostream& operator<<(std::ostream& out, const Department& dept) {
    out << dept.name << std::endl;
    for (auto it = dept.queue.rbegin(); it != dept.queue.rend(); ++it) {
        out << *it << std::endl;
    }
    return out;
}

int main() {
    Department emergency_department("Emergency department");

    bool quit = false;
    while (quit == false) {
        std::cout << "Welcome to Emergency Department (ED)" << std::endl;
        std::cout << "Please enter your choice:\n" << std::endl;
        std::cout << "1:  Add patient"<<std::endl;
        std::cout << "2:  Call a patient for operation" << std::endl;
        std::cout << "3:  Remove all patients from queue" << std::endl;
        std::cout << "4:  Show all patient" << std::endl;
        std::cout << "5:  Quit\n" << std::endl;

        int choice;
        std::cin >> choice;

        if (choice == 1) {
            Patient patient = read_patient();
            emergency_department.add_patient(patient);
        } else if (choice == 2) {
            if (emergency_department.queue.empty()) {
                std::cout << "No more patients" << std::endl;   
            } else {
                Patient patient = emergency_department.next_patient();
                std::cout << patient << std::endl;
                std::cout << "This patient is now ready for operation" << std::endl;
            }
        } else if (choice == 3) {
            emergency_department.queue.clear(); //might want to make a member function for this
            std::cout << "Cleared the patient queue" << std::endl;
        } else if (choice == 4) {
            std::cout << emergency_department << std::endl;
        } else if (choice == 5) {
            quit = true;
        }
    }
}

I think you'll agree this code is a lot more maintainable because:

• It's a lot shorter 2-3x, especially since it does more. And almost all of it is boiler plate code. add_patient/next_patient is actually the only part of it resembling an algorithm. It's actually more syntactic salt than I'd like to see in a simple program. But it's mostly a necessary to for fill the assignment.
  
• It needs a lot less maintaining. Because of the high level abstractions I have a lot more confidence in it's correctness.
  
• It is easier to use. Look at the main function, it flows quite naturally. Many errors are a result of people using something wrong. Especially when they have not written it themselves.
  
• There is some room for extension here and there without going overboard (adding complexity).

I'm not saying shorter code equals better code but there is certainly a correlation. I'm not saying shorter code should be your aim, but concise code should be. It has not happened to me often that after improving some code that I end up with something longer.

P.S.: Might have gone a bit overboard to prove a point, I was looking for an example and I got a bit side tracked, but then again I answered 2 posts at once.
P.P.S: The comments belonging in the code are in between the pieces of code

This post has been edited by Karel-Lodewijk: 23 February 2012 - 01:15 AM

[cfoley]

I'm guessing the task set for the class was "read in 4 numbers and print the biggest". I'd suggest something like this:

(untested, and I'm not fluent in c++ so please excuse any errors)

#include <iostream>

using namespace std;

int main() {
	int i, max, temp, count;
	count = 4;
	max = -1; // Assuming numbers entered >= 0

	for(i = 1; i <= count; i++) {
		cout << "Enter number " << i << ": ";
		cin >> temp;
		if (temp > max) max = temp;
	}

	cout << max << " is bigger." << endl;

	system("PAUSE");
	return 0;
}

[ishkabible]

my 1-line STL solution(iterators are amazing)

#include <iostream>
#include <algorithm>
#include <iterator>

int main() {
	std::cout << *std::max_element(std::istream_iterator<int>(std::cin), std::istream_iterator<int>());
}

you have to manually insert an EOF(ctrl + Z on windows) then press enter. it accepts any number of integers and spits out the max.

This post has been edited by ishkabible: 18 February 2012 - 09:19 PM

[raspinudo]

ishkabible, on 18 February 2012 - 09:18 PM, said:

my 1-line STL solution(iterators are amazing)

#include <iostream>
#include <algorithm>
#include <iterator>

int main() {
	std::cout << *std::max_element(std::istream_iterator<int>(std::cin), std::istream_iterator<int>());
}

you have to manually insert an EOF(ctrl + Z on windows) then press enter. it accepts any number of integers and spits out the max.

Very cool. It can be even cooler with C++11(If your compiler supports it), by utilizing the auto keyword, which can replace the verbose iterator declarations.
example from cprogramming.com

//this
map<string, string>::iterator itr = address_book.begin();
//becomes this
auto itr = address_book.begin();

This post has been edited by raspinudo: 24 February 2012 - 01:41 AM