4
\$\begingroup\$

This is by far the biggest project i've ever made. The main goal was just to make a bitset class but it expanded into making several classes based off of that main bitset class. Some things I would like suggestions on is how to optimize it better, and if I'm using any bad programming habits or there is a better way of doing something. Thanks.

Classes:

bitset - This is the base program which has no dependencies and is a building block for the other classes

i_bitset - This is used to store integers. It is dependant on the bitset class. It has 5 operator overloads and can convert a bitset to an integer.

c_bitset - This is used to store chars. It is dependant on the i_bitset class. It has 4 operator overloads and can convert a bitset to a char and is used as a building block for s_bitset.

s_bitset - This is used to store strings. It is dependant on the c_bitset class. It has 5 operator overloads and can convert a bitset to a string. This class also contains several standard functions for a string such as append, length, erase and clear.

Here is the github link to the full project: https://github.com/CowNation/cpp-Bitset/

There is too much code to paste the entire project all in this thread, but here is a demo of cpp-Bitset in action, the base bitset class and the i_bitset class:

main.cpp:

#include "s_bitset.h"

int main() {
  i_bitset set;
  set = 176; // = operator is overloaded
  std::cout << set << std::endl; // << operator is overloaded
    i_bitset k;
    k = 176;
    std::cout << (set == 420) << std::endl;
    std::cout << (set == k) << std::endl;

  c_bitset character;
  character = 'k';
  std::cout << character << std::endl;

  s_bitset ree;
  ree.Append(character);
  ree.Append("k");

  s_bitset xd;
  xd = ree + " xd";

  std::cout << xd << std::endl;

  s_bitset jeff;
  jeff = "Jeff"; // = operator is overloaded

  std::cout << jeff + " _--" << std::endl; // << and + operator are overloaded
  for (int i = 0; i < jeff.length(); i++){
    jeff.at(i).print();
    std::cout << std::endl;
  }

  s_bitset st;
    std::cout << "Enter string: ";
  std::cin >> st;
  std::cout << st << std::endl;
}

bitset.h:

#include <iostream>
#include <vector>
#include <string>
#include <math.h>

class bitset {
  protected:
  std::vector< bool >BitSet;
  int GetValue(int Index) const;
  public:
  bitset(int bits);
  void setbit(int Index, bool Value);
  bool getbit(int Index);
  void print();
};

bitset.cpp:

#include "bitset.h"

int bitset::GetValue(int Index) const {
  return pow(2, Index);
}
bitset::bitset(int bits){
  BitSet.clear();
  for(int i = 0; i < bits; i++){
    BitSet.push_back(false);
  }
}
void bitset::setbit(int Index, bool Value){
  BitSet[Index] = Value;
}
bool bitset::getbit(int Index){
  return BitSet[Index];
}
void bitset::print(){
  for (int i = 0; i < BitSet.size(); i++)
    std::cout << BitSet[i] << "/" << GetValue(i) << " ";
}

i_bitset.h:

#include "bitset.h"

class i_bitset : public bitset {
  private:
  int pGet() const;
  public:
  i_bitset() : bitset(8){}
  friend std::ostream & operator<<(std::ostream & _stream, i_bitset const & mc);
  operator int() const;
  int operator=(const int& b);
    bool operator==(const i_bitset& b);
    bool operator==(const int& b);
};

i_bitset Integer(int i);

i_bitset.cpp:

#include "i_bitset.h"

int i_bitset::pGet() const {
  int ret = 0;
  for (int i = 0; i < BitSet.size(); i++)
    ret += BitSet[i] * GetValue(i);
  return ret;
}
std::ostream & operator<<(std::ostream & _stream, i_bitset const & mc) {
  _stream << mc.pGet();
  return _stream;
}
i_bitset::operator int() const {
  return pGet();
}

void checkValue(int& iLetter, i_bitset& integer, int Num, int Index){
  if (iLetter >= Num && integer.getbit(Index) == false){
    integer.setbit(Index, true);
    iLetter -= Num;
  }
}

i_bitset Integer(int i){
  i_bitset integer;
  while (i > 0){
    checkValue(i, integer, 128, 7);
    checkValue(i, integer, 64, 6);
    checkValue(i, integer, 32, 5);
    checkValue(i, integer, 16, 4);
    checkValue(i, integer, 8, 3);
    checkValue(i, integer, 4, 2);
    checkValue(i, integer, 2, 1);
    checkValue(i, integer, 1, 0);
  }
  return integer;
}

int i_bitset::operator=(const int& b) {
  i_bitset integer = Integer(b);
  for (int i = 0; i < 8; i++){
    setbit(i, integer.getbit(i));
  }
  return b;
}

bool i_bitset::operator==(const i_bitset& b){
  return (int)b == pGet();
}

