Working code, but it's one giant "main" function. Help breaking it up?

[acemanhattan]

Alright, after much frustration I have a code that does what I want. The problem is that I'm not comfortable with functions and as a result I've got one big ugly main function.

The biggest obstacle is that I'm still a bit confused about what parameters I would pass through to my functions as well as how to have my function access data from my ifstream variable.

As I see it, there are, at minimum, two functions that I could make using the while loops in my main, but maybe three. My main function does the following:

1) Ask user for file name and open file. If file !open then we output an appropriate response

2) (using a while(!eof) loop) A) Read character by character to track occurrences of a letter. B) Create a "shift" value that will shift each letter by an amount that makes our highest occurring letter == 'e'.

3) (using a while(!eof) loop) Read character by character & manipulate each letter using the shift value while preserving whitespace/punctuation and print new letters to the screen.

I'll post the beast below. Really though, if I could be given direction about what parameters to pass through and how to access my file from inside a function, that would help quite a bit.

  #include<iostream>
#include<fstream>
#include<string>
using namespace std;

int main()
{
	
	// Declaring variables for use throughout program
	ifstream fromFile;
	string fileName;
	char ch;
	int letterCount[26] = {0};
	int index; 
	int mostCommon = 0;
	int shift = 0;
	int tracker = 0;
	string wholeDoc;


	// Prompting user to enter file name and opening file
	cout << "Please enter the file name you wish to decypher : "; 
	cin >> fileName;
	cout << endl << endl;
	fromFile.open(fileName.c_str());
	
	// If a file is not found in the location suggested by the user 
	// an appropriate error message outputs and the program quits.
	if (!fromFile.is_open()) {
		cout << "An error has occured.  File name not found." << endl;
		getchar();
		getchar();
		return 0;
	}
	
	while (!fromFile.eof()){
		fromFile >> ch;
		ch = toupper(ch);
		index = ch -'A';
		if(index < 26 && index >= 0) {
			letterCount[index]++;
		}
	}
	
	for(index = 0; index < 26; index++)	{
		if(letterCount[index] > tracker) {
			tracker = letterCount[index]; 
			mostCommon  = index;
		}
	}
	shift = (('A'+ mostCommon)-'E') ;

	fromFile.clear();
	fromFile.seekg(0);
	while (!fromFile.eof()){
		 fromFile >> ch;
		if(islower(ch)){
			ch -=shift;
			if(ch < 'a'){
				ch+=26;
			}
		}
		if(isupper(ch)){
			ch -=shift;
			if(ch < 'A'){
				ch +=26;
			}
		}

	cout << static_cast<char>(ch);
	
	}
	getchar();
	getchar();
	return 0;
}

Also, this code above is unsatisfactory in that it doesn't "text wrap" when it gets to the screen. Is there something from the iomanip library that can help?

[keskiverto]

Pass to function what the function needs. Pass the ifstream as reference.

[acemanhattan]

Pass to function what the function needs. Pass the ifstream as reference.

so something like:

void counter(ifstream& fromFile){
....do stuff...
}

[andywestken]

Ask user for file name and open file.

Strictly speaking, this needs two functions: a function should ideally "do one thing".

string fileName = AskUserForFileName();

// if successful...

OpenFile(fileName);

(though internally, you should be thinking in terms of file paths rather than names)

So saying, I think it would prob be better to leave this code (which asks the user for a file name and open the file) in main. I would also leave the ifstream's clear() and seekg() in main.

So I see 3 functions -- the last two in your list plus a helper method (for use by the function to write the shfted chars out)

char RotateCharLeft(char ch, int shift) {
	if(islower(ch)){
		ch -=shift;
		if(ch < 'a'){
			ch+=26;
		}
	}
	if(isupper(ch)){
		ch -=shift;
		if(ch < 'A'){
			ch +=26;
		}
	}
	return ch;
}

When it comes to function parameters, the function to calculate the shift should probably have the signature

int func_name(istream& in) // using better names

You can (and should) use istream rather than fstream as your code uses no file specific functionity. This will allow your function to be reused with cin or an istringstream if you wish to do so.

Similarly, the signature for the output function should probably be

void func_name(istream& in, ostream& out, int shift) // using better names

used for the moment like

func_name(fromFile, cout, shift);

But reusable with an ofstream or ostringstream for the output, if you want.

