5
\$\begingroup\$

I've tried to make a little library in C++, to gather all useful and necessary functions. I wrote the code(in one file, to make it more portable). Unfortunately, it works on Windows only.

#pragma once
#ifndef NEWCPP
    #define NEWCPP

    #include <iostream>
    #include <windows.h>
    #include <string>
    #include <ctype.h>
    #include <stdio.h>
    #include <fstream>
    #include <cctype>
    #include <algorithm>
    #include <time.h>
    #include <sstream>

    #include <sys/stat.h>
    #include <vector>
    #include <stdexcept>

    #include <cstdarg>

    using namespace std;

    HANDLE hConsole = GetStdHandle(STD_OUTPUT_HANDLE);
    
    namespace newcppPrivate {
        void Init() {
            srand(time(0));
        }
        const int fileInitializeStartTime = (int)time(0);
        #define STARTTIME fileInitializeStartTime
        #define PRESSTIMESLEEP 10
    }
    using namespace newcppPrivate;
    
    namespace COLORS {
        #define CLASSIC (-1) //WORD

        #define INVISIBLEBLACK 0

        #define BLUE 1
        #define GREEN 2
        #define LIGHTBLUE 3
        #define RED 4
        #define PURPLE 5
        #define YELLOW 6
        #define WHITE 7
        #define GREY 8
        #define LIGHTBLUE 9
        #define LIGHTGREEN 10
        #define CYAN 11
        #define LIGHTRED 12
        #define PINK 13
        #define LIGHTYELLOW 14
        #define WHITE2 15
    }
    using namespace COLORS;

    #ifdef _WIN32
        #include <io.h>
        #define access _access_s
    #else
        #include <unistd.h>
    #endif

    //Gives true if folder exist.
    bool DirExist(const string& path) {
        struct stat buffer;
        return (stat(path.c_str(), &buffer) == 0); //return PathFileExists(path.c_str());
    }

    //Returns string converted to wstring.
    wstring StringToWString(const string s) {
        std::wstring wsTmp(s.begin(), s.end());
        return wsTmp;
    }

    LPCWSTR s2ws(const std::string& s) {
        int len;
        int slength = (int)s.length() + 1;
        len = MultiByteToWideChar(CP_ACP, 0, s.c_str(), slength, 0, 0);
        wchar_t* buf = new wchar_t[len];
        MultiByteToWideChar(CP_ACP, 0, s.c_str(), slength, buf, len);
        std::wstring r(buf);
        delete[] buf;
        //return r;

        std::wstring stemp = s2ws(s);
        return stemp.c_str();
    }

    //Returns string converted to LPSTR
    LPSTR StrToLPSTR(string s) {
        return const_cast <char*> (s.c_str());
    }

    //Returns true if given char is number
    bool isCharNumber(char c) {
        return isdigit(c);
    }

    //Deleting all given chars from string and returns it.
    string DeleteAllChars(string s, char c) {
        s.erase(std::remove(s.begin(), s.end(), c), s.end());
        return s;
    }

    //Deleting chars from first parameter that are in second.
    string DeleteAllChars(string s, string chars) {
        for (int i = 0; i < chars.length(); i++)
            s = DeleteAllChars(s, chars[i]);
        return s;
    }

    //Replacing string in text to another string.
    //Usage:
    //ReplaceAll(string("The main string"), string(" "), string("-"))
    //Returns:
    //"The-main-string"
    string ReplaceAll(std::string str, const std::string& from, const std::string& to) {
        size_t start_pos = 0;
        while ((start_pos = str.find(from, start_pos)) != std::string::npos) {
            str.replace(start_pos, from.length(), to);
            start_pos += to.length(); // Handles case where 'to' is a substring of 'from'
        }
        return str;
    }

    //Returns a chracter table length.
    int CharacterTableLength() {
        //return int(char(0) - 1);
        char a = 'a' + 1;
        int counter = 1;
        while (a != 'a') {
            a++;
            counter++;
        }

        return counter;
    }

    //Returns string to uppercase
    string UpperString(string s) {
        transform(s.begin(), s.end(), s.begin(), ::toupper);
        return s;
    }

    template <typename T>
    string NumberToString(T pNumber) {
        ostringstream oOStrStream;
        oOStrStream << pNumber;
        return oOStrStream.str();
    }

    //Returns true if element is part of array.
    template <class element, class container>
    bool PartOfArray(element elem, container array, int arraySize=0) {
        if (!arraySize) arraySize = sizeof(array) / sizeof(array[0]);
    
        for (int i = 0; i < arraySize; i++)
            if (elem == array[i])
                return true;

        return false;
    }

    //Returns true if __access_s function returns 0 (~file exists)
    bool FileExists(const std::string& Filename) {
        return _access(Filename.c_str(), 0) == 0;
    }

    //Changing console text color. For changing it back, ChangeColor(COLORS::CLASSIC) or SetDefaultColor
    void ChangeColor(int color = CLASSIC) {
        SetConsoleTextAttribute(hConsole, color);
    }

    //Setting standart color
    void SetDefaultColor() {
        SetConsoleTextAttribute(hConsole, CLASSIC);
    }

    //Writes each symbol in string with function Sleep
    void Write(string s, int maxTime=100) {
        if (maxTime <= 0) maxTime = 1; //Previous version was without it
        //srand(time(0));
        for (int i = 0; i < s.length(); i++) {
            cout << s[i];
            Sleep(rand() % maxTime); //WARNING: Slowing down.
        }
    }

    //Writes string in setted color with Sleep.
    void Print(string s, int color=CLASSIC, int maxT=1, int colorAfter=CLASSIC) {
        ChangeColor(color); //WARNING: Calling SetConsoleTextAttribute insead should speed up.
        Write(s, maxT); //Slows down output.
        ChangeColor(colorAfter);
    }

    //Simulating user press
    void Press(BYTE key, int sleepTime=PRESSTIMESLEEP) {
        keybd_event(key, 0, 0, 0);
        Sleep(sleepTime);
        keybd_event(key, 0, KEYEVENTF_KEYUP, 0);
    }

    //Simulating 2 presses
    void DoublePress(BYTE key, BYTE key2, int sleepTime=PRESSTIMESLEEP) {
        keybd_event(key, 0, 0, 0);
        keybd_event(key2, 0, 0, 0);

        Sleep(sleepTime);

        keybd_event(key, 0, KEYEVENTF_KEYUP, 0);
        keybd_event(key2, 0, KEYEVENTF_KEYUP, 0);
    }

    template<class T>
    T Random(T min = 0, T max = 10000) {
        max -= min;
        T res = rand() % (max+1);
        return res + min;
    }

    //Class for bruteforcing.
    class BruteForce {
    private:
        const char pzc = 1;
        const char pnc = 0;
    public:
        string res = ""; //Bruteforce guess

        //Give next string guess
        string GiveNextGuess() {
            PlusPlus();
            return res;
        }

        string CurrentGuess() {
            return res;
        }

        //Setting start length of brute-force string. Can be given as normal(parameter 2 but s is maybe ['', ''])
        void SetStartLength(int l) {
            if (l <= 0) throw std::invalid_argument("Received incorrect value.");
            res.clear();
            for (int i = 0; i < l; i++)
                res += pzc;
        }

        void PlusPlus() {
            int a = 0;
            while (true) {
                if (a > res.length()) {
                    res += pzc;
                    break;
                }
                else if (res[a] == pnc) {
                    res[a] = pzc;
                    a++;
                }
                else {
                    res[a]++;
                    break;
                }
            }
        }
    };
    namespace sortfunctions {
        template<class T>
        vector<T> GnomeSort(vector<T> arr, bool (*compare)(T, T)) {
            const int s = arr.size();
            for (int i = 1; i < s; i++) {
                if (compare(arr[i], arr[i - 1])) {
                    T helper = arr[i];
                    arr[i] = arr[i - 1];
                    arr[i - 1] = helper;
                    if (i != 1) i -= 2;
                }
            }
            return arr;
        }

        template<class T>
        vector<T> BubbleSort(vector<T> arr, bool (*compare)(T, T)) {
            const int s = arr.size();
            for (int i = 0; i < s; i++) {
                for (int k = i + 1; k < s; k++) {
                    if (compare(arr[i], arr[k])) {
                        T helper = arr[i];
                        arr[i] = arr[k];
                        arr[k] = helper;
                    }
                }
            }
            return arr;
        }
    }

    //Returns junk. Should return last value somewhere in program used. Needs to give any string.
    template<class T>
    const int GetJunk(T r = 10) { if (r != r) return 0; }
