CodeBetter.Com
CodeBetter.Com
RSS 2.0 via Feedburner
           Do you Twitter? Follow us @CodeBetter

Jeffrey Palermo [MVP]

Software management consultant and CTO, Headspring Systems

A symptom of bad code - level 100

Take the following code:

using System;

using System.Collections.Generic;
using System.Text;

 

public class ClientCode

{

    public void CallAnOperation()

    {

        string fileName = string.Empty;

        int lengthOfFile = 0;

 

        Getter get = new Getter();

        get.GetFile(ref fileName, ref lengthOfFile);

 

        Doer doGuy = new Doer();

        int retValue = doGuy.DoSomething(fileName, lengthOfFile);

 

        if(retValue != 23) //throw an exception, etc.

    }

}

 

public class Getter

{

    public void GetFile(ref string name, ref int length)

    {

        //Do some work to get file, and then. . .

        name = "myFileName.txt";

        length = 98;

    }

}

 

public class Doer

{

    public int DoSomething(string fileName, int fileLength)

    {

        //Do something with this information. . .

        //We'll pretend the the operation succeeded and return 23;

        return 23;

        //If the operation had failed, we would have returned 67.

    }

}

There are several issues with the above code.  Kudos to you if you already know what's going on. 

First, my CallAnOperation() method needs values for fileName, and lengthOfFile.  He passes references to these two into a class that initializes them to correct values.  This is the first mistake.  Now the CallAnOperation() method has to have intimate knowledge of the inner behavior of the GetFile method.  This is bad and leads to tightly-coupled code that doesn't make sense.  Instead, the GetFile method should return an object that contains the needed information. 

The second issue is with the DoSomething method.  This method has custom return codes to signal if things are good or bad.  This requires the user code to know that "23" is a good number, and "67" is a bad number.  Again, this is forcing knowledge onto the client.  The CallAnOperation() method shouldn't have to know how a component functions.  The method names should be descriptive enough, and the return value should be quickly understandable.  At most, returning true or false may be acceptable, but a more preferrable approach is:  if something unacceptable happened, throw an exception.  If validation is in the method and is the cause for a return value instead of an exception, consider refactoring the validation into a separate method.



Comments

David Hayden said:

I understand your point, but not sure this example brings it to fruition.

First, naming a test CallAnOperation(), is really bad as it gives me absolutely no idea as to what you are testing and the expected result. Step #1 is to pick a descriptive test name that is more self-documenting.

Second, is it not obvious as to why you are initializing variables before the call to the GetFile method. Maybe you are doing this to avoid an ArgumentNullException? Since you are not doing argument validation and the code in the routine is sparse, I have no clue why the variables are being initialized and what best practice is being violated here.

Third, tests are not the same as client code. Often we are testing an interface contract and hence are using the intimate knowldege of this contract to verify it is not being broken. Therefore, there is an expected level of coupling here that is above and beyond what would be done in client code.

For example, if you are testing code that calulates the area of a rectangle:

public int Area (Rectangle r)
{
return r.length * r.width;
}

a test of

[Test]
public CalculateAreaOfRectangle()
{
Rectangle r = new Rectangle(5,4);
Assert.Equals(20, Area(r));
Assert.Equals(r.length, 5);
Assert.Equals(r.width, 4);
}

has intimate knowledge of this subroutine. We know how to calculate the area of a rectangle and are verifying that the expected contract of this subroutine is not being broken. We not only verify the calculation is correct, but we are also verifying that the length and width of the rectangle are not being changed per the contract. Hence, intimate knowledge is being used accordingly and accurately in my opinion.

Great post as it got me thinking.
# August 12, 2005 4:25 PM

Jeffrey Palermo said:

David,
Good input. You are completely right. I've change the code to show user code (controller code), and not test code.
# August 12, 2005 4:37 PM

Anonymous said:

It looks like the coder is someone who comes from the C (maybe C++) world as has no idea about OO programming. I've seen similar examples from programmers who know C/C++ very well but have not made the proper transition to C#. Lots of function calls thrown into classes just so it can seem like OO.
# August 12, 2005 8:55 PM

Kevin Barnes said:

I like articles like this because they are specific and offer a concrete example. dhayden's comment about naming is a good one. People seem to understand examples best when they have concrete (even if artifical) names and context.

In addition to the objections you have made to the example cod, here are a few more:

1) Neither Getter nor Doer nor Cleint class appears to have a meaningful job. All the methods are not instance associated (their behavior appears to be unrelated to any obvious class function). Their existence as classes (or the presence of these methods in the class) is perhaps suspect (more context is needed to say for sure).

2) Doer uses file information that (in some sense) appears to be owned by Getter. If we do make the return of doer an object, then we also need to make doer take that object since the returned object is really an aggregate data type that multiple systems appear to want to use. This may also indicate that Getter itself should BE that aggregate data type and doer should simply be passed an instantiated Getter (assuming that we could distinguish one getter from another which at this point we cant)


3) Your suggestion about throwing an exception is valid in many cases, but a boolean return is probably better if failure is a reasonably common and expected result (IMO). Also consider other systemic solutions to error handeling (if this is a large service class). For example, allowing an error manager to be registered. This would only make sense in larger scale services where encapsulating error processing makes sense.

# August 15, 2005 7:33 AM

Anonymous Coward said:

Who cares what the names are in this case? It's a friggen example, and a specific one at that. He had issues with coupling and return codes. He blogged about them. And off we go griping about names he chose in haste because they had nothing to do with the specific issues he wanted to address. Can't people just note that he made an interesting point and either accept it or comment on it (specifically)? What's the need for "Great post, but you mis-spelled 7 words, you have 4 grammatical errors, and I think you completely missed the ball on a related issue that you clearly didn't give a crap about." To everyone's credit, the bitching was accompanied by interesting comments, so I'm only half peeved.
# August 15, 2005 11:01 AM

Hasani said:

I hate it when people unintentionally make properties in the following matter:

public class SomeClass
{
private ArrayList _values;

public ArrayList Values
{
get
{
return _values;
}
set
{
_values = value;
}
}
}

instead of.

public class SomeOtherClass
{
private readonly ArrayList _values;

SomeOtherClass()
{
_values = new ArrayList();
}

public ArrayList Values
{
get
{
return _values;
}
}
}

And in the cases where I've seen this, it's because people don't take into account that in the 1st class, invoing set_Values() with a different instance of an ArrayList will not automatically invalidate code that was referencing the previous collection :(

I have a couple more 'WTF where they thinking' samples but I would need a good hour or two to go in to detail w/ those. The return code instead of throwing exceptions really makes me want to bitch slap those coders sometimes. SO F****ING AGGRAVATING. At least using a F***ING ENUM if you're gonna go down that path!!!!!!!

Looks like I said too much already... l8rz =]
# August 17, 2005 4:01 PM

About Jeffrey Palermo

Jeffrey Palermo is a software management consultant and the CTO of Headspring Systems in Austin, TX. Jeffrey specializes in Agile coaching and helps companies double the productivity of software teams. Jeffrey is an MCSD.Net , Microsoft MVP, Certified Scrummaster, Austin .Net User Group leader, AgileAustin board member, INETA speaker, INETA Membership Mentor, Christian, husband, father, motorcyclist, Eagle Scout, U.S. Army Veteran, and Texas A&M University graduate. Check out Devlicio.us!

This Blog

Syndication