Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My current task is to find a score from an array where the highest/lowest scores have been taken away, and if the highest/lowest occur more than once (ONLY if they occur more than once), one of them can be added:

E.g. int[] scores = [4, 8, 6, 4, 8, 5] therefore the final addition will be \$\sum{4,8,6, 5} = 23 \$.

Another condition of the task is that LINQ cannot be used, as well as any of the System.Array methods (you can see by my previously ask questions that has been a bit of a pain for me, since I solved this with LINQ in less than 5 minutes).

I have working code the solves the problem but the task requires multiple methods/functions. I have been trying to restructure the program but with all sorts of issues.

using System;
using System.Collections.Generic;
//using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Scoring {
class Program {

    static int highOccurrence = 0;
    static int lowOccurrence = 0;
    //static int high; <------
    //static int low; <------

    static void Main(string[] args) {
        int[] scores = { 4, 8, 6, 4, 8, 5 };

        findScore(scores);
        ExitProgram();
    }

    static int findOccurrence(int[] scores, int low, int high) { //find the number of times a high/low occurs

        for (int i = 0; i < scores.Length; i++) {
            if (low == scores[i]) {
                lowOccurrence++;
                //record number of time slow occurs
            }

            if (high == scores[i]) {
                highOccurrence++;
                //record number of times high occurs                }
        }
        return highOccurrence;
    }

    static int findScore(int[] scores) { //calculates score, needs to be restructured

        int[] arrofNormal = new int[scores.Length];

        //int low = scores[0]; <----This is where the issue is
        //int high = scores[0]; <----- ^^^^^
        int total = 0;

        for (int i = 0; i < scores.Length; i++) {             
            if (low > scores[i]) {
                low = scores[i];
            } //record lowest value

            if (high < scores[i]) {
                high = scores[i];
                //record highest value
            }
        }
        for (int x = 0; x < scores.Length; x++) {
            if (scores[x] != low && scores[x] != high) {
                arrofNormal[x] = scores[x];
                //provides the total of the scores (not including the high and the low) 
            }
            total += arrofNormal[x];
        }

        findOccurrence(scores, low, high);
        if (highOccurrence > 1) { //if there is more than 1 high (or 1 low) it is added once into the total
            total += high;
            if (lowOccurrence > 1) {
                total += low;
            }
        }
        Console.WriteLine("Sum = " + total);
        return total; //remove not all code paths return.. error
    }        
    static void ExitProgram() {
        Console.Write("\n\nPress any key to exit program: ");
        Console.ReadKey();
    }//end ExitProgram
}
}

I have placed arrows in the code above to show where my issue is. If I try to declare "high" and "low" as global variables, my final answer is always a few numbers off, buy if I leave the variables declared as high = scores[0] etc, I will get the right answer.

What I want ideally is to have separate methods for each step of the calculation, so right now I have method for finding the number of times a specific value shows up in the array.

The next I would like to do is finding the highest/lowest value in the array, one method would do the final calculation, and the final one would write the results into the console window. The last two parts (finding the high/low and final calculation) are currently in the find score method.

share|improve this question
    
The first sentence in your question is somewhat misleading. the example tells me that you want to sum distinct numbers, and has nothing to do with the highest or lowest number. if this is not true please elaborate. As is, you are doing way more than you need to to come up with the sum of these numbers minus duplicates. –  Malachi Aug 25 at 18:29
    
do you happen to have some more test cases so I can fully understand what you are aiming for? –  Malachi Aug 25 at 19:05
1  
"Another condition of the task is that LINQ cannot be used" - that's just mean –  Dan Pantry Aug 25 at 20:08
    
What is the expected result for e.g [4, 8, 4, 4, 8] –  Heslacher Aug 26 at 7:29

4 Answers 4

up vote 6 down vote accepted

My current task is to find a score from an array where the highest/lowest scores have been taken away, and if the highest/lowest occur more than once (ONLY if they occur more than once), one of them can be added