bool i_bitset::operator==(const int& b){
  return b == pGet();
}
\$\endgroup\$
3
\$\begingroup\$

I'm not sure how to review your code, because I don't understand what you're aiming for. There is a std::bitset class but it's different from yours: its size is known at compile-time and it's chiefly designed to function as a group of flags you would have coded into an integer of some kind in a more basic language.

But you use std::vector<bool> as the underlying data-structure of your class, which has a different goal, although it's hard to define precisely: std::vector<bool> is designed to manage collections of bits whose size isn't known beforehand, so I would say it's best used to represent a collection of something else very densely, or more accurately a collection's projection. An example would be an Eratosthenes's sieve, where each integer (let's say 32 bits) is only considered as whether prime or not (1 bit).

Anyway, you seem to aim for yet something else, something like providing an easier to manipulate binary interface to third-part classes: integers, characters, even strings. I have some reservations about this:

  1. binary manipulation is already handy in C++: you have dedicated operators, they aren't arcane C++, there's no need to build a complex class hierarchy to extract one bit from an object.

  2. binary manipulation is often not what you desire: when you copy a string, which essentially is a pointer, you don't want to copy that pointer but to copy what is pointed to in a different memory location, and keep a pointer to that new location. The semantic of a class is implemented through its constructor and can't be summed up into its binary lay-out. On that topic, your s_bitset class is ill-named, because it offers the binary representation of a char[], not of a string.

  3. From 1 and 2, I conclude that your project is not well conceived (unless of course it only has a learning purpose), because it ties down the object to its binary representation, and doesn't offer many more functionalities than the language itself. It leads to strange things, like the translation of a vector of booleans into a integer (in int i_bitset::pGet()), although the integer already is a sequence of 0's and 1's.

So, my first piece of advice is to think more deeply about what you're trying to achieve, and to look more closely at what's already been done. And then decide: do I want to reinvent the wheel to learn how it rolls? Then re-implement std::bitset, std::vector<bool>, or more ambitiously implement a big_integer class; all three offer different challenges: std::bitset is template-programming oriented, std::vector<bool> needs memory manipulation and proxies, and big_integer is more mathematically / algorithmically intensive.

Now onto the review of some details in your code:

  • use initialization lists in constructors:

For instance, when you write:

bitset::bitset(int bits){
  BitSet.clear();
  for(int i = 0; i < bits; i++){
    BitSet.push_back(false);
  }
}

what is done is: Bitset is created and initialized before entering the constructor's body. It is then cleared, which doesn't really make sense because it's empty. Finally, you fill it one bit at a time through proxy references. All this comes at a cost. You could have written:

bitset::bitset(int bits) : Bitset(bits, false) {}

Bitset is then directly constructed with these two arguments (number of elements, element initial value).

  • do not assume that integers are 8 bits long. It's generally false: 32- or 64-bits sized integers are more frequent.

  • it isn't idiomatic to take a const reference to a built-in type: so rather int operator=(int b); than int operator=(const int& b);

  • it isn't idiomatic, and even surprising, not to return a reference to the bitset in this assignment, so rather bitset& operator=(int b); than int operator=(const int& b);. It allows chained assignment.

  • it is also dangerous to have a constructor and an assignment operator with identical arguments but different semantics: i_bitset bs(8) is a bitset of eight bits, but bs = 8; means it contains the binary representation of eight.

  • try to insulate the basic operations from the more complex ones. For instance, you define a get_bit function, but don't use it in your print function. It's a pity, because you could very well forget to actualize it if you change the underlying data structure (for instance to a std::vector<char> instead).

  • try to offer an easier to use interface: it seems you can't initialize a bitset from another one (i_bitset& operator=(const i_bitset& b); for instance.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the in-depth review, the project was initially just for learning. What do you mean by 'std::vector<bool> needs memory manipulation and proxies'? Also, what do you mean by 'try to offer an easier to use interface: it seems you can't initialize a bitset from another one (i_bitset& operator=(const i_bitset& b); for instance.' when you just got done suggesting me to 'it isn't idiomatic, and even surprising, not to return a reference to the bitset in this assignment, so rather bitset& operator=(int b); than int operator=(const int& b);. It allows chained assignment.' \$\endgroup\$ – Cow Nation May 10 at 12:39
  • \$\begingroup\$ @CowNation: about vector<bool>, memory manipulation because it can be grown or shrinked during execution, proxies because the interface forces you to return a reference to a bit, which can only be done through a proxy class, i.e emulating the behavior of an individual byte. / about the interface: references to built-in types (such as an integer) aren't idiomatic, but references to custom classes -such as your bitset are the norm. \$\endgroup\$ – papagaga May 10 at 14:04

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.