#endif

Questions:

  1. Can it be done in other way? For example, I can write code in multiple files, but then compile into one .lib file? If so, how to install it then?
  2. Any code review suggestions?

Some notes

CharacterTableLength - is function that should return number of all symbols installed on the computer. For example, if on computer installed 3 languages, this function should return number of special and 3 languages symbols.

I specially wrote variable names without '_'. I really dont like this style, I use Camel case instead.

\$\endgroup\$
1
  • 4
    \$\begingroup\$ GnomeSort and BubbleSort are useful and necessary on Windows? Glad I never had to write anything for that platform! \$\endgroup\$ Jun 24, 2021 at 13:43

3 Answers 3

7
\$\begingroup\$

General Observations

Can it be done in other way? For example, I can write code in multiple files, but then compile into one .lib file? If so, how to install it then?

Generally it is best to put functions into C++ source files and have header files that provide the interfaces (function prototypes and constants). A library can contain multiple source files, how to do this depends on whether it is a static library or a dynamic library. Putting functions into a header file means that if a function is modified everything needs to be recompiled. If only the function prototype is in the header file then generally to fix a bug means recompiling one file and re-linking the program or library.

Please compile using all the warning messages you can. It helps find possible bugs. I compile with more warning messages than you apparently do and this is what I see when I compile:

