patterncsharpModerate
Calculating and displaying score statistics using C# 6.0 features
Viewed 0 times
scoredisplayingcalculatingstatisticsusingandfeatures
Problem
I'm studying C# and trying to work on efficiency and making the best use of C# 6.0 features. I have created the following program based on an exercise challenge in a book, and have used Resharper to fix certain things; however, I still feel my novice knowledge of C# is preventing me from creating the below program in a more efficient manner, and I'm wondering if there are any glaring changes I could make to produce a cleaner set of code that Resharper isn't pointing out.
The objective was intentionally to use Jagged Arrays, and I do know I could've just used lists and made things easier; so, please don't recommend replacing the array/jagged arrays with a list.
```
static readonly string[] Files = { @"C:\Section1.txt",
@"C:Section2.txt",
@"C:\Section3.txt"};
readonly Dictionary _myfiles = new Dictionary();
internal int[][] JaggedArray = new int[Files.Length][];
int _minV;
int _maxV;
string _minS = "";
string _maxS = "";
private void button1_Click(object sender, EventArgs e)
{
Get_File_Counts();
Insert_Into_JA();
Display_Results();
}
private void Get_File_Counts()
{
for (var i = 0; i _maxV)
{
_maxV = line;
_maxS = $"Sec {(x + 1)}";
}
else if ((line == _maxV) && (_maxS != $"Sec {(x + 1)}"))
{
_maxS = $"{_maxS} & Sec {(x + 1)}";
}
tempArray[i] = line;
i++;
switch (x)
{
case 0:
Scores1.Items.Add(line);
break;
case 1:
Scores2.Items.Add(line);
break;
case 2:
Scores3.Items.Add(line);
break;
}
The objective was intentionally to use Jagged Arrays, and I do know I could've just used lists and made things easier; so, please don't recommend replacing the array/jagged arrays with a list.
```
static readonly string[] Files = { @"C:\Section1.txt",
@"C:Section2.txt",
@"C:\Section3.txt"};
readonly Dictionary _myfiles = new Dictionary();
internal int[][] JaggedArray = new int[Files.Length][];
int _minV;
int _maxV;
string _minS = "";
string _maxS = "";
private void button1_Click(object sender, EventArgs e)
{
Get_File_Counts();
Insert_Into_JA();
Display_Results();
}
private void Get_File_Counts()
{
for (var i = 0; i _maxV)
{
_maxV = line;
_maxS = $"Sec {(x + 1)}";
}
else if ((line == _maxV) && (_maxS != $"Sec {(x + 1)}"))
{
_maxS = $"{_maxS} & Sec {(x + 1)}";
}
tempArray[i] = line;
i++;
switch (x)
{
case 0:
Scores1.Items.Add(line);
break;
case 1:
Scores2.Items.Add(line);
break;
case 2:
Scores3.Items.Add(line);
break;
}
Solution
I prefer to use explicit access modifiers -- it keeps everything uniform: you have
Every field that is outside-world accessible, should be a property. This means
This is commonly known as encapsulation.
Some people (like me) prefer to use
Conventions dictate that no underscores are used: methods are
Instead of explicitly closing your
Instead of
you could do
Why are you storing the keys as
Consider also using more descriptive names:
This can throw an exception which you should consider either catching or using
If the input is expected to contain invalid data and you're throwing on purpose, then it might be useful to document in what scenarios it could have that invalid input.
This is an ugly usage of a
I would consider slightly rewriting it with a method to help you:
At this point you could decide to continue rewriting this but that's up to you.
First you calculate everything, then you round it, then you parse and then you round it again. This is bound to lose accuracy.
internal int[][] JaggedArray so I'd make the others like _minV explicitly private.Every field that is outside-world accessible, should be a property. This means
internal, protected internal and public fields. The benefit of this is that you can always decide later to add validation logic without breaking your contract.This is commonly known as encapsulation.
Some people (like me) prefer to use
string.Empty instead of "" because the intentions are perceived unambiguously whereas "" always nags at me saying it might be a typo.Conventions dictate that no underscores are used: methods are
UpperCamelCase. The event handler might be an exception due to its widespread default usage although I would instead opt for a OnButtonClicked pattern.Instead of explicitly closing your
StreamReader, consider wrapping it in a using statement instead.Instead of
var read = new StreamReader(Files[i]);
var counter = 0;
while (read.ReadLine() != null)
{
counter++;
}
_myfiles[$"Sec{i + 1}"] = counter;
read.Close();you could do
_myfiles[$"Sec{i + 1}"] = File.ReadLines(Files[i]).Count;Why are you storing the keys as
Sec{i}? I think it would be easier if you simply addressed them as i -- that way you don't have these curious string interpolations throughout your code.Consider also using more descriptive names:
tLine, tempArray, JaggedArray, _minV, etc. don't say much about their purpose. var line = int.Parse(tline);This can throw an exception which you should consider either catching or using
int.TryParse().If the input is expected to contain invalid data and you're throwing on purpose, then it might be useful to document in what scenarios it could have that invalid input.
for (var x = 0; x < Files.Length; x++)
{
double total = 0;
var tscores = _myfiles[$"Sec{x + 1}"];
for (var i = 0; i < tscores; i++)
{
total += JaggedArray[x][i];
}
switch (x)
{
case 0:
Sec1Avg.Text = (total / tscores).ToString("n2");
break;
case 1:
Sec2Avg.Text = (total / tscores).ToString("n2");
break;
case 2:
Sec3Avg.Text = (total / tscores).ToString("n2");
break;
}
}This is an ugly usage of a
switch and prone to errors (you can easily forget another case when you add a 4th file).I would consider slightly rewriting it with a method to help you:
private double GetTotal(int fileIndex, int scoreAmount)
{
var total = 0.0;
for (var i = 0; i < scoreAmount; i++)
{
total += JaggedArray[x][i];
}
return total;
}
var sec1Scores = _myFiles[x + 1];
Sec1Avg.Text = (GetTotal(0, sec1Score) / sec1Scores).ToString("n2");
var sec2Scores = _myFiles[x + 1];
Sec2Avg.Text = (GetTotal(1, sec1Score) / sec2Scores).ToString("n2");
var sec3Scores = _myFiles[x + 1];
Sec3Avg.Text = (GetTotal(2, sec1Score) / sec3Scores).ToString("n2");At this point you could decide to continue rewriting this but that's up to you.
TAvg.Text = ((double.Parse(Sec1Avg.Text) + double.Parse(Sec2Avg.Text) + double.Parse(Sec3Avg.Text)) / 3).ToString("n2");First you calculate everything, then you round it, then you parse and then you round it again. This is bound to lose accuracy.
Code Snippets
var read = new StreamReader(Files[i]);
var counter = 0;
while (read.ReadLine() != null)
{
counter++;
}
_myfiles[$"Sec{i + 1}"] = counter;
read.Close();_myfiles[$"Sec{i + 1}"] = File.ReadLines(Files[i]).Count;var line = int.Parse(tline);for (var x = 0; x < Files.Length; x++)
{
double total = 0;
var tscores = _myfiles[$"Sec{x + 1}"];
for (var i = 0; i < tscores; i++)
{
total += JaggedArray[x][i];
}
switch (x)
{
case 0:
Sec1Avg.Text = (total / tscores).ToString("n2");
break;
case 1:
Sec2Avg.Text = (total / tscores).ToString("n2");
break;
case 2:
Sec3Avg.Text = (total / tscores).ToString("n2");
break;
}
}private double GetTotal(int fileIndex, int scoreAmount)
{
var total = 0.0;
for (var i = 0; i < scoreAmount; i++)
{
total += JaggedArray[x][i];
}
return total;
}
var sec1Scores = _myFiles[x + 1];
Sec1Avg.Text = (GetTotal(0, sec1Score) / sec1Scores).ToString("n2");
var sec2Scores = _myFiles[x + 1];
Sec2Avg.Text = (GetTotal(1, sec1Score) / sec2Scores).ToString("n2");
var sec3Scores = _myFiles[x + 1];
Sec3Avg.Text = (GetTotal(2, sec1Score) / sec3Scores).ToString("n2");Context
StackExchange Code Review Q#102220, answer score: 12
Revisions (0)
No revisions yet.