Thread: My numeric analyzer (2)

I rewrote the code for it. Please tell me how it is:
Numbers.h:

Code:

#include <fstream>
#include <vector>
#include <iostream>
#include <numeric>
#include <algorithm>
#include <string>

using namespace std;
class Numbers{
public:
	Numbers(string);	//Definitions are placed here to be inlined
	__int64 Sum() {return accumulate(_b, _e, 0);}
	double Ave() {return static_cast<double> (Sum()/numCount);}
	int Mid() {return (nums[numCount/2]);}
	void PrintNums() {for each (int val in nums) cout << val << endl;}
	int Min(){return *min_element(_b, _e);}
	int Max(){return *max_element(_b, _e);}
	int Mod();
	int Count()  {return numCount;}
	
private:
	vector<int>::iterator _b,_e;
	unsigned int numCount;
	void ReadFile(string &fileName);
	string fileName;
	vector <int> nums;

};

Numbers.cpp:

Code:

#include "Numbers.h"

Numbers::Numbers(string fileName)
{
	ReadFile(fileName);
	//So we won't need to call these functions next times
	numCount = static_cast <unsigned int> (nums.size()); _b = nums.begin(); _e = nums.end();		
	sort(_b, _e);	//We need to sort to find the middle (Am I a poet?)	
}

void Numbers::ReadFile(string &fileName)
{
	cout << "Openning \""<< fileName <<"\"." << endl;  
	ifstream file(fileName.c_str(), ios::in);
	if(file.is_open())
	{
		int buf;
		cout << "Reading numbers." << endl;
		while(file >> buf) nums.push_back(buf);
		
		if(!file.eof()) cerr << "WARNING: File was not read to end!" << endl;
		file.close();	
	}
	else cerr << "ERROR: File could not be opened!" << endl;
}

int Numbers::Mod() 
{
	unsigned int countN = 0;
	int mod = 0;
	unsigned int buff;
	for each(int val in nums)
	{
		buff = static_cast<unsigned int> (count(nums.begin(), nums.end(), val));
		if(buff > countN){ countN = buff; mod = val;}
	}
	return mod;
}

You seem to have too many casts. For example, std:.vector::size already returns an unsigned type. If you want to be more pedantic, you could make your size member a std::size_t.

Actually, why duplicate the vector size variable? You can always get the size from the vector (with zero overhead hopefully).

Code:

double Ave() {return static_cast<double> (Sum()/numCount);}

This cast seems to be out of place. If you don't want the average to be truncated, you'll need to cast either member of the division before dividing.

This cast seems to be out of place. If you don't want the average to be truncated, you'll need to cast either member of the division before dividing.

Oh yes wrote it wrong in one of clean ups

OK, I changed them this way:
Header:

Code:

#include <fstream>
#include <vector>
#include <iostream>
#include <numeric>
#include <algorithm>
#include <string>

using namespace std;
class Numbers{
public:
	Numbers(string);	//Definitions are placed here to be inlined
	__int64 Sum() {return accumulate(_b, _e, 0);}
	double Ave() {return (static_cast<double> (Sum()))/numCount;}
	int Mid() {return (nums[numCount/2]);}
	void PrintNums() {for each (int val in nums) cout << val << endl;}
	int Min(){return *min_element(_b, _e);}
	int Max(){return *max_element(_b, _e);}
	int Mod();
	size_t Count()  {return numCount;}
	
private:
	vector<int>::iterator _b,_e;
	size_t numCount;
	void ReadFile(string &fileName);
	string fileName;
	vector <int> nums;

};

cpp:

Code:

#include "stdafx.h"
#include "Numbers.h"

Numbers::Numbers(string fileName)
{
	ReadFile(fileName);
	//So we won't need to call these functions next times
	numCount = nums.size(); _b = nums.begin(); _e = nums.end();		
	sort(_b, _e);	//We need to sort to find the middle (Am I a poet?)	
}

void Numbers::ReadFile(string &fileName)
{
	cout << "Openning \""<< fileName <<"\"." << endl;  
	ifstream file(fileName.c_str(), ios::in);
	if(file.is_open())
	{
		int buf;
		cout << "Reading numbers." << endl;
		while(file >> buf) nums.push_back(buf);
		
		if(!file.eof()) cerr << "WARNING: File was not read to end!" << endl;
		file.close();	
	}
	else cerr << "ERROR: File could not be opened!" << endl;
}

