5
\$\begingroup\$
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.IO;

namespace Project2
{
    class Program
    {
        static void Main(string[] args)
        {
            int[] ageForPeople = new int[10000];
            int[] DistrictForCensus = new int[10000];
            int[] agesForCensusGroups = new int[5];
            int[] maxDistrictAmount = new int[22];
            int placeValueHolder = 0;
            foreach (string line in File.ReadAllLines("census.txt"))
            {
                string[] fields = line.Split(',');

                ageForPeople[placeValueHolder] = int.Parse(fields[0]);
                DistrictForCensus[placeValueHolder] = int.Parse(fields[3]);


                if (ageForPeople[placeValueHolder] > 0 && ageForPeople[placeValueHolder] <= 18)
                {
                    agesForCensusGroups[0]++;
                }
                if (ageForPeople[placeValueHolder] > 18 && ageForPeople[placeValueHolder] <= 30)
                {
                    agesForCensusGroups[1]++;
                }
                if (ageForPeople[placeValueHolder] > 30 && ageForPeople[placeValueHolder] <= 45)
                {
                    agesForCensusGroups[2]++;
                }
                if (ageForPeople[placeValueHolder] > 45 && ageForPeople[placeValueHolder] <= 64)
                {
                    agesForCensusGroups[3]++;
                }
                if (ageForPeople[placeValueHolder] >= 65)
                {
                    agesForCensusGroups[4]++;
                }

                //District Count info
                if (DistrictForCensus[placeValueHolder] == 1)
                {
                    maxDistrictAmount[0]++;
                }
                if (DistrictForCensus[placeValueHolder] == 2)
                {
                    maxDistrictAmount[1]++;
                }
                if (DistrictForCensus[placeValueHolder] == 3)
                {
                    maxDistrictAmount[2]++;
                }
                if (DistrictForCensus[placeValueHolder] == 4)
                {
                    maxDistrictAmount[3]++;
                }
                if (DistrictForCensus[placeValueHolder] == 5)
                {
                    maxDistrictAmount[4]++;
                }
                if (DistrictForCensus[placeValueHolder] == 6)
                {
                    maxDistrictAmount[5]++;
                }
                if (DistrictForCensus[placeValueHolder] == 7)
                {
                    maxDistrictAmount[6]++;
                }
                if (DistrictForCensus[placeValueHolder] == 8)
                {
                    maxDistrictAmount[7]++;
                }
                if (DistrictForCensus[placeValueHolder] == 9)
                {
                    maxDistrictAmount[8]++;
                }
                if (DistrictForCensus[placeValueHolder] == 10)
                {
                    maxDistrictAmount[9]++;
                }
                if (DistrictForCensus[placeValueHolder] == 11)
                {
                    maxDistrictAmount[10]++;
                }
                if (DistrictForCensus[placeValueHolder] == 12)
                {
                    maxDistrictAmount[11]++;
                }
                if (DistrictForCensus[placeValueHolder] == 13)
                {
                    maxDistrictAmount[12]++;
                }
                if (DistrictForCensus[placeValueHolder] == 14)
                {
                    maxDistrictAmount[13]++;
                }
                if (DistrictForCensus[placeValueHolder] == 15)
                {
                    maxDistrictAmount[14]++;
                }
                if (DistrictForCensus[placeValueHolder] == 16)
                {
                    maxDistrictAmount[15]++;
                }
                if (DistrictForCensus[placeValueHolder] == 17)
                {
                    maxDistrictAmount[16]++;
                }
                if (DistrictForCensus[placeValueHolder] == 18)
                {
                    maxDistrictAmount[17]++;
                }
                if (DistrictForCensus[placeValueHolder] == 19)
                {
                    maxDistrictAmount[18]++;
                }
                if (DistrictForCensus[placeValueHolder] == 20)
                {
                    maxDistrictAmount[19]++;
                }
                if (DistrictForCensus[placeValueHolder] == 21)
                {
                    maxDistrictAmount[20]++;
                }
                if (DistrictForCensus[placeValueHolder] == 22)
                {
                    maxDistrictAmount[21]++;
                }

                placeValueHolder++;

            }//End For

            Console.WriteLine("This provides a list of residents in 5 different age groups,");
            Console.WriteLine("Also it provides a list of residents in each of the district 1-22");
            Console.WriteLine("-------Ages of people, in all districts-------");
            Console.WriteLine("Age Group 18 & under = {0}", agesForCensusGroups[0]);
            Console.WriteLine("Age Group 18-30 = {0}", agesForCensusGroups[1]);
            Console.WriteLine("Age Group 31-45 = {0}", agesForCensusGroups[2]);
            Console.WriteLine("Age Group 46-64 = {0}", agesForCensusGroups[3]);
            Console.WriteLine("Age Group 65 & over = {0}", agesForCensusGroups[4]);
            Console.WriteLine("-------Amount of people in each district-------");
            for (int z = 1; z <= 22; z++)
            {
                Console.WriteLine("District {0} = {1}", z, maxDistrictAmount[z - 1]);
            }


        }
    }

}

