Solving code without extra cycle outside for loop

Posted on

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 Parse values directly such as int.Parse unless you’re sure it will be a valid value, instead use int.TryParse to 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.WriteAllLines accepts IEnumerable this means, you can pass the List<string> directly, so no need to convert it to array.
  • string.Join can concatenate a List or Array into one string so 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

Leave a Reply

Your email address will not be published. Required fields are marked *