Build started...
1>------ Build started: Project: myLittleLib, Configuration: Debug Win32 ------
1>myLittleLib.cpp
1>C:\Users\PaulC\Documents\ProjectsNfwsi\CodeReview\myLittleLib\myLittleLib\myCppLib.h(28,19): warning C4244: 'argument': conversion     from 'time_t' to 'unsigned int', possible loss of data
1>C:\Users\PaulC\Documents\ProjectsNfwsi\CodeReview\myLittleLib\myLittleLib\myCppLib.h(49,1): warning C4005: 'LIGHTBLUE': macro redefinition
1>C:\Users\PaulC\Documents\ProjectsNfwsi\CodeReview\myLittleLib\myLittleLib\myCppLib.h(43): message : see previous definition of 'LIGHTBLUE'
1>C:\Users\PaulC\Documents\ProjectsNfwsi\CodeReview\myLittleLib\myLittleLib\myCppLib.h(110,23): warning C4018: '<': signed/unsigned mismatch
1>C:\Users\PaulC\Documents\ProjectsNfwsi\CodeReview\myLittleLib\myLittleLib\myCppLib.h(186,23): warning C4018: '<': signed/unsigned mismatch
1>C:\Users\PaulC\Documents\ProjectsNfwsi\CodeReview\myLittleLib\myLittleLib\myCppLib.h(253,19): warning C4018: '>': signed/unsigned mismatch
1>C:\Users\PaulC\Documents\ProjectsNfwsi\CodeReview\myLittleLib\myLittleLib\myCppLib.h(90): warning C4717: 's2ws': recursive on all control paths, function will cause runtime stack overflow
1>myLittleLib.vcxproj -> C:\Users\PaulC\Documents\ProjectsNfwsi\CodeReview\myLittleLib\Debug\myLittleLib.exe
1>Done building project "myLittleLib.vcxproj".
========== Build: 1 succeeded, 0 failed, 0 up-to-date, 0 skipped ==========

I'm using Visual Studio 2019 Professional

Include Guards

I'm assuming this library is a header file because of the include guards. In C++ use either #pragma once or #ifndef but you don't need both. I prefer #ifndef because it may be more portable, not all C++ compilers support #pragma once but it is widely supported.

Avoid using namespace std;

If you are coding professionally you probably should get out of the habit of using the using namespace std; statement. The code will more clearly define where cout and other identifiers are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The identifiercout you may override within your own classes, and you may override the operator << in your own classes as well. This stack overflow question discusses this in more detail.

This code is abusing using namespace ANYNAMESPACE;

Never put using namespace std; in a header file.

Use Enum Types

Rather than using a lot of #defines which are frowned on in C++, there are 2 ways to create constants. One is to use constexpr, the other is using an enum type. When more than 2 or 3 constants are being defined an enum is a better way to do this. The link I provided shows an example of a color enum, however, this is what the current code could look like:

    enum MyCOLORS
    {
        BLUE,
        GREEN,
        RED,
        PURPLE,
        YELLOW,
        WHITE,
        GREY,
        LIGHTBLUE,
        LIGHTGREEN,
        CYAN,
        LIGHTRED,
        PINK,
        LIGHTYELLOW,
        WHITE2
    };

Please note that in the code under review LIGHTBLUE is defined twice which should be reported by the compiler.

Type Mismatch in for Loops

Container classes such as std::string and wstring return the type size_t from the length or size member. The type size_t varies from compiler to compiler but it is always unsigned, in many cases it is either unsigned int or unsigned long. One of the possible reasons for this is to prevent negative indexes when using the variable as an index into the container.

