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.

This program is for a class project and requires me to read data from a text file then display a list of totals. Here are the instructions/requirements:

The fields in each record will be separated by a comma.

Format: Age, Gender, Marital Status, District#

For example: 18, M, S, 2

The city has 22 districts. The census department wants to see a listing of how many residents are in each district, and a count of residents in each of the following age groups (for all the districts combined): under 18, 18 through 30, 31 through 45, 46 through 64, and 65 or older.

The only way I can get this to work was using a bunch of if statements (which is bad I've been told). Is there a way for me to just read through the data and put the correct age/district number in an array, then display the totals for the each Age Group (groups 1-5) and District (1-22)?

So say each time the program reads age 18 and district 2 for example, it will add 1 to Age Group 1 and 1 to District 2 that element and so on? Then at the end just display the results? I just barely learned about array and I don't know how I can do this.

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[] ageData = new int[900];
        int[] districtDataA = new int[900];
        int[] ageGroup = new int[5];
        int[] districtCount = new int[22];
       int i = 0;
       foreach (string line in File.ReadAllLines("test.txt"))
        {
            string[] fields = line.Split(',');

            ageData[i] = int.Parse(fields[0]);          
            districtDataA[i] = int.Parse(fields[3]);


            if (ageData[i] > 0 && ageData[i] <= 18)
            {
              ageGroup[0] = ageGroup[0] + 1;
            }
            if (ageData[i] > 18 && ageData[i] <= 30)
            {
               ageGroup[1] = ageGroup[1] + 1;
            }
            if (ageData[i] > 30 && ageData[i] <= 45)
            {
               ageGroup[2] = ageGroup[2] + 1;
            }
            if (ageData[i] > 45 && ageData[i] <= 64)
            {
                ageGroup[3] = ageGroup[3] + 1;
            }
            if (ageData[i] >= 65)
            {
                ageGroup[4] = ageGroup[4] + 1;
            }

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

        i++;

    }//End For

    Console.WriteLine("This program provides a list of residents in 5 age groups,");
    Console.WriteLine("And a list of residents in each district 1-22");
    Console.WriteLine("(1.)----------------AGE-GROUP-Count----------------");
    Console.WriteLine("Age Group 18 & under = {0}", ageGroup[0]);
    Console.WriteLine("Age Group 18-30 = {0}", ageGroup[1]);
    Console.WriteLine("Age Group 31-45 = {0}", ageGroup[2]);
    Console.WriteLine("Age Group 46-64 = {0}", ageGroup[3]);
    Console.WriteLine("Age Group 65 & over = {0}", ageGroup[4]);
    Console.WriteLine("(2.)--------------COUNT-PER-DISRTRICT--------------");
    Console.WriteLine("District 1 = {0}", districtCount[0]);
    Console.WriteLine("District 2 = {0}", districtCount[1]);
    Console.WriteLine("District 3 = {0}", districtCount[2]);
    Console.WriteLine("District 4 = {0}", districtCount[3]);
    Console.WriteLine("District 5 = {0}", districtCount[4]);
    Console.WriteLine("District 6 = {0}", districtCount[5]);
    Console.WriteLine("District 7 = {0}", districtCount[6]);
    Console.WriteLine("District 8 = {0}", districtCount[7]);
    Console.WriteLine("District 9 = {0}", districtCount[8]);
    Console.WriteLine("District 10 = {0}", districtCount[9]);
    Console.WriteLine("District 11 = {0}", districtCount[10]);
    Console.WriteLine("District 12 = {0}", districtCount[11]);
    Console.WriteLine("District 13 = {0}", districtCount[12]);
    Console.WriteLine("District 14 = {0}", districtCount[13]);
    Console.WriteLine("District 15 = {0}", districtCount[14]);
    Console.WriteLine("District 16 = {0}", districtCount[15]);
    Console.WriteLine("District 17 = {0}", districtCount[16]);
    Console.WriteLine("District 18 = {0}", districtCount[17]);
    Console.WriteLine("District 19 = {0}", districtCount[18]);
    Console.WriteLine("District 20 = {0}", districtCount[19]);
    Console.WriteLine("District 21 = {0}", districtCount[20]);
    Console.WriteLine("District 22 = {0}", districtCount[21]);


}
}
share|improve this question
2  
Please use spaces instead of tabs here. Tabs mess up the markdown. –  Jamal Apr 6 at 23:03

3 Answers 3

up vote 5 down vote accepted

Just see how your items are related. Think how your code runs, like in slow mo.

I'm falling asleep so I won't try to really explain because it could do more harm than good, so I'll get down to it.

