Solved

c# refactoring

Posted on 2013-06-30
22
383 Views
Last Modified: 2013-07-01
I have some code that I would like to refactor using lambda expressions...if possible..unless that would make the code too complex...also is there a better way to do this than just using if,else statements.


The conditions 1 thought 4 simply represent, not a triangle, and the other 3 numbers are different types of triangles..(scalene, isosceles, equilateral). ..pretty straight forward.


int GetCubicType(int Height, int Width, int Length)
    {
        if (Height < 0 || Width < 0 || Length < 0)
            return 4;
        else
        {
            if ((Height == Width) && (Height == Length))

                return 3;
            else
                if ((Height == Width) || (Width == Length) || (Length == Width))

                    return 2;
                else
                    return 1;
        }
    }

Open in new window

0
Comment
Question by:Robb Hill
  • 10
  • 10
  • 2
22 Comments
 
LVL 74

Expert Comment

by:käµfm³d 👽
ID: 39288649
What part are you trying to turn into a lambda expression, and what benefit are you seeking to receive from the conversion?
0
 
LVL 11

Author Comment

by:Robb Hill
ID: 39288654
Benefit for me is simply validate am I using the best approach under c# 4.0 to do something that has been a text book question since the days of the punchcard....either way...that is all.

I know with lambda is can read confusing..but is it better...in this case...would you do it different?   I have this method in a  class in vs 2013...so I can use whatever libraries available to do this from 4.5 framework and below.
0
 
LVL 74

Expert Comment

by:käµfm³d 👽
ID: 39288661
I think it would take some work to make the lambda confusing to read (for someone familiar with the syntax). But I'm still confused as to which part you are wanting to turn into a lambda. A lambda is basically an anonymous function. You can use it anywhere a function would be expected (provided the signatures match). Which part of this logic are you trying to lamda-fy? The whole thing? One or more of the if blocks? Can you provide a (pseudo code) example of what you think it might look like?
0
 
LVL 11

Author Comment

by:Robb Hill
ID: 39288665
I assumed the whole if else logic..but maybe I dont understand what I am talking about...I was thinking the code would be greatly reduced with lambda...and if/else type scenarios are a good place to do that....
0
 
LVL 11

Author Comment

by:Robb Hill
ID: 39288669
This is a simple function that takes in three integers...evaluates....checks if its a triangle....if all is good then returns what the type of triangle is.....


This code works...I just want to uberfy and lambdify...and anything else that may be a good example of c# at its best....
0
 
LVL 74

Expert Comment

by:käµfm³d 👽
ID: 39288681
Here's a scenario (using LINQ) where a lambda would decrease lines of code.

Given:

List<BusinessObject> theList = // initialized with various BusinessObject instances

Open in new window


Old-skool
BusinessObject foundObject = null;

foreach (BusinessObject item in theList)
{
    if (item.ID == 10)
    {
        foundObject = item;
        break;
    }
}

if (foundObject != null)
{
    // do something with foundObject
}

Open in new window


Lambda Approach
BusinessObject foundObject = theList.FirstOrDefault(item => item.ID == 10);

if (foundObject != null)
{
    // do something with foundObject
}

Open in new window


The method FirstOrDefault can take a function argument that contains the algorithm that determines which is the item we are interested in. Since I said earlier that lambda are effectively functions, our function in this case takes in a BusinessObject (via the item variable) and compares its ID property to the value of 10. If it matches, then the lambda will return a boolean value of true, telling FirstOrDefault that it has found a match, which it will subsequently return.

In your case, I'm just not sure how a lambda could be applied in such a was as to reduce code.
0
 
LVL 74

Expert Comment

by:käµfm³d 👽
ID: 39288684
Fun Fact:  The FirstOrDefault (along with the other LINQ methods) method actually uses a foreach under the hood. So in reality, you aren't really saving code overall; you're just saving code in the code that you yourself write. You can read my article to see what I mean:

http://www.experts-exchange.com/Programming/Languages/.NET/LINQ/A_10170-A-Beginner's-Guide-to-LINQ.html
0
 
