Code Review Asked by proppaganda on December 6, 2021
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]);
}
}
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.
Answered by user33306 on December 6, 2021
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;
}
}
Answered by James Khoury on December 6, 2021
Try to visualize how your items are related. Think how your code runs, step by step, like in slow-mo. You will realize you can sum up long and tedious code into simplest, more abstract, styles.
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 as you have here, there is almost always a better way to code that. Here, a simple loop makes sense:
for (int i = 1; i <= 22; i++)
{
Console.WriteLine("District {0} = {1}", i, districtCount[i-1]);
}
That makes it short and readable, less convoluted. We merely extracted the 1,2,3... and the 0,1,2... into i and i-1 respectively.
Now using the same "trick" you can replace all your ifs (which should have been if else's in the first place) with just a single line:
districtCount[districtDataA[i] - 1]++;
BONUS: When you want to type variable = variable + 1 it's shorter and more readable if you type variable++ instead. Same with variable--. For different amounts, you can do variable += amount (also -=, *=, /= and more operators depending on the language)
Answered by Neithan Max on December 6, 2021
Get help from others!
Recent Answers
Recent Questions
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP