13
\$\begingroup\$

Another more simple project from my CS1 class that I didn't upload while I was sick last week:

Write a program that computes all of the following statistics for a file and outputs the statistics to both the screen and to another file: the total number of occurrences of characters in the file, the total number of non-whitespace characters in the file, and the total number of occurrences of letters in the file.

I must use at least two functions in this project. In addition, I am also supposed to count and output the number of occurrences of each character.

fileStats.cpp:

/**
 * @file fileStats.cpp
 * @brief
 * @author syb0rg
 * @date 10/23/14
 */

#include <iostream>
#include <sstream>
#include <fstream>

constexpr int ARRAY_LENGTH = 128;

struct Data
{
    size_t total = 0;
    size_t nonWhite = 0;
    size_t letters = 0;
    size_t charCount[ARRAY_LENGTH] = {0};
};

std::string constructString(Data data)
{
    std::stringstream sstm;
    sstm << "Number of characters: " << data.total << "\n";
    sstm << "Number of non-whitespace characters: " << data.nonWhite << "\n";
    sstm << "Number of letters: " << data.letters << "\n" << "\n";

    sstm << "Occurances of each character: " << "\n";
    for (int i = 0; i < ARRAY_LENGTH; ++i)
    {
        if(data.charCount[i]) sstm << (char) i << ": " << data.charCount[i] << "\n";
    }
    return sstm.str();
}

void outputToFile(std::string fileName, std::string str)
{
    std::ofstream out(fileName);
    out << str;
    out.close();
}

int main()
{
    Data data;    
    std::ifstream file("test.txt");
    char input = '\0';

    while ((input = file.get()) && file.good())
    {
        data.total++;
        data.charCount[input]++;
        if(!isblank(input)) data.nonWhite++;
        if(isalpha(input)) data.letters++;
    }
    file.close();

    std::string result = constructString(data);
    outputToFile("output.txt", result);
    std::cout << result;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Too small: ARRAY_LENGTH = 128;. Otherwise you need to validate that your character is in the correct range. \$\endgroup\$ – Martin York Nov 4 '14 at 21:50
  • 1
    \$\begingroup\$ This sounds like it could be a TV show? \$\endgroup\$ – Ethan Bierlein Nov 5 '14 at 1:52
6
\$\begingroup\$

Loop over a file character by character.

std::for_each(std::istreambuf_iterator<char>(file),
              std::istreambuf_iterator<char>(),
              [](char c)
              {
                   // DO Stuff
              });

Notice: std::istreambuf_iterator rather than std::istream_iterator to prevent white space being skipped.

Why are you treating a file output differently to output to std::cout?

outputToFile("output.txt", result);
std::cout << result;

// Why not.

std::ofstream file("output.txt");
file      << result;
std::cout << result;

Also its not as if the output is huge and needs buffering in a string. Just ouput the data object.

std::ofstream file("output.txt");
file      << data;
std::cout << data;
\$\endgroup\$
7
\$\begingroup\$
  • You can pass data by const& as it's not being modified.

  • ARRAY_LENGTH should probably be an unsigned type, such as std::size_t.

  • Just to stay safe, terminate the program if the file cannot be opened:

    if (!file)
    {
        std::cerr << "file cannot be opened";
        return EXIT_FAILURE;
    }
    
  • You do not really need to call close() here. It would only be necessary for checking for errors, otherwise the destructor will do this for you. It shouldn't leak any resources.

\$\endgroup\$
6
\$\begingroup\$
  • Using isblank is a grey area: in C locale you'd be counting newlines as non-whitespace. I think testing for isspace is more in line with the assignment.

  • The while loop in main deserves its own function (which in turn feels like a method of Data).

  • It feels that constructString is really a method of Data (e.g. Data::toString()).

  • Shouldn't isblank and isalpha be quialified as std::?

\$\endgroup\$
5
\$\begingroup\$

You use C++-style input/output, but your design is fundamentally C.

An idiomatic C++ solution would have a smart class rather than a dumb struct. Something like

class FileStats {
  public:
    FileStats();

    /* Called once for each character in the file */
    void seen(char c);

    /* Retrieve the statistics */
    unsigned int getTotal();
    unsigned int getNonWhite();
    unsigned int getCount(char c);
};
\$\endgroup\$
  • 3
    \$\begingroup\$ Boo. Getters. Boo. Breaks encapsulation. Write an output operator instead. \$\endgroup\$ – Martin York Nov 4 '14 at 22:01
  • 2
    \$\begingroup\$ @LokiAstari If you want the reporting format to be a fixed template, sure. That hurts reusability, in my opinion. \$\endgroup\$ – 200_success Nov 4 '14 at 22:02
  • 4
    \$\begingroup\$ @200_success Somewhat esoteric, maybe for a larger program, or a library, but everything does not have to go in class just because it is C++, it is fine to use C style code with STL if its something small. \$\endgroup\$ – The Floating Brain Nov 5 '14 at 0:40
5
\$\begingroup\$

Small point.

This could be written more simply:

while ((input = file.get()) && file.good())
{
    // ...
}
file.close();

as:

while(file.get(input))
{
    // ...
}
file.close();
\$\endgroup\$
  • \$\begingroup\$ Why remove the file.good() check? \$\endgroup\$ – syb0rg Nov 10 '14 at 14:22
  • 1
    \$\begingroup\$ @syb0rg file.get() returns the input stream which is then converted to a bool which is tested by the loop. It pretty much checks the same error flags as file.good(). See Here. \$\endgroup\$ – Galik Nov 10 '14 at 16:56
4
\$\begingroup\$

There is no guarantee that input will match up within the range of ARRAY_LENGTH depending on what is coming from stdin, and from a standard perspective the data may also be up to further interpretation (again depending on what is coming from stdin, you could be working on a system which stores data in a binary format in a hex file in all situations, and you should to be able to account for that, although maybe not to such a ridiculas extend, especially in this program, just keep it in mind and don't assume it will be ASCII), I also think you should do an explicit cast to make the code more clear, overall I would critique your use of data.charCount[input]++; and would suggest the use of an std::map or setting up the array before hand, then doing this with a bounds check, and seeing if input holds a valid charterer.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.