4
\$\begingroup\$

My problem is with code about translating a single word into 5 languages

My code:

#include <iostream> #include <map> int dict_number; static const int dict_length = 10; struct Dictionary { std::map<int, std::string> dictionary_no{ {1, "Eple"}, {2, "Banan"}, {3, "Hjem"}, {4, "Hallo"}, {5, "Kafe" }, {6, "Brød"}, {7, "Melk"}, {8, "Hamster"}, {9, "Katt"}, {10, "Hund"} }; std::map<int, std::string> dictionary_en = { {1, "Apple"}, {2, "Banana"}, {3, "Home"}, {4, "Hello"}, {5, "Cafe" }, {6, "Bread"}, {7, "Milk"}, {8, "Hamster"}, {9, "Cat"}, {10, "Dog"} }; std::map<int, std::string> dictionary_pl = { {1, "Jabłko"}, {2, "Banan"}, {3, "Dom"}, {4, "Cześć"}, {5, "Kawa" }, {6, "Chleb"}, {7, "Mleko"}, {8, "Chomik"}, {9, "Kot"}, {10, "Pies"} }; std::map<int, std::string> dictionary_de = { {1, "Apfel"}, {2, "Banane"}, {3, "Haus"}, {4, "Hallo"}, {5, "Kaffee" }, {6, "Brot"}, {7, "Milch"}, {8, "Hamster"}, {9, "Katze"}, {10, "Hund"} }; std::map<int, std::string> dictionary_ua = { {1, "Яблуко"}, {2, "Банан"}, {3, "Дім"}, {4, "Привіт"}, {5, "Кафе" }, {6, "Хліб"}, {7, "Молоко"}, {8, "Хом’як"}, {9, "Кіт"}, {10, "Пес"} }; std::map<int, std::string> dictionary_cn = { {1, "蘋果"}, {2, "香蕉"}, {3, "房子"}, {4, "你好"}, {5, "咖啡"}, {6, "麵包"}, {7, "牛奶"}, {8, "倉鼠"}, {9, "貓"}, {10, "狗"} }; }; Dictionary dictionary; enum Translation { polish, ukrainian, german, english, norwegian, chinese }; void translateWord(Translation translation, Translation translateTo, std::string word) { switch (translation) { case english: for (int search = 0; search < dictionary.dictionary_en.size(); search++) { if (dictionary.dictionary_en[search] == word) { dict_number = search; switch (translateTo) { case german: std::cout << dictionary.dictionary_de[dict_number] << std::endl; break; case norwegian: std::cout << dictionary.dictionary_no[dict_number] << std::endl; break; case polish: std::cout << dictionary.dictionary_pl[dict_number] << std::endl; break; case ukrainian: std::cout << dictionary.dictionary_ua[dict_number] << std::endl; break; case chinese: std::cout << dictionary.dictionary_cn[dict_number] << std::endl; break; } } } break; case german: for (int search = 0; search < dictionary.dictionary_de.size(); search++) { if (dictionary.dictionary_de[search] == word) { dict_number = search; switch (translateTo) { case english: std::cout << dictionary.dictionary_en[dict_number] << std::endl; break; case norwegian: std::cout << dictionary.dictionary_no[dict_number] << std::endl; break; case polish: std::cout << dictionary.dictionary_pl[dict_number] << std::endl; break; case ukrainian: std::cout << dictionary.dictionary_ua[dict_number] << std::endl; break; case chinese: std::cout << dictionary.dictionary_cn[dict_number] << std::endl; break; } } } break; case norwegian: for (int search = 0; search < dictionary.dictionary_no.size(); search++) { if (dictionary.dictionary_no[search] == word) { dict_number = search; switch (translateTo) { case german: std::cout << dictionary.dictionary_de[dict_number] << std::endl; break; case english: std::cout << dictionary.dictionary_en[dict_number] << std::endl; break; case polish: std::cout << dictionary.dictionary_pl[dict_number] << std::endl; break; case ukrainian: std::cout << dictionary.dictionary_ua[dict_number] << std::endl; break; case chinese: std::cout << dictionary.dictionary_cn[dict_number] << std::endl; break; } } } break; case polish: for (int search = 0; search < dictionary.dictionary_pl.size(); search++) { if (dictionary.dictionary_pl[search] == word) { dict_number = search; switch (translateTo) { case german: std::cout << dictionary.dictionary_de[dict_number] << std::endl; break; case norwegian: std::cout << dictionary.dictionary_no[dict_number] << std::endl; break; case english: std::cout << dictionary.dictionary_en[dict_number] << std::endl; break; case ukrainian: std::cout << dictionary.dictionary_ua[dict_number] << std::endl; break; case chinese: std::cout << dictionary.dictionary_cn[dict_number] << std::endl; break; } } } break; case ukrainian: for (int search = 0; search < dictionary.dictionary_ua.size(); search++) { if (dictionary.dictionary_ua[search] == word) { dict_number = search; switch (translateTo) { case german: std::cout << dictionary.dictionary_de[dict_number] << std::endl; break; case norwegian: std::cout << dictionary.dictionary_no[dict_number] << std::endl; break; case polish: std::cout << dictionary.dictionary_pl[dict_number] << std::endl; break; case english: std::cout << dictionary.dictionary_en[dict_number] << std::endl; break; case chinese: std::cout << dictionary.dictionary_cn[dict_number] << std::endl; break; } } } break; case chinese: for (int search = 0; search < dictionary.dictionary_cn.size(); search++) { if (dictionary.dictionary_cn[search] == word) { dict_number = search; switch (translateTo) { case english: std::cout << dictionary.dictionary_en[dict_number] << std::endl; break; case german: std::cout << dictionary.dictionary_de[dict_number] << std::endl; break; case norwegian: std::cout << dictionary.dictionary_no[dict_number] << std::endl; break; case polish: std::cout << dictionary.dictionary_pl[dict_number] << std::endl; break; case ukrainian: std::cout << dictionary.dictionary_ua[dict_number] << std::endl; break; } } } break; } } int main() { translateWord(english, polish, "Hello"); return 0; }

