15
\$\begingroup\$

As a newbie programmer, I decided to try my hand at making a program whose function actually might be useful to someone, somewhere.

This utility takes a text file, reads it out to the user, displays its length (in bytes), asks the user if they would like to alphabetize, randomize, or do nothing to the contents of the file. It then asks the user what name they would like to save the new file under, assuming they chose to randomize or alphabetize (with overwrite protection!).

I'm hoping to have you (kindly) pick this program apart and tell me things I ought to do differently for the sake of standards compliance, headache avoidance, etc.

#include <iostream> #include <fstream> #include <string> #include <cstdlib> #include <ctime> using namespace std; //--**--**-- NOTE --**--**-- //Words surrounded with '*' are functions. //Words surrounded with '|' are variables. //prototyping unsigned int getLength(string fileName); string alphabetize(string source); string getContent(string fileName); string scramble(string source, int numOfPasses); bool doesFileExist(string name); //all done! int main() { char input; //used throughout *main* for various user selection purposes. cout << "Make sure you have a text file in the same directory as this program!"; cout << "\nWhat's the text file's name? (including extension!)" <<endl; string myFile; cin >> myFile; //declares then stores the name of the text file you want to scramble/alphabetize. //--**--**-- NOTE --**--**-- //at this point here, add a check to see if the file the user entered actually exists! cout << endl << "--------------" << endl; // makes a spacer, to keep things nice and neat! cout <<"File content:\n" << getContent(myFile) << endl << "--------------" << endl; cout << "Length of file: " << getLength(myFile); cout << " bytes. Do you want to scramble (alphabetize) this file? \n(y/n): "; cin >> input; if(input =='n'){ cout << ":("; return 0; //that's it, I quit. Done. Kaput. Over. } cout << "Alphabetize, Randomize, or Quit? (a/r/q)?:"; string Scrambled; cin >> input; //there's that char variable again! if (input == 'a'){ //user decided to alphabetize the file. See *alphabetize* for how this is done. Scrambled = alphabetize(getContent(myFile)); //sets |Scrambled| equal to the string returned by *alphabetize()* }else if(input == 'r'){ //user decided to randomize the file. See *scramble* for how this is done. cout << "Number of passes to scramble? \n(recommended: between 10 and 20x length of file)" << endl; int numPass; cin >> numPass; //user specifies how many passes the randomizer should perform. Scrambled = scramble(getContent(myFile),numPass); //sets |Scrambled| equal to the string returned by *scramble()* }else if (input == 'q'){ cout << "Stopping. Sorry for wasting your time. :(" << endl; return 0; //-sadface- i has a sad. } cout << "Save name: "; string name; // char answer; // Just a couple variables. Don't mind them. do{ cin >> name; ofstream output; if (doesFileExist(name) == false){ //checks to see if another file with the same name exists in the same directory as this program. //If it doesn't find one, the program will use the specified name. //if it does find one, it will ask the user if they want to overwrite it. output.open(name.c_str()); output << Scrambled; return 0; } else { cout << "That file already exists. Overwrite it? Yes? No? Cancel? (y/n/c)"; cin >> answer; if (answer == 'y'){ //if the user enters anything other than 'y' or 'c', the program will ask them for a new //filename, and checks that new name all over again. if the user enters 'y', then //the file with the name in question is overwritten. output.open(name.c_str()); output << Scrambled; output.close(); return 0; } else if (answer == 'c'){ //Save cancelled. sadface. cout << "Canceled."; return 0; } else{ cout << "Save name: "; } } } while (true); } string scramble(string source, int numOfPasses){ int randNum; unsigned int index; //unsigned. can't have negative indices, now can we? cout << "--------------" << endl; cout << source << endl; do{ index = 1; //resets |index| after every pass. do{ srand(time(0)); //re-seeds *rand()* every sub-pass. randNum = (rand() % 5); //regenerates the random number every sub-pass. if(randNum % 2 == 1){ //for every sub-pass, the contents of |source[index]| and |source[index - 1]| (if source were a character array, that is.) //have a 60% chance of being swapped. swap(source.at(index),source.at(index-1)); } ++index; }while(index < source.length()); numOfPasses--; }while(numOfPasses > 0); cout << source << endl; cout << "--------------" << endl; //prints the newly randomized string, with a spacer. return source; } string alphabetize(string source){ //not quite as complex as *scramble* int numOfSwaps; // unsigned int index; //couple variables. Don't mind them. cout << "--------------" << endl; cout << source << endl; do{ //this function keeps running until the source string is completely alphabetized. numOfSwaps = 0; //resets the tally of number of swaps performed to prevent infini-looping. index = 1; //resets the index. do{ if(source.at(index) < source.at(index-1)){ //if the ascii representation of the character in the string at (index) //is less than the ascii representation of the character at (index - 1), //swap them, and add one to 'numOfSwaps' swap(source.at(index),source.at(index-1)); ++numOfSwaps; //cout << source << endl; //uncomment the above line (by removing the '//') to print the source string after each sub-pass. //NOTE: This will seriously spam your console!! } ++index; }while(index < source.length()); //cout << source << endl; //uncomment the above line (by removing the '//') to print the source string after each pass. //NOTE: This too will spam your console! }while(numOfSwaps != 0); //ends the loop when the inner-most IF statement does not execute in a pass. cout << source << endl; cout << "--------------" << endl; //prints the newly alphabetized string, with a spacer. return source; } unsigned int getLength(string fileName){ //I have no idea how this one works. I got it off the internet. ifstream InFile(fileName.c_str()); unsigned int FileLength = 0; while (InFile.get() != EOF) FileLength++; InFile.close(); return FileLength; } string getContent(std::string fileName){ //I don't know how this function works, and just like *getLength*, I got it off the 'net. ifstream ifs; ifs.open(fileName.c_str()); string str((istreambuf_iterator<char>(ifs)), istreambuf_iterator<char>()); return str; } bool doesFileExist(string name){ //This one should be self-explanatory. ifstream file(name.c_str()); if(!file) return false; else return true; }
\$\endgroup\$
5
  • \$\begingroup\$ Welcome! I've noticed that you've stated that some code was taken from the Internet. We cannot help explain it to you, as that's off-topic on this site. In general, it may help to understand something before you try to utilize it, otherwise it could make it harder to maintain your program. \$\endgroup\$ Commented Nov 20, 2014 at 7:23
  • \$\begingroup\$ @Jamal The purpose of this question was not for an explanation of the two functions I got off the 'net. I was hoping for suggestions on improving the way the program as a whole functions. :D \$\endgroup\$ Commented Nov 20, 2014 at 7:27
  • \$\begingroup\$ Okay, just making sure, as comments like that seem to be directed towards readers only. But yes, we can certainly review everything else. I don't have much time to review it now (I am a C++ reviewer here), but there are others around. \$\endgroup\$ Commented Nov 20, 2014 at 7:30
  • \$\begingroup\$ I'd greatly appreciate and and all (constructive) criticism whenever you find the time. :D \$\endgroup\$ Commented Nov 20, 2014 at 7:33
  • 1
    \$\begingroup\$ A quick note to your comments: If you want to differentiate between functions and variables a) Use a consistent and different naming scheme for each of them b) If it's a function just write func_name() \$\endgroup\$ Commented Nov 20, 2014 at 14:52