Let me start from the last portion of your code:

    Console.WriteLine("District 1 = {0}", districtCount[0]);
    Console.WriteLine("District 2 = {0}", districtCount[1]);
    Console.WriteLine("District 3 = {0}", districtCount[2]);
    //etc etc

If you find yourself writing repetitive lines like you did there, stop for a moment and think if you could use a loop instead (almost always you can and should):

for (int i = 1; i <= 22; i++)
    {
        Console.WriteLine("District {0} = {1}", i, districtCount[i-1]);
    }

See how short is that?

Now using the same "trick" you can replace aaall your ifs (which should have been if else's in the first place) with just a single line:

districtCount[districtDataA[i] - 1]++;

If you need more explanation to fully understand how or why, just ask and I'll explain further after a good sleep.

BONUS: When you want to type variable = variable + 1; it's shorter and more readable if you type variable++; instead.

share|improve this answer

Your if blocks that are detirmining theage bracket can be shortened:

if (ageData[i] > 0 && ageData[i] <= 18)
{
    ageGroup[0] = ageGroup[0] + 1;
}
if (ageData[i] > 18 && ageData[i] <= 30)
{
    ageGroup[1] = ageGroup[1] + 1;
}
if (ageData[i] > 30 && ageData[i] <= 45)
{
    ageGroup[2] = ageGroup[2] + 1;
}
if (ageData[i] > 45 && ageData[i] <= 64)
{
    ageGroup[3] = ageGroup[3] + 1;
}
if (ageData[i] >= 65)
{
    ageGroup[4] = ageGroup[4] + 1;
}

can become and iterative approach by putting the ranges in a data sctructure. Keeping with your style using arrays.

First you declare:

 int[] ageGroup = new int[5];
 int[] ageGroupLimit = new int[] {17, 30, 45, 64, int.MaxValue};

then you loop through the data:

for(int groupID=0; groupID < ageGroupLimit.Length; groupID++)
{
    if(ageData[i] <= ageGroupLimit[groupID])
    {
         ageGroup[groupID]++;
         break;
    }
}
share|improve this answer
    
Thank you, I am getting an error for the ageGroupLimit.length; saying "'System.Array' does not contain a definition for 'length'..." I'm not sure what the error means. –  proppaganda Apr 7 at 4:57
    
@proppaganda it should be .Length If you use an IDE with auto completion/intellisense you should be able to find these things yourself. –  James Khoury Apr 7 at 4:59
    
Ah I see, thank you. I did not realize it needed to be capitalized. –  proppaganda Apr 7 at 5:17

One way to shorten the code determining the age group is to use a Dictionary. the key is the age the value is the agegroup index. One relatively simple way to fill the dictionary is with LINQ:

AgeGroups = (from i in Enumerable.Range(0, 19)
                select new
                {
                    key = i,
                    value = 0
                }).ToDictionary(x => x.key, x => x.value);
AgeGroups = AgeGroups.Concat((from i in Enumerable.Range(19, 12)
                                select new
                                {
                                    Key = i,
                                    Value = 1
                                }).ToDictionary(x => x.Key, x => x.Value)).ToDictionary(x => x.Key, x => x.Value);
AgeGroups = AgeGroups.Concat((from i in Enumerable.Range(31, 15)
                                select new
                                {
                                    Key = i,
                                    Value = 2
                                }).ToDictionary(x => x.Key, x => x.Value)).ToDictionary(x => x.Key, x => x.Value);
AgeGroups = AgeGroups.Concat((from i in Enumerable.Range(46, 19)
                                select new
                                {
                                    Key = i,
                                    Value = 3
                                }).ToDictionary(x => x.Key, x => x.Value)).ToDictionary(x => x.Key, x => x.Value);

This creates a dictionary with 65 elements with the value of each element the appropriate agegroup index.

You build this once.

Shortening the district code is simply matter of subtracting 1 from the district number to get the appropriate index.

If there's a chance the size of the data wil be large, you should consider reading each line separately rather than all of them together.

With all this in mind the code to fill your arrays looks like this:

using(StreamReader lines = new StreamReader("test.txt"))
{
    int i = 0;
    while(!lines.EndOfStream)
    {
        string[] fields = lines.ReadLine().Split(',');

        ageData[i] = int.Parse(fields[0]);          
        districtDataA[i] = int.Parse(fields[3]);

        if(AgeGroups.ContainsKey(ageData[i]))
            ageGroup[AgeGroups[ageData[i]]]++;
        else
            ageGroup[4]++;
        districtCount[districtDataA[i++] - 1]++;
    }
}

Notice how you still need one loop but you're not storing the whole file in memory. Also, the only if statement is to check for the top age group.

share|improve this answer

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.