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<string, int> _myfiles = new Dictionary<string, int>();
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 < Files.Length; i++)
{
var read = new StreamReader(Files[i]);
var counter = 0;
while (read.ReadLine() != null)
{
counter++;
}
_myfiles[$"Sec{i+1}"] = counter;
read.Close();
}
}
private void Insert_Into_JA()
{
for (var x = 0; x < Files.Length; x++)
{
var read = new StreamReader(Files[x]);
string tline;
var tempArray = new int[_myfiles[$"Sec{x + 1}"]];
var i = 0;
while ((tline = read.ReadLine()) != null)
{
var line = int.Parse(tline);
if (_minV == 0 || line < _minV)
{
_minV = line;
_minS = $"Sec {(x + 1)}";
}
else if ((line == _minV) && (_minS != $"Sec {(x + 1)}"))
{
_minS = $"{_minS} & Sec {(x + 1)}";
}
if (_maxV == 0 || line > _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;
}
}
JaggedArray[x] = tempArray;
read.Close();
}
}
private void Display_Results()
{
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;
}
}
TAvg.Text = ((double.Parse(Sec1Avg.Text) + double.Parse(Sec2Avg.Text) + double.Parse(Sec3Avg.Text)) / 3).ToString("n2");
THScore.Text = $"{_maxV}, {_maxS}";
TLScore.Text = $"{_minV}, {_minS}";
}
Solution
I prefer to use explicit access modifiers — it keeps everything uniform: you have 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.