3 Answers 3

Reset to default
9
\$\begingroup\$

In this review, I'll omit the code that was not implemented by you. At some point, you can try to figure out how it works and then replace it with your own implementation that can be reviewed here.

As a beginner, it's great to see that you've started using functions to increase modularity. However, you can use more, as main() is still doing a lot of work. You generally should just have it collect input, call other functions, and display something at the end. Most of the other functions will perform one primary thing and usually shouldn't need to display anything. Many of your conditionals in main() can be moved elsewhere, also making it easier to see what main() should really be doing itself.

The structure at this level also seems okay, but more can still be done. For instance, there is probably a better way to alphabetize, without using your own function. It looks like you're just needing to sort the characters, and if I'm not mistaken, you're attempting to implement bubble sort. Either way, this is one of the slowest sorting algorithms around, and should be avoided for serious code like this. Instead of using this function, you can use std::sort() from the standard library, which utilizes a quicker sorting algorithm called quicksort.

std::sort(source.begin(), source.end());

The same applies to scramble(). It looks like you're trying to shuffle the characters in the string, so you can instead use std::random_shuffle() (assuming you don't have access to C++11):

std::random_shuffle(source.begin(), source.end());

(The linked pages should also tell you what begin() and end() are for.)

While it is great that you're trying to implement these things yourself, you should eventually learn how to utilize the standard library to help simplify your code.