LVL 74

Expert Comment

by:käµfm³d 👽
ID: 39288695
I suppose if you really wanted to, you could gain some readability by creating lambdas at the head of your function. Tehcnically speaking, you'd lose some performance since this is introducing another (series of) function call(s), but I doubt you'd notice it.

e.g.

int GetCubicType(int Height, int Width, int Length)
{
    Func<int, int, int, bool> dimensionsAreInvalid = (h, w, l) => (h < 0 || w < 0 || l < 0);
    Func<int, int, int, bool> isIsosceles = (h, w, l) => ((h == w) && (h == l));
    Func<int, int, int, bool> isScalene = (h, w, l) => ((h == w) || (w == l) || (l == w));
	
    if (dimensionsAreInvalid(Height, Width, Length)) return 4;
    if (isIsosceles(Height, Width, Length)) return 3;
    if (isScalene(Height, Width, Length)) return 2;
	
    return 1;
}

Open in new window


I'm not sure you're logic is correct for determining the different types of triangles, so the names might need to be adjusted in the above. (Geometry class was a long, long time ago!)
0
 
LVL 11

Author Comment

by:Robb Hill
ID: 39288754
yea I will right some automated testing or case tests ..something....but not worried about that yet....

But speaking on testing...via automated or what have you...do either of these approaches complicate that.


on the geometry side...if no sides are a number..it cannot be a triangle.
then you have three types of triangles...with one being easier..as all sides are equal.  Then its two sides with one comparing height vs width...and the other being width vs length.
0
 
LVL 51

Expert Comment

by:Julian Hansen
ID: 39289135
Personally I would just go with
int GetCubicType(int Height, int Width, int Length)
    {
        if (Height < 0 || Width < 0 || Length < 0) return 4;
        if ((Height == Width) && (Height == Length))  return 3;
        if ((Height == Width) || (Width == Length) || (Length == Width)) return 2;
        return 1;
    }

Open in new window

Lamda's don't help here - in fact in my opinion they will only complicate matters as in reading each of the lines you have to refer back to the lamda to figure out what it does.

Also not sure why you have (Width == Length) || (Length == Width) - seems like a duplication to me ...
0
 
LVL 11

Author Comment

by:Robb Hill
ID: 39289640
Here is some codes that should eliminate the confusion on the formulas.  Looking at this code...and without making two function calls....using the reducing the code though for efficiancy and readability...as it seems most of these examples are on the money.  This will be written in a class accessable as a public web method.  

using System;
using System.Collections.Generic;
class Program
{
    public enum CubicType
    {
        Scalene = 1,
       
        Isosceles = 2,
        Equilateral = 3,
        Error = 4,
    };
     
    static void Main()
    {
        CubicType i;
             
              i = GetCubicType(0,2,5); // Returns Scalene
              i = GetCubicType(0, 2, -1); // Returns Error
              i = GetCubicType(0, 0, 0); // Returns Equilateral
              i = GetCubicType(1, 1, 1); // Returns Equilateral
              i = GetCubicType(5, 1, 5); // Returns Scalene
           
   
}

    private static CubicType GetCubicType(int Height, int Width, int Length)
    {
        if (Height < 0 || Width < 0 || Length < 0)
        {
            Console.Write(CubicType.Error);
            return CubicType.Error;

        }
        else
        if ((Height == Width)&&  (Height == Length))
        {

            Console.Write(CubicType.Equilateral);
            return CubicType.Equilateral;


        }
        else
        if ((Height == Width) || (Width == Length) || (Length == Width))
        {
            Console.Write(CubicType.Isosceles);
            return CubicType.Isosceles;



        }
        else
        {
           Console.Write(CubicType.Scalene);
           return CubicType.Scalene;

        }

    }
}

Open in new window

0
Better Security Awareness With Threat Intelligence

See how one of the leading financial services organizations uses Recorded Future as part of a holistic threat intelligence program to promote security awareness and proactively and efficiently identify threats.

 
LVL 74

