Cleaning up this code... options to make it more streamlined

I just finished up my forth project in my C++ class which focused on reading files, writing to them and correcting the errors in the sentence. The code works, all the objectives are complete and it's turned in but I don't think I wrote it as efficiently as I could have. What could I have done differently to make this program more efficient?

wordIn.txt
>EBC;Iit is a Capital Mistake to theorize before one has Data.



1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
#include <fstream>
#include <iostream>
#include <cstdlib>
using namespace std;

void decoder(ifstream& in_stream);
void sentence_fix(ifstream& in_stream, ofstream& out_stream);


int main()
{
	ifstream fin;
	ofstream fout;

	cout << "     Results for Project 4:" << endl << endl;

	fin.open("C:\\wordIn.txt");
	if (fin.fail())
	{
		cout << "\n     Input file opening failed.\n";
		system("pause");
		exit(1);
	}

	fout.open("wordOut.txt");
	if (fout.fail())
	{
		cout << "\n     Output file opening failed.\n";
		system("pause");
		exit(1);
	}

	decoder(fin);
	sentence_fix(fin, fout);
	fin.close();
	fout.close();

	system("pause");
	return 0;
}

void decoder(ifstream& in_stream)
{
	char next;
	char letter;

	cout << "\n     The decoded word is: ";

	for (int y = 0; y < 6; y++)
	{
		in_stream.get(next);
		letter = next + 10;
		cout << letter;
	}

	cout << endl;
}


void sentence_fix(ifstream& in_stream, ofstream& out_stream)
{
	char next;
	char letter;
	int wordCount = 0;

	in_stream.get(next);

	if (islower(next))
	{
		letter = toupper(next);
		out_stream.put(letter);
	}

	in_stream.get(next);

	while (!in_stream.eof())
	{
		if ((isspace(next)) && (in_stream.peek() == 32))
		{
			
		}
		else
		{

			if (isupper(next))
				{
					letter = tolower(next);
					out_stream.put(letter);
				}
				else
				{
					out_stream.put(next);
				}
		}

		if ((!isspace(next)) && ((in_stream.peek() == 32) || in_stream.eof()))
		{
			wordCount++;
		}

		in_stream.get(next);
	}

	cout << endl;

	cout << "     Total words in file = " << wordCount << endl;


}
I'll just give some advice about main function() and return to other later as I do not have time right now to check algorithms in other functions.
1) Open files in filestream constructors: std::ifstream fin("C:\\wordIn.txt");
2) You do not need to close files manually, file stream destructor does it.
3) you do not need to return manually anything from main
4) exit() is tricky. It can lead to loss of data and unexpected results if used carelesly. If you are in main(), you should just return
5) Two conditional operators are mostly the same, you can merge them.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
int main()
{
    std::cout << "     Results for Project 4:\n\n";
    std::ifstream fin("C:\\wordIn.txt");
    std::ofstream fout("wordOut.txt");
    if (not (fin && fout)) {
        if(!fin)  std::cout << "\n     Input  file opening failed.\n";
        if(!fout) std::cout << "\n     Output file opening failed.\n";
        system("pause");
        return EXIT_FAILURE;
    }
    decoder(fin);
    sentence_fix(fin, fout);
    system("pause");
}

In decoder you do not really need two variables, one will be enough:
1
2
3
4
5
6
7
8
9
10
11
void decoder(std::ifstream& in_stream)
{
	char letter;
	std::cout << "\n     The decoded word is: ";
	for (int y = 0; y < 6; y++) 	{
		in_stream.get(letter);
		letter += 10; //increase letter by 10
		std::cout << letter;
	}
	std::cout << '\n';
}
Additionally you can drop middleman altogether:
1
2
3
4
5
6
7
void decoder(std::ifstream& in_stream)
{
    std::cout << "\n     The decoded word is: ";
    for (int y = 0; y < 6; ++y)
        std::cout << char(in_stream.get() + 10);
    std::cout << '\n';
}

Ok, now to the last one.
1) As usual, letter is not needed, operate on next directly, or just drop the middlemn again and outut uppercase letters directly: out_stream.put(toupper(next));
2) Conditions as
1
2
3
if (isupper(next)) {
    letter = tolower(next);
    out_stream.put(letter);
are meaningless: tolower has no effect on characters for which isupper returns false, so you could just do:
1
2
letter = tolower(next);
out_stream.put(letter);
without fears. Combine it with (1) for further improvement.
3)
1
2
3
4
5
in_stream.get(next);
while (!in_stream.eof()) {
    //...
    in_stream.get(next);
}
First of all mandatory notification about dangers of looping on eof. In this case however it will not lead to any error, but it is way better to loop on read operation:
1
2
3
while(in_stream.get(next)) {
    //...
}

4) Avoid conditional operators with empty then block. If needed, just negate codition to move work from else to then. I do not know if you need to write own routine to skip whitespaces or can use standard one, so I leave it at that. Additionally, do not rely on numeric values for specific characters as they are subject to change. Just use character literals.
5) Too many braces on line 96. It is harder to read that way.
In the end:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
void sentence_fix(std::ifstream& in_stream, std::ofstream& out_stream)
{
    int word_count = 0;
    char next;
    in_stream.get(next);
    out_stream.put(toupper(next));

    while (in_stream.get(next)) {
        if (not (isspace(next) && in_stream.peek() == ' '))
            out_stream.put(tolower(next));
        if (!isspace(next) && (in_stream.peek() == ' ' || in_stream.eof()))
            word_count++;
    }
    std::cout << "\n     Total words in file = " << word_count << '\n';
}
Last edited on
Topic archived. No new replies allowed.