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\$
  • \$\begingroup\$ What are the types of Addresses and Address? Can you change them, perhaps using generics, to avoid having to cast altogether? \$\endgroup\$ – Saeed Feb 8 '11 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\$ – pstrjds Feb 8 '11 at 18:32
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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I am using .net 2.0 so instead var we need to use Adress. Thanks \$\endgroup\$ – thedev Feb 8 '11 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\$ – Snowbear Feb 8 '11 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\$ – Saeed Feb 8 '11 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\$ – Snowbear Feb 8 '11 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\$ – quentin-starin Feb 8 '11 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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ FirstName.ToString() ... done \$\endgroup\$ – thedev Feb 8 '11 at 15:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.