2

Write a console app in C# to find an index i in an array that is the maximum number in the array.

If the maximum element in the array occurs several times, you need to display the minimum index.

If the array is empty, output -1.

Please let me know what is wrong in my code?

If I input the array a = { 1, 2, 46, 14, 64, 64 };, for instance, it returns 0, while it should be returning 4.

  public static void Main()
  {
     double[] a = { 1, 9, 9, 8, 9, 2, 2 };
     Console.WriteLine(MaxIndex(a));
  }

  public static double MaxIndex(double[] array)
  {
     var max = double.MinValue;
     int maxInd = 0, maxCount = 0;
     int minIndex = 0;
     var min = double.MaxValue;
     for (var i = 0; i < array.Length; i++)
     {
        if (min > array[i])
        {
           min = array[i];
           minIndex = i;

        }
        if (max == array[i])
           maxCount++;
        if (max < array[i])
        {
           maxCount = 1;
           max = array[i];
           maxInd = i;
        }
     }

     if (array.Length == 0)
        return -1;
     if (maxCount > 1)
        return minIndex;
     return maxInd;
  }
9
  • 10
    Debug. Step through code and see where it goes wrong. Commented Nov 30, 2018 at 8:33
  • 6
    You shouldn't Compare doubles with == Commented Nov 30, 2018 at 8:35
  • 2
    "Please let me know what is wrong in my code." You should tell us what goes whrong. This means describe what problem you´re facing. Do you get any errors? Unexpected results? Afterwards we may able to help you to solve that problem. Commented Nov 30, 2018 at 8:36
  • 1
    if (maxCount > 1) return minIndex; Should be maxIndex Commented Nov 30, 2018 at 8:41
  • 1
    Why you calculate (also) the min if you just want the max? Methods should only do what they're supposed to do Commented Nov 30, 2018 at 8:41

5 Answers 5

7

Your calculation is correct, but you return the wrong variable minIndex instead of maxIndex. Don't do more than necessary in a method. This calculates also the min-index and the count how often it appears, then it does nothing with the results. Here is a compact version:

public static int MaxIndex(double[] array)
{
    var max = double.MinValue;
    int maxInd = -1;

    for (int i = 0; i < array.Length; i++)
    {
        if (max < array[i])
        {
            max = array[i];
            maxInd = i;
        }
    }

    return maxInd;
}

It also sets maxInd = -1 which was part of your requirement. Since MatthewWatson had an objection regarding repeating double.MinValue in the array, here is an optimized version:

public static int MaxIndex(double[] array)
{
    if(array.Length == 0)
        return -1;
    else if (array.Length == 1)
        return 0;

    double max = array[0];
    int maxInd = 0;

    for (int i = 1; i < array.Length; i++)
    {
        if (max < array[i])
        {
            max = array[i];
            maxInd = i;
        }
    }

    return maxInd;
}

If code-readability/maintainability is more important and you don't care of few milliseconds more or less, you could use LINQ (a version that enumerates only once):

int minIndexOfMaxVal = a.Select((num, index) => new {num, index})
    .OrderByDescending(x => x.num)
    .Select(x => x.index)
    .DefaultIfEmpty(-1)
    .First();

This works with every kind of sequence and types not only with arrays and doubles.

Sign up to request clarification or add additional context in comments.

6 Comments