So the programflow can be written as:

  1. Find the highest item of the array
  2. Find the lowest item of the array
  3. Find the number of occurance of the highest item of the array
  4. Find the number of occurance of the lowest item of the array
  5. Remove the first highest item found
  6. Remove the first lowest item found
  7. Sum together the remaining items
  8. Export/Print the results

So you will have 8 methods without the ExitProgram() method.

Review

Naming

Based on the naming conventions for C#, all methodnames should be written using the PascalCasing style. So findOccurrence should be FindOccurrence and findScore should be FindScore

So based on the list of tasks above the methods should be named

  • FindHighestItem() or GetHighestItem()
  • FindLowestItem() or GetLowestItem();
  • FindNumberOfHighestOccurance() or GetCountOfOccurance()
  • FindNumberOfLowestOccurance() or GetCountOfOccurance()
  • RemoveHighestItems() or SetArrayitemsByValueToZero()
  • RemoveLowestItems() or SetArrayitemsByValueToZero()
  • GetHighestItemIndex()
  • GetLowestItemIndex()
  • GetNormalizedArray() which returns a new array or SetArrayitemsByValueToZero() which manipulates the items in the array
  • SumItems()
  • Print()

Style

Opening brackets should be placed on the line below the statement so

for (int i = 0; i < scores.Length; i++) {  

should be

for (int i = 0; i < scores.Length; i++) 
{  

Programflow

By combining some of these tasks you are violating the single responsible principle.
The findScore() method

  • is searching for the highest and lowest item in the array
  • removes the highest and lowest item from the array (basically add only the items which aren't highest or lowest to the new array)
  • is summing the items of this new array stated above
  • is printing/exporting the sum

Refactoring

For getting the highest/lowest item in the array we can use the Math.Max() and Math.Min() methods. For finding the highest item we will initialize the var highestValue with Int32.MinValue and for finding the lowest item we will initialize the var lowestValue with Int32.MaxValue.
Based of the comment as it is not allowed for this assignment to use Math.Min() or Math.Max() I have changed the methods

    private int GetLowestItem(int[] items)
    {
        int lowestItem = Int32.MaxValue;
        foreach (int item in items)
        {
            // lowestItem = Math.Min(item, lowestItem);
            if (item < lowestItem)
            {
                lowestItem = item;
            }
        }
        return lowestItem;
    }

    private int GetHighestItem(int[] items)
    {
        int highestItem = Int32.MinValue;
        foreach (int item in items)
        {
            // highestItem = Math.Max(item, highestItem);
            if (item > highestItem)
            {
                highestItem = item;
            }
        }
        return highestItem;
    }

    private int GetCountOfOccurance(int[] items, int comparingValue)
    {
        int count = 0;
        foreach (int item in items)
        {
            if (item == comparingValue)
            {
                count++;
            }
        }
        return count;
    }  

private int GetHighestItemIndex(int[] items, int highestItem)
{
    for(int i = 0 ; i<items.Length ; i++)
    {
        if(items[i] == highestItem)
        {
            return i;
        }
    }
    return -1;
}  

private int GetLowestItemIndex(int[] items, int lowestItem)
{
    // to be filled by you
}  


Next we should create a new calssed named BoundaryItem which holds the min and max values of the array.

class BoundaryItem
{
    internal int Max { get; set; }
    internal int Min { get; set; }
}  

now we add a method to set each item of the array to zero if the value is either the max or the min of the array.

private void SetArrayitemsByValueToZero(int[] items, BoundaryItem boundaryItem)
{
    for (int i = 0; i < items.Length; i++)
    {
        if (items[i] == boundaryItem.Min || items[i] == boundaryItem.Max)
        {
            items[i] = 0;
        }
    }
}

Next we will add a 2 overloaded SumItems() methods

public int SumItems(int[] items)
{
    BoundaryItem boundaryItem = PreProcessArray(items);

    return SumItems(items, boundaryItem);
}

private int SumItems(int[] items, BoundaryItem boundaryItem)
{

    int sum = boundaryItem.Min + boundaryItem.Max;
    foreach (int item in items)
    {
        sum += item;
    }
    return sum;
}

and last we add the missing PreProcessArray() method

private BoundaryItem PreProcessArray(int[] items)
{
    BoundaryItem boundaryItem = new BoundaryItem();

    boundaryItem.Min = GetLowestItem(items);
    boundaryItem.Max = GetHighestItem(items);

    int minOccurance = GetCountOfOccurance(items, boundaryItem.Min);
    int maxOccurance = GetCountOfOccurance(items, boundaryItem.Max);

    SetArrayitemsByValueToZero(items, boundaryItem);

    if (minOccurance < 2) { boundaryItem.Min = 0; }
    if (maxOccurance < 2) { boundaryItem.Max = 0; }

    return boundaryItem;
}  

But wait, we can still do better, if the amount of methods needed would not matter. Instead of GetLowestItem() and GetHighestItem() we create a method GetBoundaryItem()

private BoundaryItem GetBoundaryItem(int[] items)
{
    BoundaryItem boundaryItem = new BoundaryItem();
    boundaryItem.Min = Int32.MaxValue;
    boundaryItem.Max = Int32.MinValue;

    foreach (int item in items)
    {
        if (item < boundaryItem.Min)
        {
            boundaryItem.Min = item;
        }
        else if (item > boundaryItem.Max)
        {
            boundaryItem.Max = item;
        }
    }
    return boundaryItem;
}  

and change the PreProcessArray() method

private BoundaryItem PreProcessArray(int[] items)
{
    BoundaryItem boundaryItem = GetBoundaryItem(items);

    int minOccurance = GetCountOfOccurance(items, boundaryItem.Min);
    int maxOccurance = GetCountOfOccurance(items, boundaryItem.Max);

    SetArrayitemsByValueToZero(items, boundaryItem);

    if (minOccurance < 2) { boundaryItem.Min = 0; }
    if (maxOccurance < 2) { boundaryItem.Max = 0; }

    return boundaryItem;
}  

The printing I will leave to you. As you see I have made instance methods out of the static methods. So it would be a good idea to create a class named Scoring where you place these methods.Then you would call this like

static void Main(string[] args) {

    Scoring scoring = new Scoring();

    int[] scores = { 4, 8, 6, 4, 8, 5 };
    int sum = scoring.SumItems(scores);
    // now print the result
    ExitProgram();
}  

See: When to Use Static Classes in C#

As I have written the refactorings I came to the conclusion that we just need to get the index of the highest/lowest items so we can skip those items at composing the new array. After the coment from Malachi I realized that using the index of the highest/lowest item will be an invalid aproach if these values are contained more than 2 times in the array.
This implementation assumes that for the given array [1 ,1 ,1 ,2 ,3 ,3 ,3] it's sum should be 1 + 2 + 3 .

share|improve this answer
    
That is the basic idea yes, but I have paired up: 1 and 2, 3 and 4, 5 and 6. –  user3496101 Aug 25 at 8:53
    
Okay thanks, I am also rewriting the program from scratch now, but I am still very interested in what you have to say. –  user3496101 Aug 25 at 8:59
    
Thanks a lot of the reply, I have read through made changed to my current code. But unfortunately I am not allowed to use the math methods you described above. I'm sorry, I should have been clearer in the question itself. I will completely understand if you want to leave it like it is here. Thanks again for the help :) –  user3496101 Aug 25 at 9:58
    
you are creating more arrays then necessary by returning arrays. you should steer away from that unless you have to return an array. you also have no need to find the index of the highest and lowest value in the array, there could be more than one. –  Malachi Aug 25 at 20:18
1  
@Malachi Finding the index of the highest and lowest items will work only correctly if the highest or lowest value will be contained max 2 times in the array. I have changed my answer. –  Heslacher Aug 26 at 10:32

In addition to the comments in Heslacher answer, I have few more notes on your code:

  • remove lines instead of commenting them. What's the use of lines such as //using System.Linq;? You're not going to use it, so you don't even need to mention it.
  • try to avoid using static variables and replace them with instance variables. Having an instance-local state is much better and will help your code to be much easier to extend and/or reuse.

I also noticed this comment //remove not all code paths return.. error and it is a bit of a red flag to me. It is the only return statement in that method so you should have it to actually return something, not only to make the compiler happy.

share|improve this answer
    
Thanks for the reply, I have fixed the issues you highlights, but could you give an example of the "instance variables" you mentioned? –  user3496101 Aug 25 at 9:26
    
Have a look at this Q&A. It is related to Java but the concept is exactly the same in C# –  mariosangiorgio Aug 25 at 9:30
    
@mariosangiorgio, you should have said that if OP doesn't want the method to return anything that they could write the method like this static void findScore(int[] scores) { instead of static int findScore(int[] scores) {, but I agree with you, methods shouldn't print out the sum that should be returned. –  Malachi Aug 25 at 20:21

In my opinion, the real problem you have with your algorithm is the number of time you go through your array. Going through it only once would bring better performance. I'd have a hard time explaining what I changed without showing you how I rewrote your algo. I decided to add all the values, since you can't know what is the highest/lowest score until you finished looping the array, then substract the highest/lowest score (more than once if needed).

 public int CalculateScore(int[] scores)
 {
    int lowestValue = int.MaxValue,
    highestValue = int.MinValue,
    ammountOfHighestValue = 1,
    ammountOfLowestValue = 1,
    finalScore = 0;

    foreach (int score in scores)
    {
       finalScore += score;

       if (score < lowestValue)
       {
          lowestValue = score;
          ammountOfLowestValue = 1; //We need to reset the ammount
       }
       else if (score > highestValue)
       {
          highestValue = score;
          ammountOfHighestValue = 1; //We need to reset the ammount
       }
       else if (score == lowestValue)
          ammountOfLowestValue++;
       else if (score == highestValue)
          ammountOfHighestValue++;
    }

    if (ammountOfHighestValue > 1)
       //This way, we keep the highest score once.
       finalScore -= ((ammountOfHighestValue - 1) * highestValue); 
    else
       finalScore -= highestValue; //The value is there once, we remove it.

    if (ammountOfLowestValue > 1)
       finalScore -= ((ammountOfLowestValue - 1) * lowestValue); //Same as highest
    else
       finalScore -= lowestValue;

    return finalScore;
 }

Note that this answer is far from the best in a OOP point of view, but I think in performance it would be better since you have a O(n) complexity.

share|improve this answer
    
I was thinking of posting exactly that. –  the_lotus Aug 25 at 17:49
    
I hope you took some time to think of this solution, because it took me awhile to find this approach :P –  TopinFrassi Aug 25 at 17:51
    
this could use some review, you should post it as a question. someone might be able to show you a way to do this without all the if then statements. it feels a little icky... lol. I had a similar idea in my code, but I have a little more overhead. –  Malachi Aug 25 at 20:25
    
@Malachi You are not mistaken, the code is pretty ugly! I can think of ways to remove the multiple if but I'm not sure they would make the code easier to read or maintain.. I will post it as a question to see what other ideas people might have –  TopinFrassi Aug 25 at 22:10
    
If you ever want to see how it goes : codereview.stackexchange.com/questions/61035/… –  TopinFrassi Aug 25 at 22:22

I figured out how I would do this and did it step by step, much like I assume that you have done it. I like to build things rather than destroy them, so I started at the beginning of the problem


First, I created methods to get the highest value and the lowest value.

I used a foreach instead of trying to use indexes which allowed me to make it clearer what was going on as well. I used a ternary statement inside of each method because I like the way they look as opposed to if then statements, I turned your if then statements into these ternary statements, they produce the same result, if you are not allowed to use them you can insert your if then statements in here.

    static int getHighestScore(int[] integerArray)
    {
        var high = 0;
        foreach (int number in integerArray)
        {
            high = high < number ? number : high;
        }
        return high;
    }

    static int getLowestScore(int[] integerArray)
    {
        var low = int.MaxValue;
        foreach (int number in integerArray)
        {
            low = low > number ? number : low;
        }
        return low;
    }

This way I can just get the high number and the low number should I need them for something, and I can take this method out of the program and insert it into others should I need something like this in the future.

Note: If we are going to be dealing with Negative numbers we would need to change the getHighestScore method it account for that, so instead of

var high = 0;

we would have:

var high = int.MinValue;

Then I decided that I wanted the sum with our the highest and the lowest numbers, so I created another method that would sum the values of the array if they were not the highest or lowest value.

What I did here was add all the numbers of the array unless they were the high and low numbers.

static int sumWithoutHighAndLowScores(int[] integerarray)
{
    int sum = 0;
    int high = getHighestScore(integerarray);
    int low = getLowestScore(integerarray);
    foreach (int number in integerarray)
    {
        if (number != high && number != low)
        {
            sum += number;
        }
    }
    return sum;
}

Now this doesn't tell me the sum if I have more than 1 high or low number, so what I did was count the high numbers and the low numbers and then if there is more than one I added it once to the sum.

static int sumWithoutHighAndLowScores(int[] integerarray)
{
    int sum = 0;
    int high = getHighestScore(integerarray);
    int low = getLowestScore(integerarray);
    foreach (int number in integerarray)
    {
        if (number != high && number != low)
        {
            sum += number;
        }
    }
    return sum;
}

I didn't see an example/test case where there was multiple values that were not the high or the low, so I haven't coded for that scenario.

I may attempt a version that only counts unique numbers and high/low(if there is more than one) later.


Here is the full code and implementation.

static void Main(string[] args)
{
    // take an array and sum the distinct numbers
    int[] numberArray = { 4, 8, 6, 4, 8, 5 };
    int[] numberArray2 = { 4, 4, 5, 6, 8, 8 };

    Console.WriteLine(sumSpecial(numberArray).ToString());
    Console.WriteLine(sumSpecial(numberArray).ToString());

    Console.ReadLine();
}

static int getHighestScore(int[] integerArray)
{
    var high = 0;
    foreach (int number in integerArray)
    {
        high = high < number ? number : high;
    }
    return high;
}

static int getLowestScore(int[] integerArray)
{
    var low = int.MaxValue;
    foreach (int number in integerArray)
    {
        low = low > number ? number : low;
    }
    return low;
}

static int sumWithoutHighAndLowScores(int[] integerarray)
{
    int sum = 0;
    int high = getHighestScore(integerarray);
    int low = getLowestScore(integerarray);
    foreach (int number in integerarray)
    {
        if (number != high && number != low)
        {
            sum += number;
        }
    }
    return sum;
}

//sum of numbers using high or low only if there is a duplicate of high or low
static int sumSpecial(int[] integerArray)
{
    var sum = sumWithoutHighAndLowScores(integerArray);
    var high = getHighestScore(integerArray);
    var low = getLowestScore(integerArray);

    var highs = 0;
    var lows = 0;
    foreach (int number in integerArray)
    {
        if (number == high) { highs++; }
        if (number == low) { lows++; }
    }
    if (lows > 1) { sum += low; }
    if (highs > 1) { sum += high; }

    return sum;
}
share|improve this answer
    
You need to initialize the high variable in the getHighestScore() method with int.MinValue. For an array like [-1, -1, -2, -3, -3] your method would fail. –  Heslacher Aug 26 at 10:50
    
@Heslacher, I assumed that we were only dealing with positive numbers because the test case given was only positive and there was nothing said of negative numbers, but noted. Thank you. It has been noted. –  Malachi Aug 26 at 13:17

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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