Expert Comment

by:käµfm³d 👽
ID: 39289646
@julianH

in fact in my opinion they will only complicate matters as in reading each of the lines you have to refer back to the lamda to figure out what it does.
Could that not be said of any function?
0
 
LVL 11

Author Comment

by:Robb Hill
ID: 39289651
on my unit test the only thing im finding wrong with the geometry is that the 0,0,0 scenario is not a triangle..its nothing...lol.   every side has to have a value to be a triangle...meaning any thing greater than 0.   That must be turned around on accident.
0
 
LVL 74

Expert Comment

by:käµfm³d 👽
ID: 39289656
@robbhill

OK. So what's wrong with that code? It really isn't' that many lines of code, and it is clear to someone reading it (the most important thing, IMO) what each part of the function does/is related to. You shouldn't stress over the lines of code; worry about whether or not your code is readable. I generally try to keep my functions sized so that they fit on one screeen of the editor. If I have to scroll in order to see the rest of a function, then I consider refactoring my function.
0
 
LVL 11

Author Comment

by:Robb Hill
ID: 39289688
What about this..or still converting this now to lambda and or linq as discussed above.

Also since I am just compiling this and want to see some output withing the screen as you would do in unit testing......how would you put this on the screen....do not want to do console but within the build.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace CalculatorProgram
{
    public class Triangle
    {

        public enum CubicType
        {
            Scalene = 1,
            Isosceles = 2,
            Equilateral = 3,
            Error = 4,
        };

        private static CubicType GetCubicType(int Height, int Width, int Length)
        {
            if (Height <= 0 || Width <= 0 || Length <= 0)
            {
               
                return CubicType.Error;

            }
            else
                if ((Height == Width) && (Height == Length))
                {

                   
                    return CubicType.Equilateral;


                }
                else
                    if ((Height == Width) || (Width == Length) || (Length == Width))
                    {
                        
                        return CubicType.Isosceles;



                    }
                    else
                    {
                       
                        return CubicType.Scalene;

                    }

        }

    }
}

Open in new window

0
 
LVL 74

Accepted Solution

by:
käµfm³d   👽 earned 500 total points
ID: 39289708
I'd like to suggest that you start working with the UnitTest projects that you can build within Visual Studio:

Screenshot
It's an extra project, and it's extra code you have to write, but it pays off in terms of knowing what you are testing, and you get regression testing in that--so long as you don't remove or modify your tests once they are working--you can repeatedly run your tests and immediately know whether not something is broken.

For instance, you have the following tests demonstrated:

CubicType i;
             
i = GetCubicType(0,2,5); // Returns Scalene
i = GetCubicType(0, 2, -1); // Returns Error
i = GetCubicType(0, 0, 0); // Returns Equilateral
i = GetCubicType(1, 1, 1); // Returns Equilateral
i = GetCubicType(5, 1, 5); // Returns Scalene

Open in new window


In reading your comments, I get a sense of what is going on. However, if something is wrong internally with your function, you won't know which tests are passing and which are failing. You will only know the first one that fails. With a unit test project, all of your tests will be run, and you'll know which ones fail and which succeed. Having a report that shows multiple failing tests could help you track down the internal function error quicker.

For example, let's say you've now put your code into a class:

using System;

namespace _28171974
{
    class Program
    {
        static void Main()
        {
            Geometry.CubicType i;

            i = Geometry.GetCubicType(0, 2, 5); // Returns Scalene
            i = Geometry.GetCubicType(0, 2, -1); // Returns Error
            i = Geometry.GetCubicType(0, 0, 0); // Returns Equilateral
            i = Geometry.GetCubicType(1, 1, 1); // Returns Equilateral
            i = Geometry.GetCubicType(5, 1, 5); // Returns Scalene
        }
    }

    public class Geometry
    {
        public enum CubicType
        {
            Scalene = 1,

            Isosceles = 2,
            Equilateral = 3,
            Error = 4,
        };

