patterncsharpMinor
Finding binary combinations based on substitute wildcards
Viewed 0 times
combinationswildcardssubstitutebinarybasedfinding
Problem
I am given a string containing the possible characters of :
For example; given
My code seems to work, I tested with two wildcards and three wildcards, but is there a better way to solve this.
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:
Should be:
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:
Also, it seems like you're fighting your IDE here:
And here:
These spaces feel very awkward. C# reads better like this:
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
Abstractions & Responsibilities.
Programming is about making abstractions - the abstraction level of
You have a loop in
Notice how much easier it is to figure out exactly what's going on - and why.
Now, looking at the
Ah.. and now I see why
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.
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.