string DeleteAllChars(string s, string chars) {
    for (size_t i = 0; i < chars.length(); i++)
        s = DeleteAllChars(s, chars[i]);
    return s;
}

Negative indexes can have bad side affects and cause exceptions.

Reinventing the Wheel

There is no good reason for this function

bool isCharNumber(char c) {
    return isdigit(c);
}

The function isdigit() is well known and well documented, why replace it with an unknown, undocumented function?

\$\endgroup\$
1
  • \$\begingroup\$ Also, raw char to a character-function? The UB. \$\endgroup\$ Jun 25, 2021 at 19:15
4
\$\begingroup\$

I'm not sure how your file is meant to be used. It appears to be set up like a header file, but nothing in it is inline so it can only appear in one translation unit. From your comments, I suspect you have only, thus far, written programs that consist of a single CPP file.

If you want a header-only library, global variables and functions must be marked inline. But you need to learn how to make a project that consists of multiple CPP files linked together, and how to use libraries. Libraries can include third-party code, not just reusable components of your own, so it's something you really need to learn!

As for the specific code...

namespace COLORS {
        #define CLASSIC (-1) //WORD

        #define INVISIBLEBLACK 0

        #define BLUE 1
        #define GREEN 2
        #define LIGHTBLUE 3
        #define RED 4
        #define PURPLE 5
        #define YELLOW 6
        #define WHITE 7
        #define GREY 8
        #define LIGHTBLUE 9
        #define LIGHTGREEN 10
        #define CYAN 11
        #define LIGHTRED 12
        #define PINK 13
        #define LIGHTYELLOW 14
        #define WHITE2 15
    }
    using namespace COLORS;

The namespace doesn't do anything since the #define does not respect namespaces at all. It is one of the top issues that you should not use #define anyway, so change this to an enumerated type — a classic style enum will still let you use the enumeration names where an int is expected, if that is your intended use.

Also, defining a namespace and then immediately using it is a feature that's automated by using inline. (This is a different use of the same keyword mentioned above).

#ifdef _WIN32
Well, you are including "Windows.h" at the top, so you are already committed to Windows for this file.

//Returns string converted to LPSTR
    LPSTR StrToLPSTR(string s) {
        return const_cast <char*> (s.c_str());
    }

Major bug! You are passing s by value, so s.c_str() will return a pointer to the buffer (or internal SSO area) that is destroyed when s goes out of scope, which is when the function returns.

You also should not be casting away the const. Don't do this. If you need a non-const nul-terminated C-style string, make sure you produce one that can be written to.

//Returns true if given char is number
    bool isCharNumber(char c) {
        return isdigit(c);
    }

Why??? This adds nothing over the standard function. If you are intentionally taking care of the legacy int parameter issue, overload it using the same name.

//Deleting chars from first parameter that are in second.
    string DeleteAllChars(string s, string chars) {
        for (int i = 0; i < chars.length(); i++)
            s = DeleteAllChars(s, chars[i]);
        return s;
    }

You are passing the second parameter, chars, by value which is not appropriate. Do you understand that this deep-copies the entire string? If it will not be modified, what is the point of that?
It's very inefficient, deleting all occurrences of one character at a time, recopying the whole string object each time.
Use std::remove_if followed by erase (look up "delete/erase idiom"). In general, become familiar with the standard library and other libraries in your project.

//Returns a chracter table length.
    int CharacterTableLength() {
        //return int(char(0) - 1);
        char a = 'a' + 1;
        int counter = 1;
        while (a != 'a') {
            a++;
            counter++;
        }

        return counter;
    }

I don't even understand what this is supposed to do. It doesn't take any parameters or refer to any global names, so what "chracter[sic] table" are you talking about? It looks like it increments a char value until it wraps around, thus (if that were legal) telling you the number of possible values in a char. If you don't want to just assume it's 256, use numeric_limits<unsigned char>::max()+1.
But this code is actually exhibiting Undefined Behavior as you are not allowed to wrap-around a signed number. The optimizer will look at the condition in your while loop and say "Oh, that's always false" and replace your code with an infinite loop, or delete it altogether.

function that should return number of all symbols installed on the computer. For example, if on computer installed 3 languages, this function should return number of special and 3 languages symbols.

No, it increments a char which has nothing to do with what languages are installed, or which of those char values has a legal code point assigned to it. It just works as a small (8 bit) integer, having nothing to do with the Locale or selected font or anything.

template <typename T>
    string NumberToString(T pNumber) {
        ostringstream oOStrStream;
        oOStrStream << pNumber;
        return oOStrStream.str();
    }

