Could this program have been done better?

[kmce]

Hi, I am trying to improve my programming by going through the Bjarne Stroustrup book, and one of the drills in it wanted me to write a program, which takes 3 int inputs and output from the largest number to the smallest. My problem solving skills are terrible, and they need a lot of work. So I am wonder if someone could tell me if the method i came up with for this program would be good enough, or if there is a better way I should have done it. Thanks

  #include <iostream>

using namespace std;

int main()
{
	int a, b, c;
	int bigger;
	int mid;
	int smallest;
	cout << "Enter a value for 3 numbers\n";
	cin >> a >> b >> c;

	if (a > b)
	{
		bigger = a;
		smallest = b;
	}
	else
	{
		bigger = b;
		smallest = a;
	}
	if (bigger < c)
	{		
		mid = bigger;
		bigger = c;
	}
	else if (smallest < c)
	{
		mid = c;
	}
	cout << bigger <<" " << mid << " "<<
		smallest << "\n";
	

	system("pause");
	return 0;
}

[Niccolo]

This is good in that you've handle all the logical possibilities.

This doesn't matter much, so I'm not saying this is better as a choice, but another way to think of it is that the test for a > b divides the entire problem in half. This is a classic "binary" division of a problem. Once that decision has been made all that is left is to determine where C fits in, which in my proposed formation of the solution would be INSIDE each of the two options flowing from a > b, instead of AFTER it.

So, instead of me showing you, see if you like that way of thinking about it and expressing the idea in code, You might discover one version or the other does less work (is simpler on the CPU - eventually a goal about performance), is easier to write, or easier to recognize when reading it.

[jonnin]

variable names... if you are going to spell them out, don't cut one short. If you have smallest, you should have biggest, if you have smaller, you should have bigger -- use the same type of word for each.

defaulting often can eliminate some work.

int a, b, c;
cout << "Enter a value for 3 numbers\n";
cin >> a >> b >> c;

int bigger = a; //now find a way so that there is are cases where you
int mid = b; //do not change some of these, eg if a really is biggest
int smallest = c; //then you never assign biggest, etc.

use this with the above idea, and it will be more efficient (not that it matters here, but someday, it will). Note that fully condensed logic is often unclear to read, so practice a comment or two when done as well.

[dutch]

#include <iostream>
#include <utility>

void order(int a, int b, int c) {
    if (a < b) std::swap(a, b);
    if (a < c) std::swap(a, c);
    if (b < c) std::swap(b, c);
    std::cout << a << ' ' << b << ' ' << c << '\n';
}

int main() {
    order(1, 2, 3);
    order(1, 3, 2);
    order(2, 1, 3);
    order(2, 3, 1);
    order(3, 1, 2);
    order(3, 2, 1);
}

[Niccolo]

Yes, @dutch, that's modern C++.

The OP is reading a very early chapter in Stroustrup's primer, which he has used as a textbook for teaching C++ at the 101 level.

This is certainly better, no doubt, and what the OP should strive for. At that point in the book I don't think the std namespace has been introduced, or namespaces, though certainly swap is as simple as such a utility can get.

At that point in the text I'm about 60% sure functions haven't even come up.

More importantly, though, @kmce, notice how @dutch's solution takes only 3 steps. In case you need to ask, swap does move a to b and b to a (using a temporary in a 3 step process).

What @dutch shows is an "unrolled" sort for 3 items, and it's a classic pattern (one of many you'll learn on the way).

[dutch]

The OP is reading a very early chapter in Stroustrup's primer

Good point.

using a temporary in a 3 step process

Can we be certain of that? It could use a cpu instruction to swap two integers held in registers.

[kmce]

Hi, Just want to say thanks for the replies. I will be reading more thoroughly through them tonight and try the suggestions and tips :)

[Niccolo]