Whatever function names you choose, they should make it clear what they do. So, to me, the name "counter" (which I take to be just for illustrative purposes in ace's post) is both a bit on the terse side and unclear (count what?). A common opinion is that as functions do things, their names should reflect that: i.e. be a verb or verbal phrase. The name should preferably also make it clear what they're doing it to, etc.

(class getter and setter member functions exceptions. Tthey are sometimes just the name of the property they operate on (e.g. std::string::length), though others favour (e.g.) getLength() or similar.)

And while you're refactoring, you should also tighten up the scoping of your variables, for example:

- for loops

for(int index = 0; index < 26; index++) {

- declare variables at first point of use, or as near as possible

(ideally initializng variables it they aren't used immediately)

And consider using ifstream like this:

	char ch = '\0';
	while (is >> ch){
		// use ch...
	}

rather than using eof()

Andy

PS regarding:

Also, this code above is unsatisfactory in that it doesn't "text wrap" when it gets to the screen. Is there something from the iomanip library that can help?

Sadly, std::istream knows nothing about text line wrapping. You'll have to sort out that yourself.

[JLBorges]

> how to access my file from inside a function,

The simplest way would be to pass the file name to the function, and then open the file inside the function.


> I'm still a bit confused about what parameters I would pass through to my functions

This will give you an idea about how to pass information to functions, and how to return information from the function to the caller.

Two caveats:

1. The intent is to illustrate how small functions, each one simple in itself, can be used achieve a large, more complex, objective; not performance. And it is untested code.

2. The code uses many C++ constructs that you may not have encountered so far. Start by focussing on just the structure of the program, what each function gets as input, does, and returns as the result. After that, if you want, you can look into how each function is implemented.

#include <iostream>
#include <string>
#include <algorithm>
#include <iterator>

// read contents of the file into a string
std::string file_to_string( const std::string& file_name ) ;

int shift( const std::string& str ) ; // get the amount to shift by

char do_shift( char c, int shift_by ) ; // shift the character

int main()
{
    std::string file_mame ;
    std::cout << "file name? " ;
    std::cin >> file_mame ;
    std::string contents = file_to_string(__FILE__) ;

    auto shift_by = shift(contents) ; // get the shift amount

    // shift and write to stdout
    std::transform( contents.begin(), contents.end(),
                    std::ostream_iterator<char>(std::cout),
                    [shift_by] ( char c ) { return do_shift(c,shift_by) ; } ) ;
}

#include <fstream>
#include <cctype>
#include <limits>

// read contents of the file into a string
std::string file_to_string( const std::string& file_name )
{
    std::ifstream file(file_name) ;
    file >> std::noskipws ;
    return { std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>() } ;
}

// return a new sanitized string (discard non-alpha, convert to upper case)
std::string sanitize( const std::string& str )
{
    std::string result ;
    for( char c : str ) if( std::isalpha(c) ) result += std::toupper(c) ;
    return result ;
}

// get the most often occurring letter
char most_common( const std::string& str )
{
    std::size_t cnt[ std::numeric_limits<unsigned char>::max() ] = {0} ;
    for( unsigned char c : str ) ++cnt[c] ;
    return *std::max_element( std::begin(cnt), std::end(cnt) ) ;
}

// get the position of a letter in the English alphabet
std::size_t position( char c )
{
    constexpr char a[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" ;
    return std::find( std::begin(a), std::end(a), std::toupper(c) ) - a ;
}

// get the amount to shift by
int shift( const std::string& str )
{
    char c = most_common( sanitize(str) ) ;
    return position('A') + position(c) - position('E') ;
}

// shift the character
char do_shift( char c, int shift_by )
{
    if( std::isalpha(c) )
    {
       if( position( std::toupper(c) ) < shift_by ) shift_by = shift_by - 26 ;
       return c - shift_by ;
    }
    else return c ;
}

[agnophilo]

Couldn't you just make the whole thing one big function and put a function call in main?

Also I've just started working with functions and find they're a nice way to simplify/compartmentalize code so you don't have to hold it all in your head at once. It also makes it easier to go back to the beginning if you want a "try again" option without using goto or making the whole thing one big loop.

[andywestken]

@agnophilo

Couldn't you just make the whole thing one big function and put a function call in main?

Yes, but that wouldn't improve the structure of the program much.

But you then go on to give a good reason not to use a single, big function! :-)

Andy

PS I think that reading the file into a string and then operating the string, as JLBorges code does, is a better implementation. I limited my previous comments to those about refactoring the existing implementation.

Also note that it is possible to chop thing up too much as well as not enough. What is appropriate is very dependent on the context, the key thing being that what's going on in any given function should be immediately clear. You should not have to look at other functions to find out what happens (how it happens is another matter.)

And this is done, ideally, wth minimal use of comments. The code should be written to be self documenting, by the appropriate naming of variables and functions.

[acemanhattan]

@andy @JL

Thanks a lot for the extensive feedback.