4
\$\begingroup\$

This asks the user for the file name and the number of lines they want to remove. It then stores each line into a vector. A dynamic array is used to hold the random line numbers (generated by random generator) and the corresponding element in the vector is deleted. This vector is then copied to a new file named "temp.txt" where each element is its own line.

Questions:

  • What can I improve to make this code for efficient? i.e. Better methods to perform this task?
  • What should I not use and should change (for example any bad habits)?
  • What did I use that is good and should keep it as a habit?
  • Are there any exceptions I can use to ensure code?

Warnings I received:

  1. Warning C26495 Variable 'LineEditor::array' is uninitialized. Always initialize a member variable (type.6). for LineEditor().
  2. Avoid unnamed objects with custom construction and destruction (es.84). for getFileName() in void removeLines().
#include <iostream>
#include <string>
#include <fstream>
#include <random>
#include <ctime>
#include <cstring>
#include <vector>
#include <algorithm>
#include <iterator>

template<typename T>
void writeTo(const std::string& filename, std::vector<T>& v)
{
    std::ofstream fstream(filename);
    if (!fstream)
        exit(1);
    std::copy(v.begin(), v.end(), std::ostream_iterator<T>(fstream, "\n"));
    fstream.close();
}

class LineEditor
{
    std::vector<std::string> buf;
    std::string fileName;
    int noOfLines{ 0 };
    int* array;
public:
    LineEditor()
    {
    }

    ~LineEditor()
    {
        std::cout << "\"" << fileName << "\"" << " " << noOfLines << " lines removed.\n";
        delete[] array;
    }

    void removeLines()
    {
        getFileName();
        TotalLinesToRemove();

        std::ifstream fileIn;
        std::string line;
        int lineCounter{ 0 };
        int* array = new int[noOfLines];

        fileIn.open(fileName);
        if (!fileIn)
        {
            std::cerr << fileName << " can not be opened." << std::endl;
            exit(1);
        }
        while (fileIn.is_open())
        {
            while (std::getline(fileIn, line))
            {
                lineCounter++;
                buf.push_back(line);
            }
            for (int i{ 0 }; i < noOfLines; ++i)
            {
                array[i] = getRandomNumber(0, lineCounter - 1);
                buf.erase(buf.begin() + (array[i]));
                --lineCounter;
            }
            writeTo("temp.txt", buf);
            fileIn.close();
        }
    }
private:
    std::string getFileName()
    {
        std::cout << "Enter name of text file: ";
        std::cin >> fileName;

        return fileName;
    }
    int TotalLinesToRemove()
    {
        std::cout << "Enter the number of lines to remove: ";
        std::cin >> noOfLines;

        return noOfLines;
    }
    int getRandomNumber(int min, int max)
    {
        std::mt19937 seed{ static_cast<std::mt19937::result_type>(std::time(nullptr)) };
        std::uniform_int_distribution<> rand(min, max);
        return rand(seed);
    }
};

int main()
{
    LineEditor file;
    file.removeLines();
    return 0;
}

I am fairly beginner to intermediate at C++ and want to improve.

\$\endgroup\$
3
\$\begingroup\$

Use the force library Luke!

Your code does use std::vector, but for reasons I don't understand, it also uses new to allocate a manually managed dynamic array as well. You don't seem to gain anything from this, so I'd advise using std::vector throughout.

I'd also look at the standard library's algorithms. Some of them (e.g., std::remove_copy_if) could be put to excellent use for this task.

Avoid std::endl