This is what I have so far, I do not know how to further condense the if statements. For example: the districtForCensus displays sequentially all districts. Is there a way to make it go one after another, without 22 if statements?

\$\endgroup\$
6
\$\begingroup\$
  1. The first if group can be slightly simplified using the else if:

    if (ageForPeople[placeValueHolder] <= 0)
    {
        // Trigger error.
    }
    else if (ageForPeople[placeValueHolder] <= 18)
    {
        agesForCensusGroups[0]++;
    }
    else if (ageForPeople[placeValueHolder] <= 30)
    {
        agesForCensusGroups[1]++;
    }
    else if (ageForPeople[placeValueHolder] <= 45)
    {
        agesForCensusGroups[2]++;
    }
    else if (ageForPeople[placeValueHolder] <= 64)
    {
        agesForCensusGroups[3]++;
    }
    else
    {
        agesForCensusGroups[4]++;
    }
    
  2. The second if group can be replaced with the following single line:

    maxDistrictAmount[DistrictForCensus[placeValueHolder] - 1]++;
    

    To make sure that DistrictForCensus[placeValueHolder] is within the allowed bounds:

    // District Count info
    int district = DistrictForCensus[placeValueHolder];
    if (district >= 1 && district <= 22)
    {
        maxDistrictAmount[district - 1]++;
    }
    
  3. Side note. Do not use capitalized names for local variables. Rename DistrictForCensus to districtForCensus.
\$\endgroup\$
3
\$\begingroup\$

This review is in addition to Dmitry's.


You do these lookups multiple times: ageForPeople[placeValueHolder], DistrictForCensus[placeValueHolder], yet outside the foreach they aren't used. Why then define them outside the foreach, and why is this an array? Why not simply assign the result of int.Parse(fields[0]) and int.Parse(fields[3]) to a local variable?


agesForCensusGroups[0] etc doesn't mean anything. IMHO this should be a Dictionary<T,T> with a meaningful enum as the key. That way you can simply loop through the enum's values and use its Description attrribute to generate the output.


This is still a fairly short program with limited functionality, but you could already consider moving both the agesForCensusGroups logic and the maxDistrictAmount logic each in a method of their own, perhaps even to a class of their own. Keep your Main lean and clean, use it to stitch together the various independent parts. Ditto for the output part: move that to a method of its own, and provide parameters with all the necessary data.

\$\endgroup\$
3
\$\begingroup\$

When you have cascading if-condition logic like you have in your first if-block, consider using a lookup table instead. Lookup tables pre-process the data in a way that makes the use of the data much faster.

How would this work? First, define where your boundaries are:

private static readonly int[] ageBounds = {18, 30, 45, 64};

The above array will be used to say: Anything less than or equal to the value at index X will have have bucket X. We use this table to build the actual lookup table:

private static int[] BuildAgeMap()
{
    int[] ret = new int[ageBounds[ageBounds.Length - 1]];
    int limit = 0;
    for (int i = 0; i < ret.Length; i++) {
        if (i > ageBounds[limit]) {
            limit++;
        }
        ret[i] = limit;
    }

    return ret;
}

public static readonly int[] AGES = BuildAgeMap();

What does this accomplish? We now have an array called AGES, with the contents:

0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,...
1,1,1,1,1,1,1,1,1,1,1,1,...
2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,...
3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3

Now, for someone who is age 33, you can get their group by going to AGES[33].

Put this in a simple wrapper method (that takes care of large ages), and you have:

public static int GetAgeGroup(int age) {
    return age >= AGES.Length ? ageBounds.Length : AGES[age];
}

Now, in your CSV parsing, you just need to do:

agesForCensusGroups[GetAgeGroup( .... )]++;

Note that there is no if-statement at all!

You can see how this all works in the following ideone example;

\$\endgroup\$
  • \$\begingroup\$ Is there really a need for a lookup table if you have the age bounds array? Sure it will be faster, but is the additional memory worth it? Won't it be fast enough to loop through the age bounds array and determine the group from that? \$\endgroup\$ – Simon Forsberg Apr 7 '15 at 18:56
  • \$\begingroup\$ That is a keen observation, @SimonAndréForsberg. You're probably mistaken on a couple of things though (as well as me). As I look at it, I can't actually say for sure that the lookup table is faster. We're talking about 256 or so bytes in memory that will be cached very close to the core (L1 cache), but, 4 if/else statements may be faster than 1 L1 lookup. It's a margin call. I would guess that all the systems (if/else, 4-value table, and all-value table) will be about as fast as each other in this situation.... Code Readability is probably the important thing, and I am not sure mine is best. \$\endgroup\$ – rolfl Apr 7 '15 at 19:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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