Can this code be improved or shortened? Big thanks for help!

\$\endgroup\$
5
  • 3
    \$\begingroup\$ Unrelated to your question, but the German word for banana is "Banane", not "Banana". \$\endgroup\$ Commented Dec 27, 2021 at 20:45
  • \$\begingroup\$ Oh.. sorry, my bad. \$\endgroup\$ Commented Dec 28, 2021 at 15:38
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ Commented Dec 28, 2021 at 16:48
  • 1
    \$\begingroup\$ Please do not edit the question, especially the code, after an answer has been posted. Changing the question may cause answer invalidation. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered. Also note that the title @200_success provided is more in line with the rules for title on Code Review. \$\endgroup\$ Commented Dec 28, 2021 at 17:04
  • \$\begingroup\$ Okay, understood \$\endgroup\$ Commented Dec 29, 2021 at 11:15

3 Answers 3

Reset to default
6
\$\begingroup\$

Your usage of std::map is wrong here, you don't look-up by index. std::map could be used to select correct words container by language though.

Keeping one container by language, you might have:

#include <algorithm> #include <iostream> #include <array> enum class Language { polish, ukrainian, german, english, norwegian, chinese }; static const int dict_length = 10; // Possibly std::map<Language, std::array<std::string, dict_length>> // instead of named dictionaries. const std::array<std::string, dict_length> dictionary_no{ "Eple", "Banan", "Hjem", "Hallo", "Kafe", "Brød", "Melk", "Hamster", "Katt", "Hund" }; const std::array<std::string, dict_length> dictionary_en = { "Apple", "Banana", "Home", "Hello", "Cafe" , "Bread", "Milk", "Hamster", "Cat", "Dog" }; const std::array<std::string, dict_length> dictionary_pl = { "Jabłko", "Banan", "Dom", "Cześć", "Kawa", "Chleb", "Mleko", "Chomik", "Kot", "Pies" }; const std::array<std::string, dict_length> dictionary_de = { "Apfel", "Banane", "Haus", "Hallo", "Kaffee", "Brot", "Milch", "Hamster", "Katze", "Hund" }; const std::array<std::string, dict_length> dictionary_ua = { "Яблуко", "Банан", "Дім", "Привіт", "Кафе", "Хліб", "Молоко", "Хом’як", "Кіт", "Пес" }; const std::array<std::string, dict_length> dictionary_cn = { "蘋果", "香蕉", "房子", "你好", "咖啡", "麵包", "牛奶", "倉鼠", "貓", "狗" }; const std::array<std::string, dict_length>& get_dictionary(Language language) { switch (language) { case Language::polish: return dictionary_pl; case Language::ukrainian: return dictionary_ua; case Language::german: return dictionary_de; case Language::english: return dictionary_en; case Language::norwegian: return dictionary_no; case Language::chinese: return dictionary_cn; } throw std::runtime_error("Unknown dictionary"); } void translateWord(Language from, Language to, std::string word) { const auto& dict_from = get_dictionary(from); const auto& dict_to = get_dictionary(to); auto it = std::find(dict_from.begin(), dict_from.end(), word); if (it != dict_from.end()) { std::cout << dict_to[std::distance(dict_from.begin(), it)] << std::endl; } } int main() { translateWord(Language::english, Language::polish, "Hello"); return 0; }

