Thread: Rate My Application: File Stream/Bubble Sort

I want some feedback on the application I coded and my source code. Please disregard the fact that I don't have any comments in the source code. I'm not a big fan of comments.

>I'm not a big fan of comments.
I am. Come back when you have some.

Well, since your code appears trivial I'll review it even though it isn't commented. But that's the first strike against you.

Main.h:

>#ifndef MAIN_H
>#define MAIN_H
Inclusion guards, good. But MAIN_H is a common tag, so you may want to choose something a little more unique. For example, I might do this:

Code:

#ifndef JULGUT_MAIN_H
#define JULGUT_MAIN_H

This doesn't protect you from everything, but it does a better job than MAIN_H.

>#include <iostream>
>#include <fstream>
It's better style to avoid including any headers you don't absolutely need in your own header files.

>typedef char* STR;
>typedef const char* CSTR;
I'm divided on whether to count this against you or not. Typdef is a good thing, but for C++ this usage borders on abuse. A better solution would be to use a string class instead of C-style strings, rather than hide the fact that you are using C-style strings behind a type name.

Data.h:

>using std::fstream;
Good! An alternative is to prefix every occurance of fstream with std::, but that gets cluttery quickly. I'm happy to see the absence of a using directive in a header file.

>typedef struct _DATA_
Bzzzt! Ten demerits for using a reserved identifier. Tell me why it's reserved and I'll drop that down to five demerits. The typedef is unnecessary, C++ automatically does to structure, enum, union, and class names what you are trying to do with typedef.

>class Data: public sData
Dumb use of inheritance. At this point you're doing nothing more with inheritance than complicating your program. If you must keep the data members in a separate structure, at least use containment instead of inheritance.

>~Data();
This is good. Your data structure indicates that you will be using dynamic memory, so a destructor to clean up is good thinking. However, if you plan on copying objects of the Data class, it would be an excellent idea to implement a copy constructor and assignment operator as well.

>bool InitOutput( STR );
>bool InitInput( STR );
If your Data class is going to handle two-way data flow, these two functions would be better suited in the constructor. Since you will most likely not use input without output and vice versa, it would be wise to initialize them both at the same time (before a client can make you sorry you didn't). If your class doesn't handle simultaneous two-way communication, this class should be broken into two classes, one for input and one for output.

Data.cpp:

Code:

Data::Data()
{
}

Waste of space. If you're not going to use the constructor, don't declare it and don't define it. The default constructor is created automagically for you by the compiler if you declare no others.

>file.close();
Not necessary. The fstream destructor will take care of this for you.

>cout << endl << "How many numbers do you want to enter? ";
There's no point in using endl right there, you can replace it with '\n' and avoid an unnecessary flush operation.

>cin >> max_num;
Always check data coming into your program for success and validity. If you don't, you'll regret it eventually. max_num is a predictable value already, but failure to read input is not a case you can just ignore and pretend didn't happen. Users don't like that.

>cout << endl << "Input a number: ";
No need for endl again.

>cin >> number;
Check return values.

>file << number << endl;
How do you know file is valid? Sure, most of the time the client will call your init function, but why take the chance? Double check here, or be sure to initialize file in the constructor.

>Buffer= new int[ max_display ];
Check for null if you use a pre-standard compiler, and either catch bad_alloc or notify the client somehow that this function can throw a bad_alloc exception if your compiler is newer.

>Buffer= new int;
This is a bad, bad thing. First, you aren't allocating memory for an array, this is just a single integer. Second, you are causing a memory leak because RecieveData also allocated memory to Buffer.

>while( !file.eof() )
This is an ill-formed loop condition. The eof member function wasn't meant to be used as a loop condition and will cause you problems.

>file >> Buffer[ count ];
Why are you even doing this? Supposedly Buffer already contains the data you want. No sane human being would call sort before getting the data. How does a client know that Sort fills its buffer? And why is a sort function performing I/O operations? "One function, one job", remember that.

I'll refrain from commenting on your sorting algorithm. It appears to be correct even though it's use is never wise.

>while( i < count )
Better to initialize i somewhere near here. Readers will be confused when they see a use without initialization. Yes, it was give the value of 0 at the top of the function, but readers don't want to go hunting for a variable initialization just to see if it was done.

Main.cpp:

>int main( int argc, char *argv[] )
Why are you declaring argc and argv? You don't use them.

>STR Filename= "Numbers.txt";
This cleverly hidden bug makes my point. Assigning a string literal to a char * is a deprecated feature. You should be using CSTR, but the bug is hard to see because you've hidden the type with a typedef.

Code:

cout << "~~Menu: ~~" << endl;
cout << "1. Send Data~" << endl;
cout << "2. Recieve Data~" << endl;
cout << "3. Sort Data~" << endl;

You don't need to use endl so much in this, nor do you even need to call cout so many times. Chain together everything and format it nicely, you won't be able to tell a difference in readability, but you're program will benefit.

>cin >> choice;
Always check return values

I see now that my fears were correct. The Data class does do two distinct jobs that could (should!) be placed in separate classes. Also, requiring the client code to open your file before working with it is no better than making them work directly with fstreams. A class is meant to abstract away difficult or tedious parts so that the program is easier to read, easier to write, and easier to make correct.

>cout << "ERROR!" << endl;
This should be cerr, not cout. And I like how you used a break for the default case, this is a good practice.

>cin.get();
This will likely not work as you expect if cin leaves a newline in the stream.

>return 0;
Not required. Standard C++ allows you to omit the return statement from main and a value of 0 will be implicitly returned.

Conclusion: Not bad, but could be designed and implemented better with a little thought.

I read it, it helped me, thank you.