I'll now cover some individual points for further simplification:

  • You don't actually need function prototypes. In order to avoid them, simply define main() below the other functions. That way, it'll already be aware of them.

  • It is common for beginners to use std::endl excessively for displaying newlines. What is not always told is that it does something else: flushes the buffer. This is a slower operation by itself, and it's not something that's usually needed.

    Instead, you can display newlines with "\n":

    std::cout << "some text\n\n";

    This is equivalent to the std::endl version:

    std::cout << "some text" << std::endl << std::endl;

    More info about this can be found here.

  • You can "move" doesFileExist() to main() since file input is involved. But instead of returning and using a bool, you can just terminate from main() if the file cannot be opened:

    if (!file) { std::cerr << "file cannot be opened"; return EXIT_FAILURE; }

    Notes: std::cerr is the error output stream, and EXIT_FAILURE is a macro primarily used for returning a failed error code.

\$\endgroup\$
3
  • \$\begingroup\$ I wasn't aware of std::random_shuffle() or std::sort(). Thanks for the suggestions, I'll work on implementing them in my next revision. \$\endgroup\$ Commented Nov 20, 2014 at 18:21
  • \$\begingroup\$ @GabeEvans: I'm not the best with file operations and such, so someone else may have more to say. But this should at least simplify much of the code. C++ is designed to do many of these things for you. \$\endgroup\$ Commented Nov 20, 2014 at 18:31
  • \$\begingroup\$ Thanks again, Jamal. It would seem that I'm going to need to do a lot of reading up on the standard library. \$\endgroup\$ Commented Nov 20, 2014 at 18:35
8
\$\begingroup\$

In several places, where you could have (and I think should have) used a simple for statement, for example, for (int index = 1; index < length_of_source; ++index), you instead wrote a do statement that scatters the logic of the loop control and makes the program more difficult to understand.

On a different note, do not reseed the rand function (at least not unless you really know what you are doing)! It may look like you get something more completely random by taking srand(time(0)), but how do you know that time(0) has changed between one loop and the next? If it doesn't change, you are repeating the same set of "random" numbers (which now aren't so random). On the other hand, if you call srand(time(0)) once and then never touch it again, you are guaranteed to have a pseudo-random sequence with as good "random" properties as your math library can deliver, which are almost certainly more "random" than anything you can design.

Addendum: As requested, one more recommendation. During a run that actually sorts or "scrambles" a file, the program will call getContent(myFile) twice and getLength(myFile) once. Each of these three function calls opens the file, reads its entire contents, and closes it. Since you will end up doing the bulk of your program's work on a stored copy of the file, I would read the file into memory once and then perform the remaining actions (including informational printouts) on the same stored copy of the file.

Others have offered excellent recommendations; I leave them out of this answer mainly because they have been well expressed elsewhere (and partly so as not to appear to take credit for them).

\$\endgroup\$
1
  • \$\begingroup\$ Hm.. that makes sense. Anything else I should change? \$\endgroup\$ Commented Nov 20, 2014 at 14:34
1
\$\begingroup\$

We can simplify doesFileExist() somewhat. When we see code like if (condition) return true; else return false; we can turn that into return condition;. In this case, we need to invert the condition, because the return statements are the other way around:

bool doesFileExist(string name) { ifstream file(name.c_str()); return file; }

We don't need a copy of name to modifiy, so we can accept a const reference, and there's no need to convert to a C-style string, as std::ifstream accepts a std::string filename (since C++11):

bool doesFileExist(const std::string& name) { std::ifstream file(name); return file; }

We can make it terser (to the point where we arguably no longer need a named function for it) by not naming the stream:

bool doesFileExist(const std::string& name) { return std::ifstream{name}; }

Having said all that, this function will only return true if the file is readable; if the file exists, but we don't have read permissions, then it will wrongly return false. Also, the absence of a file at one moment in time is no guarantee that it will still not exist a few instructions later when we attempt to write to it. The solution for that problem usually involves platform-specific code (e.g. POSIX O_CREAT|O_EXCL), so I won't go into detail here.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.