3
\$\begingroup\$

The program takes census data from a text file in the format age, gender, marital status, district. It then gives a list of residents by district (there are 22) and by 5 age groups. Any suggestions for improvement?

The only topics we have covered so far in class are methods, decisions, looping, and arrays.

static void Main(string[] args)
    {
        FileStream fStream = new FileStream("test.txt", FileMode.Open, FileAccess.Read);
        StreamReader inFile = new StreamReader(fStream);

        string inputRecord = "";
        string[] fields;
        int[] ageData = new int[1000];
        string[] genderData = new string[1000];
        string[] maritalData = new string[1000];
        int[] districtData = new int[1000];
        int[] countDist = new int[23];
        int[] ageGroup = new int[5];

        inputRecord = inFile.ReadLine();

        int i = 0;
        while (inputRecord != null)
        {
            fields = inputRecord.Split(',');

            ageData[i] = int.Parse(fields[0]);
            genderData[i] = fields[1];
            maritalData[i] = fields[2];
            districtData[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;
            }

            i++;

            inputRecord = inFile.ReadLine();            
        }
        Console.WriteLine("This Program takes census data from a file and lists residents by age group and district");
        Console.WriteLine("Please ensure the file is formatted correctly! age,gender (M/F),marital status   (M/S), and district");
        for (int x = 1; x <= 22; x++)
            for (int y = 0; y < districtData.Length; y++)
                if (districtData[y] == x)
                    countDist[x]++;

        for (int x = 1; x <= 22; x++)
            Console.WriteLine("District   " + x + "   has   " + countDist[x] + "   citizens");



        Console.WriteLine("------Amount of Residents per Age Group--------");
        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]);
    }

EDIT

Per comments below, the instructor thinks I should get rid of the ageGroup and districtData. This is what he was suggesting I do - however, I have no idea how I'm going to change my logic that was using districtData.

   for (int districtCount = 1; districtCount <= 22; districtCount++)
   {
       for (int districtNumber = 0; districtNumber < districtData.Length; districtNumber++)
       {
           if (districtData[districtNumber] == districtCount)
           {
             countDist[districtCount]++;
           }
       }
   }
\$\endgroup\$
4
\$\begingroup\$

Some ideas:

You if conditions can be simplified. The following to blocks are logically equivalent. This version is yours with only if statements:

if (ageData[i] > 0 && ageData[i] <= 18)
{
    ageGroup[0] = ageGroup[0] + 1;
}
if (ageData[i] > 18 && ageData[i] <= 30)
{
    ageGroup[1] = ageGroup[1] + 1;
}

This version uses if-else statements and is equivalent as long as ageData[i] is positive:

if (ageData[i] > 0 && ageData[i] <= 18)
{
    ageGroup[0] = ageGroup[0] + 1;
}
else if (ageData[i] <= 30) //I removed the "ageData[i] > 18 && "
{
    ageGroup[1] = ageGroup[1] + 1;
}

A more advanced technique would involve storing the age ranges in a data structure so that you can edit them more easily. You could use a dictionary that maps maximum ages to counts.


You should be disposing your stream objects: fstream and inFile. Better yet, read your whole file into a string with a single line like this:

string[] contents = System.IO.File.ReadAllLines(@"c:\path\to\file.txt");

This uses the verbatim string literal @. See here. Your loop will change as a result:

foreach(string line in contents)
{
    fields = line.Split(',');
}

If you haven't learned foreach loops yet, you can use a for loop like this:

