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

Finding binary combinations based on substitute wildcards

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

Problem

I am given a string containing the possible characters of : 0, 1, and ?, iterating over possibilities, replacing the wildcard ? with either 0 or 1.

For example; given "1?00?101", the outcome would be: ["10000101", "10001101", "11000101", "11001101"].

My code seems to work, I tested with two wildcards and three wildcards, but is there a better way to solve this.

class MainClass
    {
        static void BinaryString(char [] word, int a, int b, int count){
            if (count == 0) {
                Console.WriteLine (word);
            } else {
                for (int i = a; i < b; i++) {
                    if (word [i].Equals ('?')) {
                        word [i] = '0';
                        count--;
                        BinaryString (word, i, word.Length - 1, count);
                        word [i] = '1';
                        BinaryString (word, i, word.Length - 1, count);
                        word [i] = '?';
                        count++;
                    }
                }
            }
        }
        public static void Main (string[] args)
        {
            string binary = "1?00?1?1";
            char[] bi = binary.ToCharArray ();
            int count = 0;
            for (int i = 0; i < bi.Length; i++) {
                if (bi [i].Equals ('?')) {
                    count++;
                }
            }
            BinaryString (bi, 0, bi.Length - 1, count);
        }
    }

Solution

Let's get the obvious out of the way first.
Style.

The first thing that hits me is the indentation being offset here:

class MainClass
    {


Should be:

class MainClass
{


The next thing that hits me (in the same millisecond, really), is the inconsistent braces. You have them next-line-style at class level, and anything under class scope has them same-line-style. It's.... annoying to read. C# typically uses next-line braces:

if (count == 0) 
{
    Console.WriteLine (word);
} 
else 
{
    ...
}


Also, it seems like you're fighting your IDE here:

char[] bi = binary.ToCharArray ();


And here:

if (bi [i].Equals ('?'))


These spaces feel very awkward. C# reads better like this:

char[] bi = binary.ToCharArray();


if (bi[i].Equals('?'))


The last thing, and perhaps the most important one, is about method ordering: code should read like a book, and unveil like a story. Just like you wouldn't start reading a story at the end, a class' code shouldn't start with its implementation details - I expect void Main to be the first thing: that's where everything starts, after all.

Abstractions & Responsibilities.

Programming is about making abstractions - the abstraction level of Main is too low, it's not clear what that loop is there for and what's being counted: count isn't exactly the most meaningful name to use here. You're counting question marks, and question marks are a thing in the context of this app. Why isn't there a concept of wildcard anywhere?

private static readonly char Wildcard = '?';


You have a loop in Main, that's counting the wildcards. You could extract a method to better express that intent, but it's probably simpler to inline a bit of LINQ there - note that ToCharArray() is completely useless, since a string can already be iterated as an IEnumerable in C#:

var wildcardCount = binary.Count(c => c.Equals(Wildcard));


Notice how much easier it is to figure out exactly what's going on - and why.

Now, looking at the BinaryString method (bad name, methods should start with a verb, they're actions), I'm questioning why you would need to pass in the wildcardCount as a parameter, when all it's used for is really to exit the looping/recursion logic - you already have that information in the character array itself:

if (!word.Any(c => c.Equals(Wildcard)))
{
    Console.WriteLine(word);
}


Ah.. and now I see why ToCharArray was used - the method returning void, your entire logic depends on the ability to mutate array elements! That smells funky, because parameters are passed by value. A much more idiomatic way, in my opinion, would be to take a string input, and return a new string after processing.

That would make it easier to leave the console output out of the function's logic, too - I don't like that the function is responsible for producing/computing the string (/mutating the character array) and producing the output. It just feels wrong, clearly it breaks the Single Responsibility Principle.

Code Snippets

class MainClass
    {
class MainClass
{
if (count == 0) 
{
    Console.WriteLine (word);
} 
else 
{
    ...
}
char[] bi = binary.ToCharArray ();
if (bi [i].Equals ('?'))

Context

StackExchange Code Review Q#98193, answer score: 7

Revisions (0)

No revisions yet.