std::endl not only writes a new-line to a stream (which is what you usually want) but also flushes the stream (which you usually don't want). If you do that very often, it can slow a program significantly.

In your case, you using it when writing to std::cerr, which automatically flushes in any case, so the flushing done by std::endl is unnecessary in any case.

Random generator seeding

When you're using <random> (a good idea) the generally accepted way of seeding a generator is by using std::random_device. This is usually a truly random device (e.g., reading from /dev/random) so it normally does a substantially better job of seeding than using the system time.

As @1201ProgramAlarm already pointed out, you normally want to seed a generator exactly once, then just generate numbers from it.

Unfortunately, doing a good job of seeding a random number generator is a somewhat nontrivial task. I posted some code that demonstrates what I believe is one reasonable possibility in a previous answer. The short summary is that regardless of whether you use time(nullptr) or std::random_device, if you only use a 32-bit seed, you're limiting yourself to only \$2^{32}-1\$ possible sequences that you can generate, which is a huge limitation compared to the full capabilities of MT19377.

Single Responsibility Principle

It seems to me that your LineEditor does more than I'd like a single class to do. For most practical purpose, the entire program is embodied in that one class. That doesn't strike me as entirely ideal. I'd rather see individual pieces responsible for the individual parts of doing the job.

This also fits better with having clearly defined layers of abstraction. For example, you might have one layer that deals with the file name, and a separate one that deals with the actual content of the file.

Convenient Interface

For most uses, I'd rather run the program something like removeLines foo.txt 10, rather than doing removeLines, then having to separately walk through a questionairre (so to speak) to tell it the name of the file and number of lines to remove. Of course, depending on the situation, that might be better as something like removeLines -f foo.txt -n 10 (especially if this were part of a larger program that might have more/other command line arguments as well.

If you are going to ask questions interactively, I'd ask for the filename first, then read in the file, then ask for the number of lines to remove. This will allow you to check how many lines you've actually read, which (in turn) you can use to advise the user as to the maximum number of lines they can have removed (e.g., "Please enter the number of lines to remove (1-723): ").

Constructor Definition

You've currently defined your constructor with no member initializer list and nothing in the body. It's not clear what you're hoping to accomplish by defining it explicitly rather than just accepting what the compiler would define if you didn't define one at all. Until/unless your ctor will actually accomplish something, it's probably better not to define it explicitly.

Possible Code

#include <iostream>
#include <vector>
#include <string>
#include <set>
#include <fstream>
#include <iterator>
#include "rand.h"

struct InputSpec {
    std::string filename;
    std::size_t count;

    InputSpec(int argc, char **argv) { 
        if (argc != 3)
            throw std::runtime_error("Usage: removeLines <filename> <count>");

        filename = argv[1];
        count = std::atoi(argv[2]);

        if (count == 0)
            throw std::runtime_error("numbers of lines to remove can't equal 0");
    }
};

int main(int argc, char **argv) { 
    InputSpec input{argc, argv};

    std::ifstream infile(input.filename);
    std::vector<std::string> lines;  
    std::string line;
    while (std::getline(infile, line))
        lines.push_back(line);

    int maxLines = std::min(input.count, lines.size());

    generator g(maxLines);
    std::set<int> removals;

    while (removals.size() < maxLines)
        removals.insert(g());

    int current_line = 0;
    std::remove_copy_if(lines.begin(), lines.end(), 
        std::ostream_iterator<std::string>(std::cout, "\n"),
        [&](std::string const &) { return removals.find(++current_line) != removals.end(); });
}

Note that for the moment, this presumes that the number of lines to be removed from the file is small compared to the number of lines in the file. If there's a significant chance that won't be true, you may want to see the selection algorithm in yet another old answer.

\$\endgroup\$
1
\$\begingroup\$

Since writeTo does not modify v, it should take that parameter as a const reference (const std:::vector<T>&v).

In LineEditor, the constructor doesn't initialize ary, so if you construct a LineEditor object and destroy it without calling removeLines (or if that function returns, possibly by an exception, before you allocate the memory) you'll delete an uninitialized pointer. Those problems can be avoided by using std::vector<int> instead of a raw int pointer. And since ary is only used within one function (other than deleting the allocated memory), it shouldn't be a class member but should be a local variable within the function.

The removeLines function is doing several things. It asks for a couple of inputs, then opens the file and removes the lines. This can be split into two (or more) functions, one to ask for input and another to do the actual removal. while (fileIn.is_open()) can be an if instead since it will be true only once, if at all.

What will happen if the user wants to delete more lines than the file contains?

Deleting lines like you do can be inefficient because erase will move all the entries in the vector after the deleted entry. If you need to process larger files or deleting a large portion of the lines you may want to use a method that will only move the lines that are not deleted once, although doing that can introduce other problems to avoid.

And then there's getRandomNumber. The proper way to use the random number generator is to only create the engine (seed) once. Since the seed is recreated on every function call (a time consuming process), probably within a very small time frame, the value returned by std::time is very likely to be the same, resulting in the same generated random number. Although the returned number will be different with how you're using it (since the range is different every time), this is still bad practice. Common solutions to this are to either have a static local variable with the engine, or (preferably) make seed a member of the class, so every instantiation of the class will have its own engine that is only created once. The distribution object (rand) should remain a local since it can be different on every call.

\$\endgroup\$
  • \$\begingroup\$ Thanks. Could you show what the code for the getRandomNumber should look like? \$\endgroup\$ – YaniAR Sep 9 at 7:00

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.