int Numbers::Mod() 
{
	int mod = 0;
	__w64 int buff, countN = 0;
	for each(int val in nums)
	{
		buff = count(nums.begin(), nums.end(), val);
		if(buff > countN){ countN = buff; mod = val;}
	}
	return mod;
}

And the runner:

Code:

#include "Numbers.h"

int main(int argc, char* argv[])
{
	string fName;
	if(argc>1) fName = argv[1];
	else fName = "nume.txt";
	Numbers nums(fName);
	if(nums.Count()){
		nums.PrintNums();
		cout << "-------------------------------------------------------------------" << endl;
		cout << "Count: " << nums.Count() << endl;
		cout << "Sum: " << nums.Sum() << endl;
		cout << "Average: " << cout.precision(10) << nums.Ave() << endl;
		cout << "Mid: " << nums.Mid() << endl;
		cout << "Mod: " << nums.Mod() << endl;
		cout << "Max: " << nums.Max() << endl;
		cout << "Min: " << nums.Min() << endl;
	}
	cout << "\nPress a key to exit..." << endl;
	_getch();
	return 0;
}

What about inlinings?

Code:

#include <fstream>
#include <vector>
#include <iostream>
#include <numeric>
#include <algorithm>
#include <string>

using namespace std;
class Numbers{
public:
	Numbers(string);	//Definitions are placed here to be inlined
	__int64 Sum() {return accumulate(_b, _e, 0);}
	double Ave() {return (static_cast<double> (Sum()))/numCount;}
	int Mid() {return (nums[numCount/2]);}
	void PrintNums() {for each (int val in nums) cout << val << endl;}
	int Min(){return *min_element(_b, _e);}
	int Max(){return *max_element(_b, _e);}
	int Mod();
	size_t Count()  {return numCount;}
	
private:
	vector<int>::iterator _b,_e;
	size_t numCount;
	void ReadFile(string &fileName);
	string fileName;
	vector <int> nums;

};

You should avoid putting a "using namespace" statement anywhere within a header file as it opens up that entire namespace to whatever source file includes that header... intentional or otherwise. The namespace should be explicitly indicated where needed within the header. After that, any source file which includes a header can then put in a "using namespace" statement if necessary. For example:

In the header file:

Code:

#include <fstream>
#include <vector>
#include <iostream>
#include <numeric>
#include <algorithm>
#include <string>

class Numbers{
public:
	Numbers(std::string);	//Definitions are placed here to be inlined
	__int64 Sum() {return std::accumulate(_b, _e, 0);}
	double Ave() {return (static_cast<double> (Sum()))/numCount;}
	int Mid() {return (nums[numCount/2]);}
	void PrintNums() {for each (int val in nums) std::cout << val << std::endl;}
	int Min(){return *std::min_element(_b, _e);}
	int Max(){return *std::max_element(_b, _e);}
	int Mod();
	size_t Count()  {return numCount;}
	
private:
	std::vector<int>::iterator _b,_e;
	size_t numCount;
	void ReadFile(std::string &fileName);
	std::string fileName;
	std::vector <int> nums;
};

And then in the source files:

Code:

#include "Numbers.h"
using namespace std;

...

Code:

void PrintNums() {for each (int val in nums) cout << val << endl;}

I'm pretty sure this is not valid C++. Do you compile your code before posting it?

I'm pretty sure this is not valid C++. Do you compile your code before posting it?

Of course. You can read about that in MSDN.

Ah, a language extension of MSVC. You might want to specify that next time, or simply dont use such an extension.

I didn't know that. Are you sure it is not standard? Maybe it is in standard proposal?

I didn't know that. Are you sure it is not standard? Maybe it is in standard proposal?

Apparently it is part of C++/CLI. As such, it is not part of standard C++, but is a language extension. The next edition of the C++ Standard will probably include a for each loop, but with a slightly different syntax.

OK, I used this technique.

Code:

int Numbers::Mod() 
{
	int mod = 0;
	__w64 int buff, countN = 0;

	for(vector<int>::const_iterator i = nums.begin(); i < nums.end(); ++i)
	{
		buff = count(nums.begin(), nums.end(), *i);
		if(buff > countN){ countN = buff; mod = *i;}
	}
	return mod;
}