0
\$\begingroup\$

This is a follow-up question based on the original one. Could you please see the updated and refactored version and point at things to be improved in term of logic or efficiency?

I have now know as I have been made aware about the Greedy Algorithm; Math.Max and binary shifting which deal more gracefully with the task, however, if I were in a position to be unaware of them and this is the code I managed to write, what's your thoughts on it?

    public static int FindGap(int number)
    {
        string toBinary = Convert.ToString(number, 2);
        char[] toChar = toBinary.ToCharArray();

        char previous = '0';
        char current = '0';
        int maxGap = 0;
        int answer = 0;

        for (int i = 0; i < toChar.Length; i++)
        {
            if (toChar[i] == '1')
            {
                current = toChar[i];

                // keep last/latest largest gap as an answer
                if (answer < maxGap)
                {
                    answer = maxGap;
                }

                // reset if 1 appeared and start counting next gap
                maxGap = 0;
            }
            if ((toChar[i] == '0' && current == '1') || (toChar[i] == '0' && previous == '0'))
            {
                current = '0';
                maxGap++;
                previous = current;
            }
        }

        if (answer != 0)
        {
            return answer;
        }
        else return 0;
    }
\$\endgroup\$
3
  • 1
    \$\begingroup\$ The code still creates a string although this is not needed. You should try the code from my answer on the previous question. It is much more efficient. \$\endgroup\$ May 24, 2019 at 13:52
  • 1
    \$\begingroup\$ @RolandIllig: The OP states that yours and other suggestions are ignored on purpose. I'm not claiming that the justification is particularly reasonable. \$\endgroup\$
    – AlexV
    May 24, 2019 at 14:14
  • \$\begingroup\$ Long story short, I wanted to see my solution improved with the approach I have undertaken initially, without knowing what was already invented to support me in resolving such problems. It was important for me to see that I can do the Convert.ToString() inside the foreach loop and also that I have failed the obvious binary logic. To me that helps a lot. \$\endgroup\$
    – Kris
    May 24, 2019 at 14:50

1 Answer 1

4
\$\begingroup\$

If I were in a position to be unaware of them and this is the code I managed to write, what's your thoughts on it?

  • No need to call ToCharArray() because you can iterate over the chars of a string as well
  • By using an else if instead of the second if you won't need to check for toChar[i] == '0' but basically you only need a else.
  • Using a foreach loop instead of a for loop seems more natural for this task.
  • The returning of the answer can be simply return answer;
  • Because ToString(int, 2) strips leading '0' you can remove both current and previous.

Summing up it could look like so

public static int FindGap(int number)
{
    int maxGap = 0;
    int answer = 0;

    foreach(var c in Convert.ToString(number, 2))
    {
        if (c == '1')
        {
            if (answer < maxGap)
            {
                answer = maxGap;
            }
            maxGap = 0;
        }
        else
        {
            maxGap++;
        }
    }
    return answer;
}
\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot, I knew I was missing obvious things such as.. binary work, which translates to if and else.. This is exactly what I wanted, thanks a lot. I will work out all suggested ways of going about it, with Math.Max, binary shift and what @Roland Illig suggested. This is a great way for learning. \$\endgroup\$
    – Kris
    May 24, 2019 at 14:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.