# Count the frequencies of each word in a file

I have tried to implement a code that does the following:

1. store words in an array (unordered)
2. use function order_array() to place repetitions for the same word near (so the result, given a sample text file, could be: mom mom mom mom lamb lamb eye eye pain tears)
3. use function count_frequency to store every word one single time + its frequency (I store them in a linked list)
4. print the linked list to obtain given result

The code works, but it appears (to my eye) cumbersone and unefficient. How can I improve it?

Note that I am not familiar with concepts like vectors, hash maps or dictionaries yet, but I have a good grasp of basic data structures (eg linked lists, stacks, queues, binary search trees). Still, any hint, help or advice would be greatly appreciated

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

const int DIM = 50;
int count = 0;
string list[DIM];
string ordered_list[DIM];

struct node
{
string word;
int frequency;
node* next;
};

node* remove_node(node* &h)
{
return s;       // and then remove everything
}

void add_node(string &w, int &f, node* &h)
{
node* t = new node;
t->word = w;
t-> frequency = f;
t->next = h;
h = t;
}

void print_highestf(node* &h)
{
node* s = h;
int higherf = 0;
while(s!=NULL)
{
if((s->frequency) >= higherf)
{
higherf = s->frequency;
}
s = s->next;
}
cout << "The highest frequency for a word is: " << higherf << endl;
}

void print_res(node* &h)
{
for(node* s = head; s != NULL; s = s->next)
{
cout << "Frequency for word  " << s->word << ": " << s->frequency << endl;
cout << "\n";
}
}

{

a[count] = w;
count ++;

}

bool is_new_string(string &word, int position)      //check better
{
bool repeated = false;
for(int i = 0; i < position; i++)
{
if(word == list[i] && repeated == true)
{
return false;
}
else if(word == list[i] && repeated == false)
{
repeated = true;
}
}
return true;
}

void order_array(string a[], string oa[])
{
int i = 0;
int times = 0;
while(i!= count)
{
int j = i;
while(j != count)
{
if((a[i] == a[j]) && is_new_string((a[i]), i+1))
{
//new node
oa[times]=a[i];
times++;
}

j++;
}
i++;
}
}

void count_frequency(string oa[])
{
int frequency = 1;
for(int i = 0; i<count-1; i++)
{
if(oa[i] != oa[i+1])
{
frequency = 1;
}
else if(oa[i] == oa[i+1])
{
frequency++;
}
}
}

int main()
{
fstream mystream;
mystream.open("words.txt", ios::in);
string word;

while(!mystream.eof())
{
mystream >> word;

}

order_array(list,  ordered_list);
count_frequency(ordered_list);

for(node* s= head; s!=NULL; s = s->next)
{
delete r;
}
cout << "\nDeinitialization executed. " << endl;
mystream.close();
return 0;
}



Kudos on writing a working program! You already sense that things can be improved, which is also great. Toby Speight already pointed out some issues in your code. You are a beginning C++ programmer, so it's normal that you don't write optimal code yet; you have just learned the basic tools. To show you how a more experienced C++ programmer would tackle the problem of counting frequencies, let me show you this implementation:

#include <cstdlib>
#include <algorithm>
#include <format>
#include <fstream>
#include <iostream>
#include <ranges>
#include <unordered_map>
#include <vector>

// A struct to hold a value and its frequency
template<typename T>
struct counted_value {
std::size_t count;
T value;

// Add a default comparison operator so we can sort on these structs
friend auto operator<=>(const counted_value&, const counted_value&) = default;
};

// A templated function that can take any kind of input
template<std::ranges::input_range Range>
auto count_frequencies(Range&& range) {
// Deduce the type of values stored in the range
using value_type = decltype(std::ranges::begin(range))::value_type;

// Count frequencies in a hash map
std::unordered_map<value_type, std::size_t> counters;

for (const auto& value: range) {
counters[value]++;
}

// Copy the result into a vector
std::vector<counted_value<value_type>> result;
result.reserve(counters.size());

for (const auto& [value, count]: counters) {
result.emplace_back(count, value);
}

// Sort the vector on the frequency count, largest count first
std::ranges::sort(result, std::greater{});

return result;
}

int main() {
std::ifstream input("words.txt");

// Create a view of the words in the input stream
auto words = std::ranges::istream_view<std::string>(input);

auto frequencies = count_frequencies(words);

// We should have reached the end, if we didn't something went wrong
if (!input.eof()) {
return EXIT_FAILURE;
}

for (const auto& item: frequencies) {
std::cout << std::format("Frequency for word {}: {}\n",
item.value, item.count);
}
}


I've used a lot of features from the standard library and from the language itself, in particular features from C++20. I'll list them here with links to cppreference.com:

Don't worry about all of this, just continue your programming journey, and eventually you learn most or all of the above.

• Thank you so much for your detailed answer. There is still a lot to go but your precise explanation helped me seeing what I need to learn in order to improve! Feb 4 at 13:00

using namespace std;


That immediately takes away the benefit of namespaces, making it harder to be sure which namespace identifiers come from. It's particularly harmful when you declare names of your own (e.g. list and next) that are the same as those in std.

This is an anti-pattern:

while(!mystream.eof())
{
mystream >> word;

}


The EOF flag is set on a stream when input has failed due to reaching the end. It does not guarantee that the next read will succeed. What we want is

    while(mystream >> word)
{
}


Whilst implementing your own linked list may be an instructive experience, I think it's more useful to familiarise yourself with the standard library containers. As a C++ programmer, you'll be using these on a daily basis, whereas you may never need to implement one of your own.

This program would be much, much simpler if we used one of the standard map classes (std::map<std::string, std::size_t> if we want the results to be in a repeatable order, or the std::unordered_map equivalent if not). And we wouldn't have any raw pointers, so no need to verify that every new is matched with a delete.

The program would be more useful if it read from std::cin rather than requiring a specifically-named file for its input. That allows the user to redirect from any file, or from a pipe, or even interactively.

The end of the program has some unnecessary code:

cout << "\nDeinitialization executed. " << endl;
mystream.close();
return 0;


There's no need to flush the output stream, given that happens automatically at program termination, so we can remove the std::endl and simply add \n to the end of the string literal.

The destructor of mystream will automatically close the file, so that's unnecessary unless we're actually going to take any notice of the result.

Executing off the end of main() (but no other function) is defined as returning 0 (success), so the return can be omitted.

• Thank you very much for your answer, I have learnt something new and useful. I will treasure what you have said ! Feb 4 at 12:54