Demo

but the fact that order of word should be identical is error prone IMO.

having a struct

struct translated_word { std::string polish; std::string ukrainian; std::string german; std::string english; std::string norwegian; std::string chinese; };

seems a grouping less error prone.

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

translateWord has a lot of code duplication and you also do not use the Key in the map efficiently. I'd probably make a big map with all the languages in it and make it possible to lookup a word from the word index that you've assigned every word and also by making it possible to find the index from a word efficiently, which your current solution doesn't do. That would require a bimap, that is, a map where you can do lookup on both key and value. See boost::bimap. I've added a small bimap class to the answer to show the idea in case you don't have boost.

You may also want to use unordered_maps which may speed things up a little. I'm sticking with map here though.

#include <algorithm> #include <initializer_list> #include <iostream> #include <map> template <class L, class R> struct bimap { // build the bimap bimap(const std::initializer_list<std::pair<L, R>>& il) : left(il.begin(), il.end()) { std::transform(il.begin(), il.end(), std::inserter(right, right.end()), [](auto& p) -> std::pair<R, L> { return {p.second, p.first}; }); } const L& from(const R& r) const { return right.at(r); } // from word to index const R& to(const L& l) const { return left.at(l); } // to word from index std::map<L, R> left; // this implementation stores 2 copies of each L and R std::map<R, L> right; // this implementation stores 2 copies of each L and R };

Your Dictionary class could then be as small as this:

struct Dictionary { // Holds all dictionaries: static const std::map<std::string, bimap<int, std::string>> dict; const std::string& translateWord(std::string from, std::string to, std::string word) { // all the previous loops replaced by this: return dict.at(to).to(dict.at(from).from(word)); } }; 
// Example with three of the languages const std::map<std::string, bimap<int, std::string>> Dictionary::dict{ {"Norwegian", { {1, "Eple"}, {2, "Banan"}, {3, "Hjem"}, {4, "Hallo"}, {5, "Kafe"}, {6, "Brød"}, {7, "Melk"}, {8, "Hamster"}, {9, "Katt"}, {10, "Hund"}, }}, {"English", { {1, "Apple"}, {2, "Banana"}, {3, "Home"}, {4, "Hello"}, {5, "Cafe"}, {6, "Bread"}, {7, "Milk"}, {8, "Hamster"}, {9, "Cat"}, {10, "Dog"}, }}, {"Polish", { {1, "Jabłko"}, {2, "Banan"}, {3, "Dom"}, {4, "Cześć"}, {5, "Kawa"}, {6, "Chleb"}, {7, "Mleko"}, {8, "Chomik"}, {9, "Kot"}, {10, "Pies"}, }}, } 
int main() { Dictionary dict; std::cout << dict.translateWord("English", "Polish", "Hello") << '\n'; std::cout << dict.translateWord("Polish", "Norwegian", "Chleb") << '\n'; }

Demo

\$\endgroup\$
1
  • \$\begingroup\$ @Random19 You're welcome! \$\endgroup\$ Commented Dec 27, 2021 at 13:36
0
\$\begingroup\$

Have you noticed that the translateTo part of each case is identical? Don't copy/paste large blocks of code. Put it in one place, and refer to it multiple times instead.

More subtly, the first part of each case is identical logic but varies only which dictionary is searched. Again, this is duplicated code but this time with a parameter. Make it common code with a parameter for the part that changes.

If you play around with it, you might get it down to a single case that doesn't even "repeat" via calling. But as a starting point to the conceptual ideas, start by not repeating yourself. Then, in the simpler code that remains, you can find more things that are the same.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.