        public static CubicType GetCubicType(int Height, int Width, int Length)
        {
            if (Height < 0 || Width < 0 || Length < 0)
            {
                Console.Write(CubicType.Error);
                return CubicType.Error;
            }
            else if ((Height == Width) && (Height == Length))
            {
                Console.Write(CubicType.Equilateral);
                return CubicType.Equilateral;
            }
            else if ((Height == Width) || (Width == Length) || (Length == Width))
            {
                Console.Write(CubicType.Isosceles);
                return CubicType.Isosceles;
            }
            else
            {
                Console.Write(CubicType.Scalene);
                return CubicType.Scalene;
            }
        }
    }
}

Open in new window


Now you can add a UnitTest project to your solution. Here's what tests for the Error condition might look like:

using _28171974;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace UnitTestProject1
{
    [TestClass]
    public class GeometryTests
    {
        [TestMethod]
        public void When_Calling_GetCubicType__If_Invalid_Height_Is_Passed__Then_CubicType_Error_Should_Be_Returned()
        {
            // Arrange
            Geometry.CubicType expected = Geometry.CubicType.Error;
            Geometry.CubicType actual;
            int h = -1;
            int w = 1;
            int l = 1;

            // Act
            actual = Geometry.GetCubicType(h, w, l);

            // Assert
            Assert.AreEqual(expected, actual);
        }

        [TestMethod]
        public void When_Calling_GetCubicType__If_Invalid_Width_Is_Passed__Then_CubicType_Error_Should_Be_Returned()
        {
            // Arrange
            Geometry.CubicType expected = Geometry.CubicType.Error;
            Geometry.CubicType actual;
            int h = 1;
            int w = -1;
            int l = 1;

            // Act
            actual = Geometry.GetCubicType(h, w, l);

            // Assert
            Assert.AreEqual(expected, actual);
        }

        [TestMethod]
        public void When_Calling_GetCubicType__If_Invalid_Length_Is_Passed__Then_CubicType_Error_Should_Be_Returned()
        {
            // Arrange
            Geometry.CubicType expected = Geometry.CubicType.Error;
            Geometry.CubicType actual;
            int h = 1;
            int w = 1;
            int l = -1;

            // Act
            actual = Geometry.GetCubicType(h, w, l);

            // Assert
            Assert.AreEqual(expected, actual);
        }
    }
}

Open in new window


And if I run my tests:

Screenshot
Now let's say the function's internals are screwed up:

