Solving a Binary Puzzle

• You said

  The entire grid should be filled up with 1's and 0's.

  That means the only state in BineroTile should be a bool - true for 1 and false for 0. It's not clear why you need two bool members to capture BineroTile's state. What does it mean when one is true and zero is true? What's the state of that tile?

  I am guessing that you want to capture "unspecified" as a state. In that case, an enum would be better.

  class BineroTile {
  public:
      enum State { UNSPECIFIED = -1, ZERO, ONE };
      State state;
  
      // ... 
  };
  

• The operator= function

  Idiomatically speaking, it's perfectly reasonable to use:

  BineroTile tile;
  tile = 1;
  

  However, the signature of the function and the implementation need to be updated to be idiomatic:

  BineroTile& operator=(const int &value) {
      set(value);
      return *this;
  }
  

  Then, you can use:

  BineroTile tile1;
  BineroTile tile2;
  tile1 = tile2 = 1;
  

• Redundant data in BineroPuzzle

  width and height are redundant member variables. Please remove them. You can get them from the grid member variable.

• Better initialization of BineroPuzzle::grid

  You can use

  BineroPuzzle(int width, int height) :
  grid(height, std::vector<BineroPuzzle>(width, BineroTile())) {
  

  to initialize grid. That will remove a block of code from BineroPuzzle's constructor.

I'll start with some of the basic conventions now. There is quite a bit of code here.

• This looks unusual for a constructor:

  BineroTile() {
      clear();
  }
  

  The call to a function called clear() makes this look like a destructor, though that's not even what's being done here.

  If you're just trying to set initial values, use an initializer list:

  BineroTile()
      : zero(true)
      , one(true)
  {}
  

• This doesn't quite look right:

  int getValue() {
      if (!hasValue()) {
          return -1;
      }
      return one ? 1 : 0;
   }
  

  What value is it getting? It looks like it'll return a boolean value depending on the state of one. Perhaps this should've been named something else.

• This operator= overload isn't in the proper form:

  void operator=(const int &value) {
      set(value);
  }
  

  You would typically use the argument as the right-hand operand to set the current object (this) to that operand. You're not doing that at all here.

  Here's what it should look like:

  BineroTile& operator=(const BineroTile &rhs) {
      zero = rhs.zero;
      one = rhs.one;
      return *this;
  }
  

  In case you may need to check for self-assignment as well:

  if (this != &rhs) {
      zero = rhs.zero;
      one = rhs.one;
  }
  
  return *this;
  

  Moreover, you don't really have to overload operator= if you're just doing simple assignments. The compiler automatically generates a default assignment operator for you. One good reason to define your own operator= is if you have pointer members, which will not be handled properly automatically by the compiler. This is because the compiler-generated version performs a shallow copy instead of a deep copy.

• print() should also be const. This applies to all member functions, especially print functions, that don't modify any data members. Any attempts at modifying data members within such functions will result in a compiler error.

• In readFromFile(), I don't think the last close() is needed if file fails to open. This is usually only necessary for error-checking.

• Also in readFromFile(), there doesn't appear to be a need for new. In C++, unlike in Java, manual memory allocation is used very sparingly in favor of the classes in the standard library that handle memory itself. You are properly deallocating afterwards as you should, but again, it doesn't appear to be necessary here.

I see some things that I think could help you improve your code.

Separate interface from implementation

It makes the code somewhat longer for a code review, but it's often very useful to separate the interface from the implementation. In C++, this is usually done by putting the interface into separate .h files and the corresponding implementation into .cpp files. It helps users (or reviewers) of the code see and understand the interface and hides implementation details.

Make data members private

The zero and one members of BineroTile are public but they really ought to be private members. Similarly, data members for BineroPuzzle should be made private. If you need accessors, you could use simple const ones like this:

int Width() const { return width; }
int Height() const { return height; }

Use const where practical

The hasValue and getValue member functions of BineroTile do not alter the underlying object and therefore should be declared const.

Isolate platform-specific code

If you must have stdafx.h, consider wrapping it so that the code is portable:

#ifdef WINDOWS
#include "stdafx.h"
#endif

Avoid reusing the same name for different things

Inside your BinaroPuzzle::solve() function, you use a variable set for each match_set, but there is also a member function set. Although it's unambiguous to the compiler, it could be clearer to a reader of the code if that variable were named something else.

Don't rely on uninitialized variables

In side the BinerorMatch::scan function, we have this peculiar code:

if (tiles.size() - zeros - ones != unknown) {
    unknown = tiles.size() - zeros - ones;
}

The first problem is that unknown is, well, unknown at that point the first time through the code, so your code is making a decision based on an uninitialized value which is an error. It works anyway because unknown is then set to a particular value. Why not just set the value?

unknown = tiles.size() - zeros - ones;

Prefer references to raw pointers

Within BineroMatch::scan(), variables current, next, before and after are all declared and used as pointers to BinaroTile but what you really want is a reference. The difference is that a reference cannot be nullptr and must actually point to an object, which is exactly the guarantee your code relies on anyway. Instead, I would do it like this:

BineroTile& current = *tiles[i];

if (current.getValue() == 0) {
    zeros++;
}
// etc.

Make your functions match typical usage

Every instance of use of hasValue() negates the result to use it. That strongly suggests that the function should probably be this instead:

bool isUnknown() const {
    return zero == one;
}

Similarly, the code includes this:

if (current->getValue() == 0) {
    zeros++;
}
if (current->getValue() == 1) {
    ones++;
}

I would instead be inclined to write that as:

if (current->hasValue()) {
    if (current->isZero()) {
        ++zeroes;
    } else {
        ++ones;
    }
}

Use an idiomatic operator=

Your current BineroTile::operator=() is this:

void operator=(const int &value) {
    set(value);
}

It should actually return BineroTile& and not void, otherwise chained operations such as a = b = c can't work. Since the result of this is just a simple copy, delete the whole function and let the compiler automatically generate it. However, you might want to create another form, like this:

BineroTile &operator=(bool value) {
    one = value;
    zero = !value;
    return *this;
}

Provide for more natural syntax

Rather than the somewhat strange thing that's being done with value inside BineroMatch::scan(), you might want to use the suggested operator= above and this:

bool operator!() const {
    return zero;
}

That will allow you write much more clearly, as with this example:

if (i > 0) {
    BineroTile& before = *tiles[i - 1];
    simplified |= before.isUnknown();
    before = !current;
}

Eliminate unused structures and classes

The Puzzle struct is unused and should be deleted.

Separate input from output

The BineroPuzzle::readFromInput() both prompts and reads. Better design would be to separate prompts from the actual input. Alternatively, since it's unused in this sample program, you could omit it.

Reconsider your choice of data structures

The code currently uses a number of std::vectors for things that might be better expressed as std::valarray. Using a std::valarray would allow you to use a std::slice which seems like a better fit for this particular problem.

Prefer constructors to special member functions

The readFromFile function has two problems, as I see it. First, it's a free-standing function and not a member function even though it's clearly closely associated with the BineroPuzzle class. Second, it returns a raw pointer rather than an actual object. Finally, the use of new and delete is really not needed here, especially since the value returned is never checked for nullptr. You can solve all of those problems by refactoring this as a constructor taking std::ifstream& as an input.

BineroPuzzle::BineroPuzzle(std::istream &file) {
    std::string line;
    getline(file, line);
    unsigned int size = line.length();
    BineroPuzzle puzzle(size, size);
    readLine(&puzzle, 0, line);
    for (unsigned int y = 1; y < size; y++) {
        getline(file, line);
        readLine(&puzzle, y, line);
    }
    std::swap(*this, puzzle);
}

This could (and should!) be refactored further, including making readLine a private member function but I'll leave that to you.

Prefer a stream inserter to a custom print routine

Your custom BineroPuzzle::print routine could instead be written as a stream inserter:

friend std::ostream& operator<<(std::ostream &out, const BineroPuzzle &bp) {
    for (int y = 0; y < bp.Height(); y++) {
        for (int x = 0; x < bp.Width(); x++) {
            out << bp.grid[y][x];
        }
        out << "\n";
    }
    return out;
}

Prefer for to do ... while

In BinaroPuzzle::solve() the loop is this:

void solve() {
    bool simplified = false;
    do {
        simplified = false;
        for (auto set : match_sets) {
            simplified |= set.scan();
        }
    }
    while (simplified);
}

A clearer alternative would be this:

void solve() {
    for (bool simplified = true; simplified; ) {
        simplified = false;
        for (auto set : match_sets) {
            simplified |= set.scan();
        }
    }
}

Or you could also use std::for_each and a lambda:

void solve() {
    for (bool simplified = true; simplified; ) {
        simplified = false;
        std::for_each(match_sets.begin(), match_sets.end(), 
                [&simplified](BineroMatchSet &bms){ 
                    return simplified |= bms.scan(); 
                }
        );
    }
}

Omit return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.