kazack said:
Welp after writing and rewriting I hope I have code that is 100% standard
with the exception of the void fileexistcheck(filename) that is OS
dependant. I hope the rest of it is totally standard.
Yes
I would like to know for my own benefit, Is it
better to have a value returning function or to pass a pointer?
Almost always returning a value is preferable, at least IMO. See my
comments in code.
#include <iostream>
#include <io.h>
#include <cstring>
#include <string>
#include <fstream>
#include <cctype>
using namespace std;
[Function prototypes omitted]
enum Triangles{Scalene,Isosceles,Equalateral,Invalid,Hold};
Triangles Entered = Hold;
int main()
{
string filename;
char yeahneah;
bool exists;
selectfilename(filename);
fileexistcheck(filename,exists);
Why oh why do you do this?
One of the reasons that some people
don't like reference passing (and one reason Java doesn't have it in
the sense C++ does, and one reason why C# requires the keyword ref in
the function call, as in fileexists(filename, ref exists)) is that it
is not apparent at all that the function changes the value of exists.
Returning the bool then saying
exists = fileexistcheck(filename);
is *much* more readable for anyone I've talked to. Passing values out
of functions in the way you did has uses in some cases, for instance
if you need two values returned (e.g. a pointer and a bool if the
pointer is valid), but I'd say change this. And if I were your boss
I'd say change it.
Plus, here you could say
if (fileexistcheck(filename) == true)
if you did that. You can compose function calls.
{
readdata(filename);
}
else
{
cout << endl << filename << " does not exist." << endl;
cout << "Would you like to select a different filename? (Y/N): ";
cin >> yeahneah;
if (toupper(yeahneah) == 'Y')
{
main();
}
else
{
cout << "Would you like to create this file? (Y/N):";
cin >> yeahneah;
if (toupper(yeahneah)=='Y')
{
createfile(filename);
readdata(filename);
}
}
}
return 0;
}
void selectfilename(string& filename)
{
cout << "Please Enter The Data File You Would Like To Use: ";
cin >> filename;
}
Case in point of what I said earlier: before I got here I had a
comment after the declaration of filename in main that said "I assume
filename is initialized somewhere in here" because it was not clear at
all that selectfilename sets its argument to the filename.
void fileexistcheck(string filename, bool& exists)
{
if ((_access(filename.c_str (),0))!=-1)
{
exists = true;
}
else
{
exists = false;
}
}
Here you can just test to see if the fstream you attempt to open in
the next function is good. Like you have a while(inData) loop; this
will not run if the file does not exist and is not created (if you
want to stop creation you need to pass another argument to the
constructor). It's not entirely the same thing, but it's probably
acceptable.
void readdata(string filename)
{
int sidea;
int sideb;
int sidec;
ifstream inData;
inData.open(filename.c_str());
inData >> sidea >> sideb >> sidec;
while(inData)
{
NotValid(sidea, sideb, sidec);
if (Entered == Invalid)
{
triangle(Entered,sidea,sideb,sidec);
As the program is structured now, there is no need to pass Entered
since its global anyway.
}
else
{
Equal(sidea,sideb,sidec);
if (Entered == Equalateral)
{
triangle(Entered,sidea,sideb,sidec);
}
else
{
Isos(sidea,sideb,sidec);
if (Entered == Isosceles)
{
triangle(Entered,sidea,sideb,sidec);
}
else
{
Entered = Scalene;
triangle(Entered,sidea,sideb,sidec);
}
}
}
inData >> sidea >> sideb >> sidec;
}
inData.close();
}
[Other functions omitted; no comments]
I would move the variable Entered into the readdata function. Then
pass it as a parameter to the other functions like you do with
triangle.
[Several functions omitted]
void triangle(int Entered,int sidea, int sideb, int sidec)
{
switch(Entered)
{
case 0 : cout << "The dimensions " << sidea << ", "<<sideb <<", "<<sidec
<<" makes this a Scalene Triangle"<<endl; break;
case 1 : cout << "The dimensions " << sidea << ", "<<sideb <<", "<<sidec
<<" makes this an Isosceles Triangle"<<endl; break;
case 2 : cout << "The dimensions " << sidea << ", "<<sideb <<", "<<sidec
<<" makes this an Equalateral Triangle"<<endl; break;
case 3 : cout << "The dimensions " << sidea << ", "<<sideb <<", "<<sidec
<<" makes this an Invalid Triangle"<<endl; break;
}
}
"Magic numbers" in you switch statement. You should use the constants
defined with the Triangle enum instead. case Scalene:, case: Isoceles,
etc. That's what they are there though.
[Several more functions omitted]
Sometime in the next couple days I'll post code the way I'd attack the
problem for comparison purposes. I don't have time at this exact
moment but should be able to find time in a couple days, so check
back. (I'll also address the file existance problem.)
Evan