5
\$\begingroup\$

I have implemented the following program below for an assignment for practice on STL templates and algorithms. All I'm doing is implementing the code for the printing of empty files, un-empty files, etc.. I would love to know if there is any way to make the code more optimized.

Note: From the requirements of my school (Must be adhered to):

  • Code with the comment "helper function" should not be modified. These are empty_check(), split(), and print_filename().

  • Functions commented as "Can modify" means the code within that function can be modified.

  • int main() should not be modified

  • No extra headers allowed.

  • No definition of any new complex types or templates

  • No using of other functions other than the helper functions.

  • No use of lambda expressions.

  • No use of operators:

    • . (member access)
    • -> (member access via a pointer)
    • * (dereference).
  • No use of explicit iteration (for, while, do while) or selection (if, switch, ?:) statements or operators.

  • No use of keyword auto.

  • No use of std::cout, std::cerr or any other functions that perform printing of the text. I'll have to use the provided helper function to do it.

#include <iostream>
#include <vector>
#include <map>
#include <string>
#include <algorithm>
#include <functional>
#include <utility>

using name_file = std::string;
using sizeFile = size_t;
using record_in_file = std::pair<name_file, sizeFile>;
using file_datas = std::map<name_file, sizeFile>;

bool empty_check( //helper function
    const record_in_file& dataRecord,
    bool true_if_its_not
)
{
    sizeFile size;
    std::tie(std::ignore, size) = dataRecord;
    bool result = (size == 0);
    if (true_if_its_not)
    {
        result = !result;
    }
    return result;
}

name_file split(const record_in_file& dataRecord) //helper function
{
    name_file name;
    std::tie(name, std::ignore) = dataRecord;
    return name;
}

void print_filename(const name_file& name1) //helper function
{
    std::cout << " * " << name1 << std::endl;
}

void file_names_print(const file_datas& map1) //can modify 
{
    std::vector<name_file> files;
    std::transform(std::begin(map1), std::end(map1), std::back_inserter(files), split);
    std::for_each(std::begin(files), std::end(files), print_filename);
}

size_t files_un_empty_print(const file_datas& map1) //can modify
{
    std::vector<record_in_file> files;
    std::copy_if(std::begin(map1), std::end(map1), std::back_inserter(files),
        std::bind(empty_check, std::placeholders::_1, true));
    std::vector<name_file> file_names;
    std::transform(std::begin(files), std::end(files), std::back_inserter(file_names),
        split);
    std::for_each(std::begin(file_names), std::end(file_names), print_filename);

    return std::count_if(std::begin(map1), std::end(map1),
        std::bind(empty_check, std::placeholders::_1, true));
}

size_t files_empty_print(const file_datas& map1) //can modify
{
    std::vector<record_in_file> files;
    std::copy_if(std::begin(map1), std::end(map1), std::back_inserter(files),
        std::bind(empty_check, std::placeholders::_1, false));

    std::vector<name_file> file_names;
    std::transform(std::begin(files), std::end(files), std::back_inserter(file_names),
        split);

    std::for_each(std::begin(file_names), std::end(file_names), print_filename);

    return std::count_if(std::begin(map1), std::end(map1),
        std::bind(empty_check, std::placeholders::_1, false));
}

std::tuple<file_datas&>  get_param(file_datas& map1) //can modify
{
    return std::forward_as_tuple<file_datas&>(map1);
}

void empty_removal(file_datas& map1) //can modify
{
    std::vector<record_in_file> files;
    std::copy_if(std::begin(map1), std::end(map1), std::back_inserter(files),
        std::bind(empty_check, std::placeholders::_1, true));

    file_datas n_map{ std::begin(files),std::end(files) };

    std::swap(map1, n_map);
}