Can we be certain of that? It could use a cpu instruction to swap two integers held in registers. (About swap as a 3 step process

std::swap is a template function, designed to work on various types which may not fit in registers, so what you're describing is the optimized output.

Even if we do consider a register swap (many CPU's have exchange instructions), the notion that the two registers are "just swapped" is a view through a window, without looking at the circuits behind that view which must, indeed, preserve a temporary of at least one of the registers in a primitive implementation.

There are, however, register renaming features on some CPU's that can merely reorient which register is considered which.

Are we in the weeds yet?

[367kGNh0]

You could try a switch statement. That would be far less bulky. You may need to use a nested switch statement

[Ganado]

@Racake I don't see how what you provided makes sense, at least in isolation. Mind showing a complete example?
<Edit: Now there's a weird combination of bools with case statements. I don't think that's legal.>

[dutch]

@Rascake, your switch demonstrates a total misunderstanding of how switches work. It makes absolutely no sense at all. In particular, case labels must be constant. And in general it would be weird for them to be bools since at a minimum it's pointless.

[malibor]

It's possible to forget about switch or if else stuff altogether;)

#include <iostream>
#include <algorithm>
#include <vector>

// Helper function that returns median value
int median(std::vector<int>&& vec)
{
	size_t n = vec.size() / 2;
	std::nth_element(vec.begin(), vec.begin() + n, vec.end());
	return vec[n];
}

int main()
{
	int a, b, c;
	int bigger;
	int mid;
	int smallest;
	std::cout << "Enter a value for 3 numbers\n";
	std::cin >> a >> b >> c;

        // problem simply put into 3 lines of code
	bigger = std::max(std::max(a, b), c);
	smallest = std::min(std::min(a, b), c);
	mid = median({ a, b, c });

	std::cout << bigger << " " << mid << " " << smallest << "\n";

	std::cin.get();
	std::cin.get();
	return 0;
}

[367kGNh0]

@dutch @Ganado What I did was not good. I had seen this page, and tried to make this as a nested switch statement https://www.tutorialspoint.com/cplusplus/cpp_nested_switch.htm

[Niccolo]

@malibor,

Yes, as @dutch demonstrated, it's possible to use a variety of tools like these, but the OP is reading Stroustrup's primer, in early chapters where functions and the features of the std library have not yet been introduced.

At that point in the book the OP is being instructed on elementary concepts of if/else, assignments and variables. Hardly more than cin and cout have been introduced.

[dhayden]

Niccolo wrote:

This is good in that you've handle all the logical possibilities.

Actually he hasn't handled the case were c is smallest:

$ ./foo
Enter a value for 3 numbers
3 2 1
3 0 2

dutch wrote:

@Rascake, your switch demonstrates a total misunderstanding of how switches work.

You're making the big assumption that you know what he's switching on. Maybe he was thinking of something like this:

#include <iostream>

using namespace std;

void print(int a, int b, int c)
{
    cout << a << ' ' << b << ' ' << c << '\n';
}

int main()
{
	int a, b, c;
	cout << "Enter a value for 3 numbers\n";
	cin >> a >> b >> c;

	int val = a<b;
	val |= (a<c) << 1;
	val |= (b<c) << 2;
	switch (val) {
	case 0:			// b<=a c<=a c<=b
	    print(a,b,c);
	    break;
	case 1:			// a<b  c<=a c<=b
	    print(b,a,c);
	    break;
	case 2:			// b<=a a<c  c<=b
	    // impossible
	    break;
	case 3:			// a<b  a<c  c<=b
	    print(b,c,a);
	    break;
	case 4:			// b<=a c<=a b<c
	    print(a,c,b);
	    break;
	case 5:			// a<b  c<=a b<c
	    // impossible
	    break;
	case 6:			// b<=a a<c  b<c
	    print(c,a,b);
	    break;
	case 7:			// a<b  a<c  b<c
	    print(c,b,a);
	    break;
	}

	system("pause");
	return 0;
}

[lastchance]

#include <iostream>
using namespace std;

int main()
{
   int a, b, c;
   cout << "Enter 3 numbers: ";   cin >> a >> b >> c;
	
   int pos[3];
   pos[ ( a <  b ) + ( a <  c ) ] = a;
   pos[ ( b <= a ) + ( b <  c ) ] = b;
   pos[ ( c <= a ) + ( c <= b ) ] = c;
	    
   for ( int i : pos ) cout << i << '\t';
}

[dutch]

dhayden wrote:

You're making the big assumption that you know what he's switching on

I wasn't assuming anything. Did you see his example switch before he removed it from his post? It was totally bizarre. Something like:

int n;
...
switch (n) {
case (n < 5):
    ...
case (n > 10):
    ...
}

[367kGNh0]

wasn't assuming anything. Did you see his example switch before he removed it from his post? It was totally bizarre. Something like:


int n;
...
switch (n) {
case (n < 5):
...
case (n > 10):
...
}

No, that is illegal code, i did not implement brackets nor skip break;. It was valid because it ran fine on the akacrestonian compiler. But I posted a massively deprecated versiob

[dhayden]

dutch wrote:

I wasn't assuming anything. Did you see his example switch before he removed it from his post? It was totally bizarre.

You're right, he must have deleted it before I saw your comment. I apologize.

[dutch]

Rascake wrote:

No, that is illegal code,

Your code WAS illegal you lying pos.