That is just a slow re-implementation of std::to_string. What's wrong with to_string?

//Returns true if element is part of array.
    template <class element, class container>
    bool PartOfArray(element elem, container array, int arraySize=0) {
        if (!arraySize) arraySize = sizeof(array) / sizeof(array[0]);
    
        for (int i = 0; i < arraySize; i++)
            if (elem == array[i])
                return true;

        return false;
    }

Ugh, don't use the old C sizeof trick to determine array sizes. It's brittle and will give the wrong answer if you end up giving it a parameter that's already decayed into a pointer.

Don't use the legacy for loop with index numbers when the range-based for will do.

But, your code, even though container is templated, only works with bare C style arrays, not with containers in general. And you are just reproducing existing standard functions anyway!

Just see if std::count is non-zero, or (more efficient since it can quit when it finds the first match) if std::find returns something other than the end.

Even writing out the logic (not using library function) with the previous tips would make it work with any container, such as vector, not just plain C arrays:

    template <typename element, typename container>
    bool PartOfArray(element elem, container array) {
        for (const auto& val : array)
            if (elem == elem) return true;
        return false;
    }

Oh, I noticed you are passing container array by value. If you actually called this with an array, it will have decayed to a pointer in the parameter! So remember what I said about the sizeof trick being easy to silently get wrong? sizeof (array) actually returns the size of a single pointer, nothing to do with the original array.

Look at Compiler Explorer: If the optional 3rd parameter is not given, it is replace with the value 2 even though the array size is 2089. You can see why in the signature reported in the assembly view: int, int*, int. See, the "array" is actually passed as a pointer to the first element.

Had you tested your code, you would have noticed that it did not work at all.

\$\endgroup\$
4
  • \$\begingroup\$ That isn't really a "legacy int parameter issue". Allowed arguments are all values of type unsigned char and EOF. Anything else is UB. \$\endgroup\$ Jun 25, 2021 at 19:18
  • \$\begingroup\$ sizeof container is a compilation-error. sizeof (container) would be the same as sizeof array, and pretty useless if the argument passed is a native array. \$\endgroup\$ Jun 25, 2021 at 19:21
  • \$\begingroup\$ @Deduplicator corrected. Interesting that the template argument deduction gives the decayed type when the parameter is container x (with no &). I'll have to review the rules for that. By "legacy 'int' parameter" I meant that the function, inherited from C (thus "Legacy"), takes an int and doesn't work with normal char which is signed. Using it correctly requires casting, or better yet making your own wrapper that takes char. Using it correctly requires extra work, thus it's an "issue". \$\endgroup\$
    – JDługosz
    Jun 28, 2021 at 13:51
  • \$\begingroup\$ That is not in any way surprising. A function cannot have an argument of type array. If it looks like it does, that really is a pointer. \$\endgroup\$ Jun 28, 2021 at 19:47
3
\$\begingroup\$
  1. Your s2ws() function is a recursive function with no base condition. Presumably, it's from here? Notice where the closing braces are placed. It's also invoking undefined behavior because it's returning a pointer to memory that will be deallocated once the function returns. Same with StrToLPSTR() and NumberToString() functions.

  1. #defines are not bound by namespace. The COLORS namespace is literally empty at the time of compilation. The compiler isn't throwing a compile error because you've used using namespace COLORS;.

  1. As mentioned above, use constexpr to define constants.

  1. If you're using C++17, you used should std::filesystem instead of relying on specific OS calls. It has the benefit of being portable.

  1. If you're using C++17, you should generally use std::string_view instead of const std::string&.

  1. You can use std::to_string instead of using std::stringstream in the NumberToString() function.

Right now, the function can accept any type. The constrain them to numeric types, you can use static_assert to do a compile time check. Using the type_traits library, you can do

static_assert(is_integral_v<T> && is_floating_point_v<T>, "only numeric types allowed"!);

This will now throw an error if you pass in any other type other than numeric types.


  1. PartOfArray contains a bug. What if someone passes a C-style array and doesn't pass the size?
int arr[] = {1, 2, 3};
bool inArray = PartOfArray(3, arr);
std::cout << std::boolalpha << inArray; // prints false;

  1. Use <chrono> library instead of <time.h>. Use <random> library instead of rand()

  1. The PlusPlus() method invokes undefined behavior because of an out-of-bounds memory access. Hint: what happens when a=0 and res=""?

  1. Some are the comments are redundant. Code should be as self-documenting as possible. Returns true if function returns 0 doesn't add anything that I can't figure out by looking at the code.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.