Code pointers/cleanup for working program

[justStarting]

I'm just looking for some pointers on the following code. Everything worked in the end but I feel like some of the code was unneeded and wasn't sure if I did the switch statements they way they should have been done. Again, I got everything to work, I just want some cleanup pointers. The file that I loaded had a mixed list of part#s (char and #s), part class(single char), total in stock inventory for that part, and cost for the particular part. Thank you.

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

using namespace std;

int main()
{
	ifstream inFile;
	string partNumb;
	
	char partClass; // letter of parts A,B,C,D, & Unknown
	int partA = 0, partB = 0, partC = 0, partD = 0, partU = 0; // int totals of each part
	int inventory;
	double price;
	double totalA=0, totalB=0, totalC=0, totalD=0, totalU = 0; // int totals for inventory/price
	
	
	cout << fixed << setprecision(2);
	inFile.open("parts.txt"); // opening "parts.txt"
	if (inFile)
	{		
		while (inFile >> partNumb >> partClass >> inventory >> price) // pulling data from txt.
		{
			//switch statments to sort the classes and total up how many of each and calculate the total prices/inventory
			switch (partClass)
			{
			case 'A': cout << "";
				if (partClass == 'A')
				{
					partA++;
					totalA += inventory*price;
				}
				break;
			case 'B': cout << "";
				if (partClass == 'B')
				{
					partB++;
					totalB += inventory*price;
				}
				break;
			case 'C': cout << "";
				if (partClass == 'C')
				{
					partC++;
					totalC += inventory*price;
				}
				break;
			case 'D': cout  <<"";
				if (partClass == 'D')
				{
					partD++;
					totalD += inventory*price;
				}
				break;
			default: cout <<"";
				if (partClass != 'A','B','C','D')
				{
					partU++;
					totalU += inventory*price;
				}				
			}
		}
		inFile.close(); // closing file

		cout << "---------------------- INVENTORY REPORT ------------------\n";
		cout << "A parts\t Count:\t\t" << partA << "  " << "Value of inventory:\t " << setw(9) << totalA << endl;
		cout << "B parts\t Count:\t\t" << partB << "  " << "Value of inventory:\t " << setw(9) << totalB << endl;
		cout << "C parts\t Count:\t\t" << partC << "  " << "Value of inventory:\t " << setw(9) << totalC << endl;
		cout << "D parts\t Count:\t\t" << partD << "  " << "Value of inventory:\t " << setw(9) << totalD << endl;
		cout << "Unknown\t Count:\t\t" << partU << "  " << "Value of inventory:\t " << setw(9) << totalU << endl;
	}
		
	// else statment that will load if txt. fails to.
	else
	{
		cout << "Failed to Load \n";
	}
	
	return 0;
/*
---------------------- INVENTORY REPORT ------------------
A parts  Count:         85  Value of inventory:  191180.07
B parts  Count:         69  Value of inventory:   74764.16
C parts  Count:         77  Value of inventory:   50322.30
D parts  Count:         60  Value of inventory:   22416.49
Unknown  Count:         13  Value of inventory:    4282.68
Press any key to continue . . .
*/
}

[jlb]

First this line is probably not doing what you think it is: if (partClass != 'A','B','C','D'). You need both sides of the comparison for every "test": if(partClass != 'A' || partClass != 'B').

Second you probably don't need the above if() statement or any of the other if() statements that are in the case statements because the if() statements are just repeating the logic of the switch.

Third you should start learning to use the class constructors when dealing with C++ streams instead of calling the open() function. And it is usually unnecessary to call the close() function, the destructor will close the stream when the instance goes out of scope. To use the constructor: ifstream inFile("parts.txt"); // opening "parts.txt"

[jonnin]

case 'D': cout <<"";
if (partClass == 'D')


this is totally unnecessary. the case is 'D' so that if statement is always, always true so remove these redundant if statements.

the totals and prices could be much cleaner by using some simple containers, if you have seen these things in your learning?

struct s
{
int part; double total;
}

vector<s> alltheunnecessaryvariables(10); //10 lets you add a couple more if you need to later.


enum letters
{A,B,C,D,U, lmax };


case 'C': //for example
alltheunnecessaryvariables[C].part ++;
alltheunnecessaryvariables[C].total += inventory*price;

if you want to waste space you can even totally eliminate the switch statement and the enum:

vector<s> alltheunnecessaryvariables(256); //allocate enough for every letter in the ascii alphabet

the code collapses to mostly:\
while(stuff)
{
alltheunnecessaryvariables[partClass].part ++;
alltheunnecessaryvariables[partClass].total += inventory*price;
}

and your end of program print statements after look like
cout << "words" << alltheunnecessaryvariables['A'].part << alltheunnecessaryvariables['A'].total << "words" << endl;

wasting this much space is questionable in larger programs where the objects are bigger and you have to think about whether to do this kind of thing. Here, its really not much space in the grand scheme of a modern computer, so its OK. You also run into the time/space tradeoff where you can avoid work (we did that here a little but I mean much more complex work) by wasting space, and again, you have to decide. /shrug now you have seen it, you can make your own decision :) An unfortunate side effect of Unicode and modern coding is that you can't do the cute ascii stuff outside of English programs.

[tpb]

What do you think this does: cout << "";

This is not the correct way to compare partClass to multiple values:

            if (partClass != 'A','B','C','D')

It should be

            if (partClass != 'A' && partClass != 'B' && partClass != 'C' && partClass != 'D')

However, it's not needed since it can never be false (since if it was then one of the other switch cases would have handled it).

Maybe I'm misunderstanding, but shouldn't you be adding "inventory" to partA (or whatever) instead of simply incrementing it?
And you should probably be using a setw(4) (or whatever width) before printing that field.

[justStarting]

Thank you for all the replies and critiques. I will review the code and try to replace/redo some of the fields.