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

Library for C++ programs

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

Problem

Here is some of the code in my library. If there any inefficient things in my code, let me know.
UPDATE: The following code is deprecated. All of your fixes have been incorporated. The project now lives at InitializeSahib/Inverse
InverseAPI.cpp:

```
// InverseAPI (Cobalt)
// Developed by Sahibdeep Nann (@SahibdeepNann)
// https://github.com/InitializeSahib/InverseAPI
// Licensed under the MIT License
#include "stdafx.h"
#include "InverseCobalt.h"
namespace InverseCobalt {
int InverseRandom::randomInteger(int minimumInteger, int maximumInteger) {
std::mt19937 generateInteger(time(0));
std::uniform_int_distribution integerDistribution(minimumInteger, maximumInteger);
return integerDistribution(generateInteger);
}
double InverseMath::inverseExponents(double baseNumber, double exponentNumber) {
return pow(baseNumber, exponentNumber);
}
double InverseMath::inverseSquare(double numberToSquare) {
return InverseMath::inverseExponents(numberToSquare, 2);
}
void InverseSystem::runSystemCommand(const char *commandToRun) {
system(commandToRun);
}
time_t InverseSystem::getSystemTime() {
return time(0);
}
int InverseRandom::randomDigit() {
std::mt19937 generateDigit(time(0));
std::uniform_int_distribution integerDistribution(0, 9);
return integerDistribution(generateDigit);
}
char InverseRandom::randomLetterUppercase() {
std::mt19937 generateLetterInt(time(0));
std::uniform_int_distribution integerDistribution(1, 26);
int randomLetterInt = integerDistribution(generateLetterInt);
return (char)'A' - 1 + randomLetterInt;
}
char InverseRandom::randomLetterLowercase() {
std::mt19937 generateLetterInt(time(0));
std::uniform_int_distribution integerDistribution(1, 26);
int randomLetterInt = integerDistribution(generateLetterInt);
return (char)'a' - 1 + randomLetterInt;
}
int In

Solution

This seems redundant:

if (randomLetterInt == 1) {
        return 'a';
    }
    if (randomLetterInt == 2) {
        return 'b';
    }
    if (randomLetterInt == 3) {
        return 'c';
    }


.... etc.

Replace with an array lookup.

static char randomLetter[] = "abcdefghtijklmnopqrstuvwxyz";
  return randomLetter[randomLetterInt-1];


You dont want to build a random nuber generator every time you want a random number. You byuild the generator once then ask for numbers,

int InverseRandom::randomDigit() {

    // build the generator once
    static   std::mt19937 generateDigit(time(0));
    ^^^^^^

    // It would be better to create one generator 
    // that is used for all calls

    std::uniform_int_distribution integerDistribution(0, 9);
    return integerDistribution(generateDigit);
}


Lots of static methods is very OO. But C++ is not just OO.

class InverseMath {
public:
    static INVERSECOBALT_API double inverseExponents(double baseNumber, double exponentNumber);
    static INVERSECOBALT_API double inverseSquare(double numberToSquare);
    static INVERSECOBALT_API double inverseCube(double numberToCube);
    static INVERSECOBALT_API bool isEven(int numberToCheck);
    static INVERSECOBALT_API bool isOdd(int numberToCheck);
};


A better technique for C++ is to use namespace.

namespace InverseMath {
    INVERSECOBALT_API double inverseExponents(double baseNumber, double exponentNumber);
    INVERSECOBALT_API double inverseSquare(double numberToSquare);
    INVERSECOBALT_API double inverseCube(double numberToCube);
    INVERSECOBALT_API bool isEven(int numberToCheck);
    INVERSECOBALT_API bool isOdd(int numberToCheck);
};


These functions do nothing:

void InverseData::tempInt(int valueOfTempInt) {
    int tempInt = valueOfTempInt;
//  ^^^^ this is a local variable
//       Once scope is left the vale disappears.   
}
void InverseData::tempChar(char valueOfTempChar) {
    char tempChar = valueOfTempChar;
//  ^^^^
//  klop
}


Using an if condition to return true/false. You already have a boolean in the test.

if (numberToCheck % 2 == 0) {
        return true;
    }
    else {
        return false;
    }


This is easier to write as:

return (numberToCheck % 2 == 0);


Reading a file:

There is no point in using two lines. When the constructor will do it in one line.

std::ifstream readStream;
    readStream.open(fileToRead);


Why not just

std::ifstream readStream(fileToRead);


There is no reason to explicitly close a file.

readStream.close();


In fact there are situations where this is bad. But in general you should let the destructor close the file. The destructor will close the file (and importantly catch and discard exceptions). You should only call close() explicitly if you want to explicitly handle error conditions (there are cases where this is important, but usually not).

Code Snippets

if (randomLetterInt == 1) {
        return 'a';
    }
    if (randomLetterInt == 2) {
        return 'b';
    }
    if (randomLetterInt == 3) {
        return 'c';
    }
static char randomLetter[] = "abcdefghtijklmnopqrstuvwxyz";
  return randomLetter[randomLetterInt-1];
int InverseRandom::randomDigit() {

    // build the generator once
    static   std::mt19937 generateDigit(time(0));
    ^^^^^^


    // It would be better to create one generator 
    // that is used for all calls


    std::uniform_int_distribution<int> integerDistribution(0, 9);
    return integerDistribution(generateDigit);
}
class InverseMath {
public:
    static INVERSECOBALT_API double inverseExponents(double baseNumber, double exponentNumber);
    static INVERSECOBALT_API double inverseSquare(double numberToSquare);
    static INVERSECOBALT_API double inverseCube(double numberToCube);
    static INVERSECOBALT_API bool isEven(int numberToCheck);
    static INVERSECOBALT_API bool isOdd(int numberToCheck);
};
namespace InverseMath {
    INVERSECOBALT_API double inverseExponents(double baseNumber, double exponentNumber);
    INVERSECOBALT_API double inverseSquare(double numberToSquare);
    INVERSECOBALT_API double inverseCube(double numberToCube);
    INVERSECOBALT_API bool isEven(int numberToCheck);
    INVERSECOBALT_API bool isOdd(int numberToCheck);
};

Context

StackExchange Code Review Q#103839, answer score: 4

Revisions (0)

No revisions yet.