HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Summing up distinct elements in steps

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
summingdistinctstepselements

Problem

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;  scores[i]) {
                low = scores[i];
            } //record lowest value

            if (high  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

Solution

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:

  • Find the highest item of the array



  • Find the lowest item of the array



  • Find the number of occurance of the highest item of the array



  • Find the number of occurance of the lowest item of the array



  • Remove the first highest item found



  • Remove the first lowest item found



  • Sum together the remaining items



  • 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  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

Code Snippets

for (int i = 0; i < scores.Length; i++) {
for (int i = 0; i < scores.Length; i++) 
{
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
}

Context

StackExchange Code Review Q#60975, answer score: 6

Revisions (0)

No revisions yet.