Problem
This code reads barrel diameter and height from file, then, based on dimension make decisions and finally these decisions are printed out to a file. My pronbelm is how to avoid using the code wrtine on lines 70-101? Probably has to do with line 35, but I haven´t figured my way around it. Would be nice if there is a way (chance) to make this code shorter.
P.S d – diameter, h – height
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
namespace TaaviSimsonTest
{
class Program
{
static void Main(string[] args)
{
//Reads values from an input file
var numbersList = File.ReadAllLines("C:\temp\vaaditest07.txt")
.Select(line => line.Split(' '))
.Select(barrels => new
{
diameter = int.Parse(barrels[0]),
height = int.Parse(barrels[1])
})
.ToArray();
//Puts diameter and height values to arrays
int[] d = numbersList.Select(x => x.diameter).ToArray();
int[] h = numbersList.Select(x => x.height).ToArray();
//Displays numbersList with line number, diameter and height
for (int i = 0; i < d.Length; i++)
{
Console.WriteLine("Line {0}: diameter: {1}, height: {2}",
i, d[i], h[i]);
}
//comparing barrel sizes and making a decision
List<string> output = new List<string>();
for (int j = 0; j < (d.Length - 1); j++)
{
int a = j + 1;
int b = j + 2;
if (d[j] > d[j + 1] && h[j] > h[j+1])
{
string command = a + "<-" + b; //1 <- 2, if j = 0
output.Add(command); //puts command in array
}
else if (d[j] < d[j + 1] && h[j] < h[j+1])
{
string command = a + "->" + b;
output.Add(command);
}
else if (d[j+1] < h[j] &&
((d[j+1] * d[j+1] + h[j+1] * h[j+1]) <
d[j] * d[j]))
{
string command = a + "<-" + b; // 2 <- 3
output.Add(command);
}
else if(d[j] < h[j+1] &&
((d[j] * d[j] + h[j] * h[j]) <
d[j+1] * d[j+1]))
{
string command = a + "->" + b;
output.Add(command);
}
else
{
string command = a + "--" + b;
output.Add(command);
}
}
//how to avoid this cycle?
int c = 1;
int e = d.Length;
if (d[0] > d[d.Length - 1] && h[0] > h[d.Length - 1])
{
string command = c + "<-" + e;
output.Add(command);
}
else if (d[0] < d[d.Length - 1] && h[0] < h[d.Length-1])
{
string command = c + "->" + e;
output.Add(command);
}
else if (d[d.Length-1] < h[0] &&
((d[d.Length-1] * d[d.Length-1] + h[d.Length-1] * h[d.Length-1]) <
d[0] * d[0]))
{
string command = c + "<-" + e;
output.Add(command);
}
else if (d[0] < h[d.Length-1] &&
((d[0] * d[0] + h[0] * h[0]) <
d[d.Length-1]*d[d.Length-1]))
{
string command = c + "->" + e;
output.Add(command);
}
else
{
string command = c + "--" + e;
output.Add(command);
}
//Writes values of array to a new file
String[] outputlist = output.ToArray();
File.WriteAllLines(@"C:TempbarrelsOutput.txt", outputlist);
//Displaying output in console
Console.WriteLine("");
foreach (var item in outputlist)
{
Console.WriteLine(item.ToString());
}
}
}
}
Solution
Some Notes :
- you should make your code more readable more often. This would give your code more sense even for yourself, specially if you reviewed it in the future.
- you need to consider naming convention like using Pascal Casing for Properties, naming your variables with meaningful names.
- Don’t split the string without validating first.
- Don’t
Parsevalues directly such asint.Parseunless you’re sure it will be a valid value, instead useint.TryParseto avoid exceptions. - Don’t create an array from anonymous type (or any other type) array unless if is needed (in your case it’s not needed) instead, use the array that you’ve created.
- Always if you see some redundancy in your code (repetitive code) move it to a method, this way you can reuse the method and your adjustments would be in one place.
File.WriteAllLinesacceptsIEnumerablethis means, you can pass theList<string>directly, so no need to convert it to array.string.Joincan concatenate aListorArrayinto onestringso use that to your advantage.- Always validate your values as much as possible, this would give more stability to your code, and ensure your results.
Your question regarding this part of code :
if (d[0] > d[d.Length - 1] && h[0] > h[d.Length - 1])
{
string command = c + "<-" + e;
output.Add(command);
}
else if (d[0] < d[d.Length - 1] && h[0] < h[d.Length-1])
{
string command = c + "->" + e;
output.Add(command);
}
else if (d[d.Length-1] < h[0] &&
((d[d.Length-1] * d[d.Length-1] + h[d.Length-1] * h[d.Length-1]) <
d[0] * d[0]))
{
string command = c + "<-" + e;
output.Add(command);
}
else if (d[0] < h[d.Length-1] &&
((d[0] * d[0] + h[0] * h[0]) <
d[d.Length-1]*d[d.Length-1]))
{
string command = c + "->" + e;
output.Add(command);
}
else
{
string command = c + "--" + e;
output.Add(command);
}
This part needs to be moved into a method and make it more generalized to be reused, like this :
private static string GetCommand(int currentDiameter , int nextDiameter , int currentHeight , int nextHeight)
{
var currentDiameterMultiplied = currentDiameter * currentDiameter;
var nextDiameterMultiplied = nextDiameter * nextDiameter;
var currentHeightMultiplied = currentHeight * currentHeight;
var nextHeightMultiplied = nextHeight * nextHeight;
if(currentDiameter > nextDiameter && currentHeight > nextHeight)
{
return "<-";
}
if(currentDiameter < nextDiameter && currentHeight < nextHeight)
{
return "->";
}
if(nextDiameter < currentHeight && ( ( nextDiameterMultiplied + nextHeightMultiplied ) < currentDiameterMultiplied ))
{
return "<-";
}
if(currentDiameter < nextHeight && ( ( currentDiameterMultiplied + currentHeightMultiplied ) < nextDiameterMultiplied ))
{
return "->";
}
return "--";
}
now you can reuse it in your code, here is the modified version of your code:
static void Main(string[] args)
{
//Reads values from an input file
var path = "C:\temp\vaaditest07.txt";
if(!File.Exists(path)) { throw new FileNotFoundException(nameof(path)); }
var data = File.ReadAllLines(path);
if(data == null) { throw new ArgumentNullException(nameof(data)); }
var numbersList = data
.Select(line =>
line.Split(' ')
.Select(barrels => new
{
Diameter = int.TryParse(barrels[0], out var diameter) ? diameter : 0,
Height = int.TryParse(barrels[1], out var height) ? height : 0
}).ToArray();
//comparing barrel sizes and making a decision
var output = new List<string>();
var length = numbersList.Length;
var command = string.Empty;
for(int index = 0, nextIndex = 1; index < length; index++, nextIndex++)
{
var current = numbersList[index];
var next = numbersList[nextIndex];
// Displays numbersList with line number, diameter and height
Console.WriteLine("Line {0}: diameter: {1}, height: {2}" , index , current.Diameter , current.Height);
if(index != length - 1)
{
var commandSymbol = GetCommand(current.Diameter , next.Diameter , current.Height , next.Height);
command = $"{nextIndex}{commandSymbol}{nextIndex + 1}";
}
else
{
var commandSymbol = GetCommand(numbersList[0].Diameter , current.Diameter , numbersList[0].Height , current.Height);
command = $"{0}{commandSymbol}{length}";
}
output.Add(command);
}
// Writes values of array to a new file
File.WriteAllLines(@"C:TempbarrelsOutput.txt" , output);
//Displaying output in console
Console.WriteLine();
Console.WriteLine(string.Join(Environment.NewLine, output));
}
I would advice starting with writing code that is actually readable. Also from your question I don’t really know what your goal is.
Do you want short code that get the job done? Then I’d say you are already there. If you want readable and maintainable code, then maybe try an OO approach where you encapsulate bits of code into objects which with their name and methods you document what you are actually doing.
Because I’m waaaay too lazy to interpret what you are trying to do at lines 70-101. I wanna read the names and titles and know it.
So, that is my two cents