4
\$\begingroup\$

This sample code works fine, but it looks awful. How would you improve this?

data.Add(((Adress)(((OwnerIDList)owner.Adresses.Value)[0].Adress.Value)).FirstName.Value.ToString());
data.Add(((Adress)(((OwnerIDList)owner.Adresses.Value)[0].Adress.Value)).LastName.Value.ToString());

Why do we use .Value in FirstName.Value.ToString()?

FirstName is a DTString object (implements a basic interface for all data types to be stored in data base).

\$\endgroup\$
2
  • \$\begingroup\$ What are the types of Addresses and Address? Can you change them, perhaps using generics, to avoid having to cast altogether? \$\endgroup\$ Commented Feb 8, 2011 at 11:46
  • \$\begingroup\$ I see in the code sample you are using the index of 0, but I was wondering if this is actually being done in a loop. If so, I would also suggest that extract a variable for the OwnerIDList outside of the loop. \$\endgroup\$ Commented Feb 8, 2011 at 18:32

2 Answers 2

Reset to default
8
\$\begingroup\$

at least I would extract a variable: 

var address = (Adress)((OwnerIDList)owner.Adresses.Value)[0].Adress.Value;
data.Add(address.FirstName.Value.ToString());
data.Add(address.LastName.Value.ToString());

All these cast operations make me believe that your code is not that strongly typed. If it so then there is not that much you can do with readability in this code.

P.S.: Address in English has two d.

\$\endgroup\$
6
  • \$\begingroup\$ I am using .net 2.0 so instead var we need to use Adress. Thanks \$\endgroup\$ Commented Feb 8, 2011 at 11:55
  • 1
    \$\begingroup\$ @thedev, as far as I'm aware you can use var even for .Net 2.0 if you're using VS 2008 \$\endgroup\$ Commented Feb 8, 2011 at 13:04
  • 1
    \$\begingroup\$ @Snowbear, but why would you want to in this situation? Using Adress is strictly better if you ask me. var saves you a few keystrokes while writing the code, but Adress gives you additional information about the type of the variable every time you read it. IMO, var should be used for its intended purpose only - to declare a variable that will hold an anonymous type. \$\endgroup\$ Commented Feb 8, 2011 at 21:55
  • 1
    \$\begingroup\$ @Saeed, I do not agree. I believe you should have your variables named in that way so you will not need ANY additional information. Address address = ... is redundant. \$\endgroup\$ Commented Feb 8, 2011 at 23:46
  • \$\begingroup\$ @Saeed: And I will add that with the cast to Adress on the right of the assignment, the type is already very visible in the statement. Using var here would be DRY. \$\endgroup\$ Commented Feb 8, 2011 at 23:50
2
\$\begingroup\$

What's with all the .Value is it because the variables are defined as Nullable< T > variables?

In that case, you can substitute 

FirstName.Value.ToString()

With

FirstName.ToString()

Besides that, the place where I would clean up would be the actual objects that you are querying for data. You have a lot of casts, and a lot of .Value lookups to get to the real data.

This results in the code that uses those classes reflects the design choices made when implementing those classes, And that is the real reason why your code looks awful.

I would change those types, so you would be able to write:

data.Add(owner.Adresses[0].Adress.FirstName);

Also it seems illogical that an object that you retrieve from an Addresses collection should have a property called Address? Maybe that should have a better name.

And as already mentioned, there are two d's in address.

\$\endgroup\$
1
  • \$\begingroup\$ FirstName.ToString() ... done \$\endgroup\$ Commented Feb 8, 2011 at 15:06

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.