HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

gGgGgG translator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
ggggggtranslatorstackoverflow

Problem

This is basically a program that takes the first line of a file and makes a map using ggGgg h gGgGg s makes ggGgg equals h and gGgGg equals s. Now the rest of the file makes sentences with these. Idea is from here.

```
#include
#include
#include
#include
#include
#include
#include
#include

std::map makeMap(std::ifstream&);
const std::string makeWord(const std::string&, const std::map&);

void translate(std::ifstream &file) {
auto transMap = makeMap(file);
std::string line;
while (std::getline(file, line)) {
bool firstWord = true;
std::string word;
std::istringstream wordIN(line);
while (wordIN >> word) {
if (firstWord)
firstWord = false;
else
std::cout makeMap(std::ifstream &toTrans) {
std::map transMap;

std::string line;
std::getline(toTrans, line);
std::istringstream input(line);

std::string key;
char value;

while (input >> key) {
if (key.find_first_not_of("Gg") != std::string::npos)
throw std::runtime_error("You can only use g's");
if (key.size() != 5)
throw std::runtime_error("The key is not equal to 5");

if (input >> value)
transMap[key] = value;
else
throw std::runtime_error("No value for the key");
}
return transMap;
}

const std::string makeWord(const std::string &word, const std::map &m) {
std::string oldWord, newWord;
for (auto letter = word.cbegin(); letter != word.cend(); ++letter) {
if (!std::ispunct(*letter)) {
oldWord.push_back(*letter);
if (oldWord.size() == 5) {
if (m.find(oldWord) != m.cend()) {
newWord.push_back((m.find(oldWord)->second));
oldWord.clear();
}
else
throw std::runtime_error("Couldnt find word");
}
}

Solution

Handle singular cases outside of the loop

The firstWord variable is there to handle a single word, and complicates the whole loop (and makes it a tiny bit slower, but that is secondary):

while (std::getline(file, line)) {
    bool firstWord = true;
    std::string word;
    std::istringstream wordIN(line);
    while (wordIN >> word) {
        if (firstWord)
            firstWord = false;
        else
            std::cout << " ";
        std::cout << makeWord(word, transMap);
    }
    std::cout << "\n";
}


I would write it like:

while (std::getline(file, line)) {
    std::string word;
    std::istringstream wordIN(line);

    wordIN >> word;
    std::cout > word) {
        std::cout << " ";
        std::cout << makeWord(word, transMap);
    }
    std::cout << "\n";
}


This way the code is more simple because the special case is clearly handled alone out of the loop.

Algorithm / IO separation

All your functions (except main) are void, so their only purpose is modifying the outside world. This code is mainly about solving a conceptual (algoritmhic) problem, so it would benefit greatly if logic and Input Output were separated, because testing would become much easier.

Code Snippets

while (std::getline(file, line)) {
    bool firstWord = true;
    std::string word;
    std::istringstream wordIN(line);
    while (wordIN >> word) {
        if (firstWord)
            firstWord = false;
        else
            std::cout << " ";
        std::cout << makeWord(word, transMap);
    }
    std::cout << "\n";
}
while (std::getline(file, line)) {
    std::string word;
    std::istringstream wordIN(line);

    wordIN >> word;
    std::cout << makeWord(word, transMap);
    while (wordIN >> word) {
        std::cout << " ";
        std::cout << makeWord(word, transMap);
    }
    std::cout << "\n";
}

Context

StackExchange Code Review Q#123280, answer score: 2

Revisions (0)

No revisions yet.