for(int lineNumber = 0; lineNumber < contents.Length; lineNumber++
{
    fields = contents[lineNumber].Split(',');
    //etc.
}

You haven't said whether you've learned to use generic Lists or not. Your arrays hold 1000 items. If your input has more than 1000 items, you'll get an exception. If you have learned about Lists, you should use them instead because they automatically re-size themselves when they run out of room.

Instead of:

int[] ageData = new int[1000];
ageData[i] = int.Parse(fields[0]);

You do:

List<int> ageData = new List<int>();
ageData.Add(int.Parse(fields[0]));  
//you have to add an int to the list before you can use it

You have numbers sprinkled around everywhere. This is a problem for readability and maintenance. What happens if they add a district? I see two changes you'd have to make. What about if the age and gender columns are reversed? It's not so clear what changes you'd need to make to account for that.

Create constants like this:

const int COLUMN_AGE = 0;
const int ARRAY_SIZE = 1000;
//etc.

And then do this:

int[] ageData = new int[ARRAY_SIZE];
ageData[i] = int.Parse(fields[COLUMN_AGE]);
//etc.

Is there some reason you didn't use string formats for the following line?

Console.WriteLine("District   " + x + "   has   " + countDist[x] + "   citizens");

You can use this:

Console.WriteLine("District   {0}   has   {1}   citizens", x, countDist[x]);

These lines are a headache:

for (int x = 1; x <= 22; x++)
    for (int y = 0; y < districtData.Length; y++)
        if (districtData[y] == x)
            countDist[x]++;

You should get in the habit of using braces for all for loops and all but the simplest if statements. Also, the variable names x and y aren't very descriptive. Why not rename x to districtCount, and rename y to districtNumber?

for (int districtCount = 1; districtCount <= 22; districtCount++)
{
    for (int districtNumber = 0; districtNumber < districtData.Length; districtNumber++)
    {
        if (districtData[districtNumber] == districtCount)
        {
            countDist[districtCount]++;
        }
    }
}

Visual Studio makes it easy to rename variables. Right click on one and go Refactor > Rename. Or click F2. Don't be afraid of long variable names. Because of Intellisense, you really only need to type them out once.

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for your ideas. Your answer was very clear and easy for me to understand. I appreciate the help. \$\endgroup\$ – redcell98 Mar 30 '15 at 21:19
  • \$\begingroup\$ Glad to help. Don't forget to upvote answers that you like and if a submission answered your question, check it as "answered". \$\endgroup\$ – user2023861 Mar 30 '15 at 21:23
5
\$\begingroup\$

user2023861 covers a lot of good ground, so I just have a couple more things to add.

First, instead of using parallel arrays, you might want to consider using a collection container like a List and filling it with objects. Each line appears to a distinct record with a set of properties, so I would make them a simple Class:

public class Record
{
    public int Age { get; set; }
    public string Gender { get; set; }
    public string MaritalStatus { get; set; }
    public int District { get; set; }
}

That way, you don't have to hard code the size of the arrays that you are using, and you don't have to do any work to keep the related pieces of the record in synch (think what would happen if one of the arrays got sorted and the rest didn't).

Then, in your parser you can create objects for each record:

var records = new List<Record>();
var inputRecord = inFile.ReadLine();

while (inputRecord != null)
{
    var fields = inputRecord.Split(',');
    var currentRecord = new Record()
    {
        Age = int.Parse(fields[0]),
        Gender = fields[1],
        MaritalStatus = fields[2],
        District = int.Parse(fields[3])
    };

    records.Add(currentRecord);
    //...
}

You need to remember to dispose of your FileStream and StreamReader if you are accessing them this way. The easier and safer way to do this is with the using syntax, which will close and dispose of them at the ending braces. I'd get into the habit of using this with any object with a .Dispose() method.

using (var fStream = new FileStream("test.txt", FileMode.Open, FileAccess.Read))
{
    using (var inFile = new StreamReader(fStream))
    {
        //Work with the file here
        //...
    } //inFile.Dispose() happens here automatically.
} //fStream.Dispose() happens here automatically.

This code...

    for (int x = 1; x <= 22; x++)
        for (int y = 0; y < districtData.Length; y++)
            if (districtData[y] == x)
                countDist[x]++;

...is begging for braces to make it easier to read. It also help avoid potential errors.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your comment. I talked to my instructor today about the code. He alluded to lists when I asked, "How do I know how big to make an array?" He said we won't learn about it until we take the class after this one. One thing he said was that I didn't need to use an array for ageData and districtData. I know I'm supposed to put my logic into the loop to avoid using two arrays. I'm not sure how to change my code to accomplish that though.. \$\endgroup\$ – redcell98 Mar 31 '15 at 3:43
  • \$\begingroup\$ @redcell98 - You'd probably have to ask what he had in mind, because I doubt what springs to my mind is what he's after. I'm guessing if you haven't covered Lists yet you haven't covered Linq though. You can always do something like this using Linq: Console.WriteLine("Age Group 18 & under = {0}", ageData.Count(x => x < 19 && x > 0)); \$\endgroup\$ – Comintern Mar 31 '15 at 3:57
  • \$\begingroup\$ I updated my question with what I believe he is telling me to do. \$\endgroup\$ – redcell98 Mar 31 '15 at 4:09
  • \$\begingroup\$ I just rolled back the edit and added your new information below. If you edit your code on Code Review it invalidates answers. That said, your code is basically doing what the Linq code in the above comment is doing - filling the ageData and districtData arrays as you run through the parsing loop makes more sense than getting rid of them because you don't have to iterate over the data more than once to fill them. \$\endgroup\$ – Comintern Mar 31 '15 at 4:25
  • \$\begingroup\$ Unfortunately, we have not covered linq in class. So I would have no idea how to write the pseudocode, flowchart, test plan and desk check it. Keeping ageData and districtData would make my life so much easier but he said he would take off points because its not needed. I just don't know how to go about changing it. \$\endgroup\$ – redcell98 Mar 31 '15 at 4:31

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.