1
\$\begingroup\$

Can anyone give me recommendations on how to improve my code?

using System;
namespace mybinsearch2
{
    class Program
    {
        static void Main(string[] args)
        {
            int n = int.Parse(Console.ReadLine());
            int key = int.Parse(Console.ReadLine());
            int[] arr = new int[n];  int min = 0;int max = n - 1;
            int mid = min + (max - min) / 2;int temp;
            for (int i = 0; i <= n - 1; i++)
            {
                arr[i] = i *5;
            }
            while (true)
            {
                temp = arr[mid];
                if (arr[mid] == key)
                {
                    Console.WriteLine("Position is: " + mid); break;
                }
                else if (arr[mid] < key) min = mid;
                else max = mid;
                mid = min + (max - min) / 2;
                if(arr[mid]==temp)
                {
                    Console.WriteLine("Not Found"); break;
                }
            }

       }
    }
}
\$\endgroup\$
7
  • 2
    \$\begingroup\$ Step 1: use much better (longer) names. \$\endgroup\$ Commented Jan 30, 2017 at 15:41
  • 1
    \$\begingroup\$ Step 2: group lines together more logically. The first mid = ... statement isn't used in the fill loop, so it should come after it. \$\endgroup\$ Commented Jan 30, 2017 at 15:44
  • 2
    \$\begingroup\$ If you have repeating values I think the second if(arr[mid]==temp) can fail. \$\endgroup\$ Commented Jan 30, 2017 at 15:53
  • \$\begingroup\$ I would recommend putting each statement on a new line, e.g. Console.WriteLine("Not Found"); break; should be two lines. It made the break statements a lot easier to miss. \$\endgroup\$ Commented Jan 30, 2017 at 16:07
  • \$\begingroup\$ What problem does your code solve? We're missing context here. \$\endgroup\$ Commented Jan 30, 2017 at 16:11

1 Answer 1

Reset to default
0
\$\begingroup\$

If you have repeating values I think the second if(arr[mid]==temp) can fail.

Separate the create the test from the program (method). Return a value from method and print it in the test.

The first if (arr[mid] == key) that is the least likely condition so put it last and it can just be an else as you should test for < and > so = is all that is left.

You repeatedly reference arr[mid] when you already have it temp = arr[mid];

You have max = n - 1; but you don't use it in the create test loop

Put assignments on separate lines
This still has problems - this is just cleaning up existing code 

static void Main(string[] args)
{
    int n = int.Parse(Console.ReadLine());
    int key = int.Parse(Console.ReadLine());
    int[] arr = new int[n];
    int min = 0;
    int max = n - 1;
    int mid = min + (max - min) / 2;
    int temp;
    for (int i = 0; i <= max; i++)
    {
        arr[i] = i * 5;  
    }
    while (true)
    {
        temp = arr[mid];
        if (temp < key)
        {
            min = mid;  // this seems backwards but that is what you have
        }
        else if (temp > key)
        {
            max = mid;
        }
        else
        {
            Console.WriteLine("Position is: " + mid);
            break;
        }
        mid = min + (max - min) / 2;
        if (arr[mid] == temp)  // this still has problems
        {
            Console.WriteLine("Not Found");
            break;
        }
    }
}
\$\endgroup\$
7
  • \$\begingroup\$ How could I stop the loop if we have repeating values? Also, why do you say that I don't use "max = n - 1;" ? I use it when defining mid in the loop. \$\endgroup\$ Commented Jan 30, 2017 at 16:07
  • 1
    \$\begingroup\$ @nikolay Fundamentally change the algorithm. Wiki has a good implementation en.wikipedia.org/wiki/Binary_search_algorithm \$\endgroup\$ Commented Jan 30, 2017 at 16:18
  • \$\begingroup\$ Downvote may I ask what the problem is? \$\endgroup\$ Commented Jan 30, 2017 at 16:24
  • \$\begingroup\$ There is no problem, I just wanted to see how I can optimize the code. \$\endgroup\$ Commented Jan 30, 2017 at 16:30
  • \$\begingroup\$ @nikolay But there are problems \$\endgroup\$ Commented Jan 30, 2017 at 16:31

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.