I'm just going to be awkward here and point out that if array[] contains only values equal to double.MinValue this code will incorrectly return -1 rather than 0 - but it seems really unlikely that you'd ever hit that edge case. (Obviously you could fix that by changing if (max < array[i]) to if (maxInd < 0 || max < array[i]), but it's probably not worth it!)
just wondering, why does this code get 3 upvotes, whereas mine gets 4 downvotes? Whilst they basically both do EXACTLY the same, except this one iterates through the ENTIRE array twice...
@MatthewWatson: I've added an optimized version
@DennisVanhout This answer doesn't iterate through the array twice; I don't know why you think that... But your answer DOES iterate through the array twice: First when it calls a.Max(); and then again when you search through the array looking for that max value.
@MatthewWatson each time it fetches Length, it goes through the array to count the length, or am I mistaken in how Length is found? But I'm also just a bit salty about how a valid answer gets downvoted for "not being the most efficient" or "not providing enough info", I mean, bad answers deserve to get downvoted, incomplete answers just deserve a comment on them.
|
3

Try this simple line of code:

int[] numbers = { 1, 2, 3, 4, 5, 4, 3, 2, 1 };
var index = numbers.ToList().IndexOf(numbers.Max());

I think code is simple enough to be self-explanatory.

However, if you aim for performance, conversion to List could be omitted and you could wirte own code to find index of maximum number.

Or even simplier, proposed by @fubo:

Array.IndexOf(numbers, numbers.Max());

6 Comments

To quote @MickyD: "Code-only answers are generally not useful, particularly for beginners. Consider adding a description and point out where the problem was and how you fixed it".
Array.IndexOf(numbers, numbers.Max());
This is obviously homework, so your answer is probably not very useful. It also needs two loops instead of OP's one.
I really don't like solutions that iterate collections twice when it only needs to be iterated once...
@fubo I would venture to suggest that doubling the speed of a method is more than a "little" performance improvement (especially if this is homework!)
|
2

A lot of simplification is possible in this code:

int? maxVal = null; // null because of array of negative value; 
int index = -1;

for (int i = 0; i < array.Length; i++)
{
  int current = array[i];
  if (!maxVal.HasValue || current > maxVal.Value)
  {
    maxVal = current ;
    index = i;
  }
}

Will get you the first index of the max value. If you just need a short code and Don't mind iterating twice a simple linq can do the trick

var index = array.ToList().IndexOf(array.Max());

4 Comments

Whilst simplification or rewrite is good, it's not helpful when a poster just wants to know what is wrong with their existing code. By rewriting it, no one really learns from the excercise. Also you have introduced more advanced concepts such as null and LINQ dot notation, the latter being arguably harder to read
By all means post a simplification as Rango has done but be sure to point out the original issue and fix first
@MickyD, The original issue is a typo, we have a close reason for that. Here I try to adress something more general and usefull for future reader that will never comes with the wrong variable exactly name like that. and 90% of Op code is not related to the function and are just wandering code with no purpuse. If I had to make a metaphore: Op is in a kitchen with flour eggs milk and coffe spray on ever wall. And ask how to make a tea, with a kettle with an unplug kettle in his hands.
If I had to correct the typo I would review the code line by line and produce an answer that will return all those variables. Because I would believe that those wandering line of code have a purpuse.
2

It returns zero, because minIndex is indeed zero: Change minIndex to maxIndex:

if (maxCount > 1) return maxIndex;

In order to compare doubles use the following code instead of ==:

if (Math.Abs(a-b)<double.Epsilon)

3 Comments

To be fair, this is really the correct answer to the question "Why isn't this code working". The other answers are all rewriting the code rather than correcting it. ;)
@Access Denied Why shouldn't we use == operator for doubles?
@ChetanMehra because of double inner representation. When you perform operation with doubles it may loose last digits in base. While doubles will be the same in reality due to rounding they may become a little bit different. See the following article: extremeoptimization.com/resources/Articles/…
-1

Your code in general is way too complicated with way too many variables for what it has to do.

You want the index of the first occurence of the highest value, so there isn't any real reason to store anything but a MaxValueIndex and a MaxValue.

All other variables should be local so garbage collection can dispose of them.

As for what is actually wrong with your code, I can't help out, since the variable names are confusing to me. But here is some code that achieves the goal you want to achieve.

double[] a = { 1, 9, 9, 8, 9, 2, 2 };
var highestValueInArray = a.Max();
var arrayLength = a.Length;

for (int i = 0; i < arrayLength; i++)
{
    if (a[i] == highestValueInArray)
    {
        Console.WriteLine(i);
        break;
    }
}

instead of manually calculating the max value, you can just use YourArray.Max() to get the highest value.

then just iterate through it like normal, but at the first occurence, you break; out of the loop and return the index it is at.

I'm pretty sure there's also a way to use FirstOrDefault in combination with IndexOf, but couldn't get that to work myself.

15 Comments

Code-only answers are generally not useful, particularly for beginners. Consider adding a description and point out where the problem was and how you fixed it
That's quite ok. Looking forward to the update. If good I will be happy to upvote :)
also, this code can be replaced with a single line Console.WriteLine(Array.IndexOf(a, a.Max()))
@DennisVanhout what DragandDrop is saying, suppose your array have n elements. It is obvious that to find a max you need exactly n operations (get element - check if max), but your solution is need n^2 operations (get max - check if current equal max), so for array of 1 000 elements, in worst case your algorithm need to run 1 000 000 operations instead of 1 000. 99.9% of time is waste for nothing (999 000 operations are useless)
"Your code in general is way too complicated with way too many variables" - Again, like with the other answers on this page you have missed the point of the question. i.e. why doesn't it work not how can I make it more efficient
|

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.