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;
  }
Community
  • 1
  • 1

5 Answers5

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.

Tim Schmelter
  • 450,073
  • 74
  • 686
  • 939
  • 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!) – Matthew Watson Nov 30 '18 at 09:04
  • 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... – Dennis Vanhout Nov 30 '18 at 09:07
  • 3
    @MatthewWatson: I've added an optimized version – Tim Schmelter Nov 30 '18 at 09:20
  • 1
    @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. – Matthew Watson Nov 30 '18 at 10:00
  • @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. – Dennis Vanhout Nov 30 '18 at 11:46
  • @DennisVanhout: an array has a property `Length` which doesn't need to be calculated. You confuse it with [`Enumerable.Count`](https://referencesource.microsoft.com/#System.Core/System/Linq/Enumerable.cs,41ef9e39e54d0d0b) which is an extension method often used in LINQ queries. That method checks if the source is a collection with a `Count`-property, then it will use that, otherwise it will enumerate the source-collection/query. But this is not the case with arrays (or lists). So even if i would use `array.Count()` it would not need to enumerate it because an array implements `ICollection`. – Tim Schmelter Nov 30 '18 at 12:04
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());
Michał Turczyn
  • 32,028
  • 14
  • 47
  • 69
  • 1
    [To quote @MickyD](https://stackoverflow.com/questions/53553901/c-sharp-write-a-program-to-find-an-index-of-max-item-in-an-array#comment93973351_53554006): "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". – Uwe Keim Nov 30 '18 at 08:44
  • 4
    `Array.IndexOf(numbers, numbers.Max());` – fubo Nov 30 '18 at 08:46
  • 1
    This is obviously homework, so your answer is probably not very useful. It also needs two loops instead of OP's one. – Tim Schmelter Nov 30 '18 at 08:48
  • 5
    I really don't like solutions that iterate collections twice when it only needs to be iterated once... – Matthew Watson Nov 30 '18 at 08:49
  • @MatthewWatson `2O` = `O` so I think the readability/maintianability of the code is more important compared to the little performance improvement. OP has just 6 items in the array – fubo Nov 30 '18 at 08:53
  • 1
    @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!) – Matthew Watson Nov 30 '18 at 08:57
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());
Drag and Drop
  • 2,672
  • 3
  • 25
  • 37
  • 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 –  Nov 30 '18 at 11:45
  • 1
    By all means post a simplification as Rango has done but be sure to point out the original issue and fix first –  Nov 30 '18 at 11:54
  • @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. – Drag and Drop Dec 04 '18 at 08:41
  • 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. – Drag and Drop Dec 04 '18 at 08:44
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)
Access Denied
  • 8,723
  • 4
  • 42
  • 72
  • 1
    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. ;) – Matthew Watson Nov 30 '18 at 10:09
  • @Access Denied Why shouldn't we use == operator for doubles? – Chetan Mehra Nov 30 '18 at 10:45
  • @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: https://www.extremeoptimization.com/resources/Articles/FPDotNetConceptsAndFormats.aspx – Access Denied Nov 30 '18 at 11:25
-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.

  • 5
    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 –  Nov 30 '18 at 08:41
  • @MickyD I'm a beginner myself, just saw a easy, short solution, but I'll add some commentary :) – Dennis Vanhout Nov 30 '18 at 08:42
  • 1
    That's quite ok. Looking forward to the update. If good I will be happy to upvote :) –  Nov 30 '18 at 08:44
  • your code wil calculate `a.Max()` `a.Length` times (7 times in this case). Why not to find it only once before for-loop? – vasily.sib Nov 30 '18 at 08:49
  • your `Max` is in the loop.. I mean it will calculate ethe max on each iteration.. – Drag and Drop Nov 30 '18 at 08:49
  • 2
    also, this code can be replaced with a single line `Console.WriteLine(Array.IndexOf(a, a.Max()))` – vasily.sib Nov 30 '18 at 08:51
  • Thats O(n^2) solution. to an O(n) problem. – Drag and Drop Nov 30 '18 at 08:53
  • first I had the a.Max() appointed to a seperate value, only reason I put it in the loop again was just to save some lines. and @vasily.sib I was aware of that, hence why I mentioned that as well... still no reason to downvote the solution though... I mean, it might not be the best solution, but it is A solution. – Dennis Vanhout Nov 30 '18 at 08:57
  • @DennisVanhout I didn't downvote your solution, because it is A solution. Not the best one, but ok. `O(n)` and `O(n^2)` is a [Big O notation](https://en.wikipedia.org/wiki/Big_O_notation) from [Computational complexity theory](https://en.wikipedia.org/wiki/Computational_complexity_theory). – vasily.sib Nov 30 '18 at 09:08
  • @vasily.sib thanks for the info. And sorry, wasn't meant directly at you, but I can see how it came across that way. – Dennis Vanhout Nov 30 '18 at 09:10
  • 2
    @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) – vasily.sib Nov 30 '18 at 09:17
  • @DennisVanhout Behind the scenes, `a.Max()` will loop through the entire array once to find the maximum value. And then you do the `for` loop through the array, which (as a worst case) can loop through the entire array again. Each loop-through is 'n', so this is an O(n^2) algorithm. – John Go-Soco Nov 30 '18 at 09:32
  • The same line of code in a different order is not the same code, a function and block of code is not the same, return value and best and worst case senario are totaly different. It respect the requirement of returnning -1. One had text context and explanation. Ps: Im not rude or condescending, I'm trying to explain why one could have downvoted the first version of the code. If we need a metaphore on is a standard car with standard security, the other is a weird car that need a refill every Km, with no door. – Drag and Drop Nov 30 '18 at 09:49
  • @JohnGo-Soco it's called O(n) when you loop through array twice. – Access Denied Nov 30 '18 at 10:37
  • 2
    _"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_ –  Nov 30 '18 at 11:48