int main()
{
    file_datas map = {
        {"readme.txt", 2000},
        {"main.exe", 10000},
        {"save.bak", 0},
        {"library.dll", 1243},
        {"0.res", 121100},
        {"1.res", 121100},
        {"2.res", 115600},
        {"errors.log", 0}
    };

    std::cout << "Files:" << std::endl;
    file_names_print(map);
    std::cout << std::endl;

    std::cout << "Files that are not empty:" << std::endl;
    size_t Count_of_unemptyFiles = files_un_empty_print(map);
    std::cout
        << " There are "
        << Count_of_unemptyFiles
        << " non-empty files.\n"
        << std::endl;

    std::cout << "Files that are empty:" << std::endl;
    size_t Count_of_emptyFiles = files_empty_print(map);
    std::cout
        << " There are "
        << Count_of_emptyFiles
        << " empty files.\n"
        << std::endl;

    std::cout << "Files after removing the empty ones:" << std::endl;
    auto parameters = get_param(map);
    std::apply(empty_removal, parameters);
    file_names_print(map);
}
```
\$\endgroup\$
3
\$\begingroup\$

Avoid creating temporary vectors

You should avoid creating unnecessary temporary vectors. For example, in file_names_print(), you can use a nested std::bind() to avoid the vector files:

void file_names_print(const file_datas& map1) //can modify 
{
    std::for_each(std::begin(map1), std::end(map1),
                  std::bind(print_filename, std::bind(split, std::placeholders::_1)));
}

This can be done with all the std::transform() + std::for_each() combinations. Given the restrictions you have, I don't see how to avoid the temporary vectors for std::copy_if.

Avoid unnecessary counting

In those cases where you used std::copy_if(), you then no longer need to call std::count_if() to count matching elements in the original input, you can just get the std::size() of the temporary vector. For example:

size_t files_un_empty_print(const file_datas& map1) //can modify
{
    std::vector<record_in_file> files;
    std::copy_if(std::begin(map1), std::end(map1), std::back_inserter(files), std::bind(empty_check, std::placeholders::_1, true));
    std::for_each(std::begin(files), std::end(files), std::bind(print_filename, std::bind(split, std::placeholders::_1)));

    return std::size(files);
}

Removing elements from a container

If you can use C++20, then you would simply write:

void empty_removal(file_datas& map1) //can modify
{
    std::erase_if(map1, std::bind(empty_check, std::placeholders::_1, true));
}

If you cannot use C++20, then the typical way would be to use a for-loop that calls erase() on matching elements. Of course, you are restricted from doing that, and then you indeed have to make a copy.

Copy directly into a std::map

In empty_removal(), you first create a std::vector of files, and then convert that to a map. That can be avoided by creating an empty std::map and inserting in that instead:

void empty_removal(file_datas& map1) //can modify
{
    file_datas files;
    std::copy_if(std::begin(map1), std::end(map1),
                std::inserter(files, std::end(files)),
                std::bind(empty_check, std::placeholders::_1, true));

    std::swap(map1, files);
}

About those restrictions

It might be a way to force you to use STL algorithms, but there are several drawbacks. Unfortunately, due to the fact you have to pass multiple iterators to STL algorithms, and never get a container as a return value, it is very hard to compose multiple STL algorithms. You then have to use intermediate copies, which is very inefficient. A range-based for loop is then usually clearer and more efficient. In the real world, you would be able to use all the tools that C++ provides, and select the ones that are most appropriate to the task. You would likely combine for-loops, algorithms and lambdas.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Hey, many thanks for the much needed suggestions! Anyway, for the 'empty_removal()", std::inserter is part of #include<iterator> header which is not covered in any of the headers above. No extra headers allowed. Any other substitute for std::inserter? I tried back_inserter intuitively, but of course, it doesn't work for all container types, std::map, etc.. \$\endgroup\$ – starrk Aug 11 at 3:56
  • 1
    \$\begingroup\$ @starrk It compiles for me without explicitly including <iterator> when using GCC (version 10.2.0) or Clang (version 10.0.1). And you were already using std::back_inserter, which is also part of <iterator>. Technically, if you are not able to add #include <iterator>, then you cannot safely use any iterator class. If it works it is only through luck that another header includes it for you. \$\endgroup\$ – G. Sliepen Aug 11 at 7:00
3
\$\begingroup\$

Apologies for being a bit brief with this review, however, my time is limited today. My review is a bit less technical than G. Sliepen's and is focused more on code style.

Prefer consistent names

Looking at the first two lines after the list of includes:

using name_file = std::string;
using sizeFile = size_t;
...

Pick a style (two_words, twoWords, TwoWords, etc.) and stick with it throughout your code.

Choose names which improve code readability and follow proper grammar

size_t files_un_empty_print(const file_datas& map1);  // what does "files_un_empty" mean?
void file_names_print(const file_datas& map1);        // what is "map_1"?
bool empty_check(
    const record_in_file& dataRecord,
    bool true_if_its_not // "is_empty" is concise and better communicates the intent
);
void empty_removal(file_datas& map1);  // "remove" or "delete_empty_files" are clearer

For the second point about proper grammar, this may be an insignificant nitpick on my part, however, if you can use proper English grammar when choosing names, do so:

using name_file = std::string;  // "file_name" is a more natural and familiar way to say the same thing
using file_datas = std::map<name_file, sizeFile>;  // the word "data" is plural

There may be additional examples in your code but this should illustrate the point.

Using declarations

You created an alias:

using sizeFile = size_t;

and then reverted to using size_t:

size_t files_un_empty_print(const file_datas& map1);
| improve this answer | |
\$\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.