public static CubicType GetCubicType(int Height, int Width, int Length)
{
    if (Height < 0 || Width < 0 || Length > 0)  // Length greater than zero is an error?
...

Open in new window


If I run the tests under this condition:

Screenshot
Now I know that something is wrong with the length checking of the function.

If you're not already, when you move into an environment that uses build servers, you can set up your tests to automatically run whenever code is checked in--something that won't happen when you unit test in the manner that you are currently doing. This can be very beneficial when working in large teams with many people working on different parts of the application. Knowing that code is going to fail before actually checking it in to source control saves you from checking in bad code.
0
 
LVL 11

Author Comment

by:Robb Hill
ID: 39289747
Yes I like that approach!!!   so here is my code...so far....im going to strip out the console piece of this and try to implement the unit test..so I can see the checks at runtime.


using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Triangle
{
    public class Program
   {
         public enum TriangleType
         {
             Scalene = 1,
             Isosceles = 2,
             Equilateral = 3,
             Error = 4,
         }
         private static TriangleType triangle(int a, int b, int c)
         {
             TriangleType answer;
  
             if (a <= 0 || b <= 0 || c <= 0)
             {
                 // negative length -- illegal
                 answer =  TriangleType.Error;
             }
             else if ((a + b <= c) || (b + c <= a) || (a + c <= b))
             {
                 // Sides can't possible connect -- illegal
                 answer = TriangleType.Error;
             }
             else if ((a == b) && (b == c))
             {
                 // by the transitive rule, a == c well
                 answer = TriangleType.Equilateral;
             }
             else if ((a == b) || (b == c) || (a == c))
             {
                 // We've already eliminated quilateral triangles above,
                 // we'll only get here if exactly two sides match
                 answer = TriangleType.Isosceles;
             }
             else
             {
                 // Since, we've eliminated all alternatives, we must have
                 // a scalene triangle if we get here.
                 answer = TriangleType.Scalene;
             }
  
             return answer;
         }
  
         static void Main(string[] args)
         {
             TriangleType result;
             result = triangle(0, 0, 0);
             Console.WriteLine("(0, 0, 0) = {0}", result.ToString());
  
             result = triangle(-1, 2, 3);
             Console.WriteLine("(-1, 2, 3) = {0}", result.ToString());
  
             result = triangle(1, -2, 3);
             Console.WriteLine("(1, -2, 3) = {0}", result.ToString());
  
             result = triangle(1, 2, -3);
             Console.WriteLine("(1, 2, -3) = {0}", result.ToString());
  
             result = triangle(1, 1, 1);
             Console.WriteLine("(1, 1, 1) = {0}", result.ToString());
  
             result = triangle(1, 2, 2);
             Console.WriteLine("(1, 2, 2) = {0}", result.ToString());
  
             result = triangle(4, 5, 6);
             Console.WriteLine("(4, 5, 6) = {0}", result.ToString());
  
             result = triangle(5, 1, 5);
             Console.WriteLine("(5, 1, 5) = {0}", result.ToString());
  
             result = triangle(5, 5, 5);
             Console.WriteLine("(5, 5, 5) = {0}", result.ToString());
  
             result = triangle(-10, -10, -10);
             Console.WriteLine("(-10, -10, -10) = {0}", result.ToString());
         }
     }
 }

Open in new window

0
 
LVL 74

Expert Comment

by:käµfm³d 👽
ID: 39289780
so I can see the checks at runtime.
To note:  There may be a way to have Visual Studio run the tests when you build/run your project, but I'm not sure how to configure that. I usually just manually run the tests from the Test menu.

Screenshot
0
 
LVL 51

Expert Comment

by:Julian Hansen
ID: 39289793
@kaufmed
Could that not be said of any function?
Of course but when your function simply replaces an if (a==b || b==c) - you are not really simplifying are you?
0
 
LVL 11

Author Comment

by:Robb Hill
ID: 39289808
my namespaces are messing up in the unit test because of Geometry....what am I missing here....

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace TriangleProject
{
    public class Geometry
   {
         public enum CubicType
         {
             Scalene = 1,
             Isosceles = 2,
             Equilateral = 3,
             Error = 4,
         }
         private static CubicType triangle(int a, int b, int c)
         {
             CubicType answer;
  
             if (a <= 0 || b <= 0 || c <= 0)
             {
                 // negative length -- illegal
                 answer =  CubicType.Error;
             }
             else if ((a + b <= c) || (b + c <= a) || (a + c <= b))
             {
                 // Sides can't possible connect -- illegal
                 answer = CubicType.Error;
             }
             else if ((a == b) && (b == c))
             {
                 // by the transitive rule, a == c well
                 answer = CubicType.Equilateral;
             }
             else if ((a == b) || (b == c) || (a == c))
             {
                 // We've already eliminated quilateral triangles above,
                 // we'll only get here if exactly two sides match
                 answer = CubicType.Isosceles;
             }
             else
             {
                 // Since, we've eliminated all alternatives, we must have
                 // a scalene triangle if we get here.
                 answer = CubicType.Scalene;
             }
  
             return answer;
         }
  
         //   static void Main(string[] args)
         //{
         //    CubicType result;
         //    result = triangle(0, 0, 0);
         //    Console.WriteLine("(0, 0, 0) = {0}", result.ToString());
  
         //    result = triangle(-1, 2, 3);
         //    Console.WriteLine("(-1, 2, 3) = {0}", result.ToString());
  
         //    result = triangle(1, -2, 3);
         //    Console.WriteLine("(1, -2, 3) = {0}", result.ToString());
  
         //    result = triangle(1, 2, -3);
         //    Console.WriteLine("(1, 2, -3) = {0}", result.ToString());
  
         //    result = triangle(1, 1, 1);
         //    Console.WriteLine("(1, 1, 1) = {0}", result.ToString());
  
         //    result = triangle(1, 2, 2);
         //    Console.WriteLine("(1, 2, 2) = {0}", result.ToString());
  
         //    result = triangle(4, 5, 6);
         //    Console.WriteLine("(4, 5, 6) = {0}", result.ToString());
  
         //    result = triangle(5, 1, 5);
         //    Console.WriteLine("(5, 1, 5) = {0}", result.ToString());
  
         //    result = triangle(5, 5, 5);
         //    Console.WriteLine("(5, 5, 5) = {0}", result.ToString());
  
         //    result = triangle(-10, -10, -10);
         //    Console.WriteLine("(-10, -10, -10) = {0}", result.ToString());
         //}
     }
 }

Open in new window




using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;


namespace UnitTestTriangle
{
    [TestClass]
    public class GeometryTests
    {
        [TestMethod]
        public void When_Calling_GetCubicType__If_Invalid_Height_Is_Passed__Then_CubicType_Error_Should_Be_Returned()
        {
            // Arrange
            Geometry.CubicType expected = Geometry.CubicType.Error;
            Geometry.CubicType actual;
            int h = -1;
            int w = 1;
            int l = 1;

            // Act
            actual = Geometry.GetCubicType(h, w, l);

            // Assert
            Assert.AreEqual(expected, actual);
        }

        [TestMethod]
        public void When_Calling_GetCubicType__If_Invalid_Width_Is_Passed__Then_CubicType_Error_Should_Be_Returned()
        {
            // Arrange
            Geometry.CubicType expected = Geometry.CubicType.Error;
            Geometry.CubicType actual;
            int h = 1;
            int w = -1;
            int l = 1;

            // Act
            actual = Geometry.GetCubicType(h, w, l);

            // Assert
            Assert.AreEqual(expected, actual);
        }

        [TestMethod]
        public void When_Calling_GetCubicType__If_Invalid_Length_Is_Passed__Then_CubicType_Error_Should_Be_Returned()
        {
            // Arrange
            Geometry.CubicType expected = Geometry.CubicType.Error;
            Geometry.CubicType actual;
            int h = 1;
            int w = 1;
            int l = -1;

            // Act
            actual = Geometry.GetCubicType(h, w, l);

            // Assert
            Assert.AreEqual(expected, actual);
        }
    }
}

Open in new window

0
 
LVL 74

Expert Comment

by:käµfm³d 👽
ID: 39289829
Within your UnitTest project you need to add a reference to the project being tested. (Sorry, I forgot to mention that earlier.)
0
 
LVL 11

Author Comment

by:Robb Hill
ID: 39290358
Thanks very much....working now.....so I guess the conclusion here is to not do the lambda...and just make this more readable with the if else as is...and ofcourse the added unit testing..which was very helpful!
0

Featured Post

Free Trending Threat Insights Every Day

Enhance your security with threat intelligence from the web. Get trending threat insights on hackers, exploits, and suspicious IP addresses delivered to your inbox with our free Cyber Daily.

Join & Write a Comment

This document covers how to connect to SQL Server and browse its contents.  It is meant for those new to Visual Studio and/or working with Microsoft SQL Server.  It is not a guide to building SQL Server database connections in your code.  This is mo…
Many of us here at EE write code. Many of us write exceptional code; just as many of us write exception-prone code. As we all should know, exceptions are a mechanism for handling errors which are typically out of our control. From database errors, t…
Here's a very brief overview of the methods PRTG Network Monitor (https://www.paessler.com/prtg) offers for monitoring bandwidth, to help you decide which methods you´d like to investigate in more detail.  The methods are covered in more detail in o…
This video explains how to create simple products associated to Magento configurable product and offers fast way of their generation with Store Manager for Magento tool.

705 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

20 Experts available now in Live!

Get 1:1 Help Now