10
\$\begingroup\$

Please suggest ways of cleaning up this code.

Hashtable newValues = e.Values;

string NewPosition = null;
string NewFirst = null;
string NewLast = null;
string NewEmail = null;
int NewAppt = 0;

if (newValues["Position"] == null)
{
    NewPosition = "";
}
else
{
    NewPosition = newValues["Position"].ToString();
}

if (newValues["First Name"] == null)
{
    NewFirst = "";
}
else
{
    NewFirst = newValues["First Name"].ToString();
}

if (newValues["Last Name"] == null)
{
    NewLast = "";
}
else
{
    NewLast = newValues["Last Name"].ToString();
}

if (newValues["Email"] == null)
{
    NewEmail = "";
}
else
{
    NewEmail = newValues["Email"].ToString();
}

if (newValues["Appts"] == null)
{
    NewAppt = 0;
}
else
{
    NewAppt = 1;
}
\$\endgroup\$
  • \$\begingroup\$ I would suggest rewording the question to only have a representative snippet of that pattern, rather than repeating it 5 times \$\endgroup\$ – Rob H Sep 25 '12 at 19:23
  • \$\begingroup\$ I didn't know that was an option. Thanks @DaveZych. I will keep that in mind for future postings like this one. \$\endgroup\$ – James Wilson Sep 25 '12 at 20:02
15
\$\begingroup\$

You can use the null-coalescing operator ?? like this:

NewPosition = (newValues["Position"] ?? "").ToString();

The core of this approach is this expression:

newValues["Position"] ?? ""

The ?? operator evaluates the left expression first (i.e. newValues["Position"]), and uses it as the result if it's not null. If the first expression is null, the second expression is evaluated, and its result is returned as the overall result of the expression.

For the last one, use ?:

NewAppt = (newValues["Appts"] != null) ? 1 : 0;
\$\endgroup\$
  • \$\begingroup\$ Does this also assign the value if it is not null? Could you explain how this works just so I understand it? \$\endgroup\$ – James Wilson Sep 25 '12 at 19:24
  • \$\begingroup\$ And do I create the var first like string NewPosition; or Can I do a full assignment like string NewPosition = (newValues["Position"] ?? "").ToString(); \$\endgroup\$ – James Wilson Sep 25 '12 at 19:25
  • 4
    \$\begingroup\$ +1 Outstanding. Though I'd replace "" with string.Empty as a matter of habit. \$\endgroup\$ – Jesse C. Slicer Sep 26 '12 at 14:46
  • 1
    \$\begingroup\$ @AMissico It does make things harder to developers unfamiliar with the ?? operator. Developers who are familiar with their basic technology, on the other hand, tend to appreciate the beauty and the brevity of this approach. \$\endgroup\$ – Sergey Kalinichenko Oct 22 '13 at 19:35
  • 1
    \$\begingroup\$ @AMissico Note that ?? can be replaced with + without changing the validity of your statement. Yet very few people in their right mind write AddOne(length) instead of length+1, and it does not make anyone's code any harder to "maintain, reuse, and read". \$\endgroup\$ – Sergey Kalinichenko Oct 22 '13 at 20:10
8
\$\begingroup\$

While these are all good suggestions, I would wrap the code into a method.

NewPosition = GatherValue("Position");
NewFirst = GatherValue("First Name");

...
string GatherValue(string name) {
    return newValues[name] == null ? "" : newValues[name].ToString(); 
}
\$\endgroup\$
  • \$\begingroup\$ Does this offer any benefit over the other option, or is it just personal preference? \$\endgroup\$ – James Wilson Sep 25 '12 at 19:32
  • \$\begingroup\$ @James: Data container access method encapsulation/extraction. \$\endgroup\$ – abatishchev Sep 25 '12 at 20:01
  • \$\begingroup\$ I would need to make a seperate method for the int portion I am assuming? Or modify the GatherValue to return a different value if the one passed in is Appts right? \$\endgroup\$ – James Wilson Sep 25 '12 at 20:04
  • \$\begingroup\$ @James Wilson; Yes, create another method for the int. \$\endgroup\$ – AMissico Sep 27 '12 at 0:58
  • \$\begingroup\$ Personal preference based on a couple decades of experience. :O) \$\endgroup\$ – AMissico Sep 27 '12 at 0:59
3
\$\begingroup\$

In case of Hashtable:

string value = ht["key"] as string ?? "";

But it's better to use generic thus type-safe Dictionary<string, string>:

string value = dic["key"] ?? "";

or Dictionary<string, objects>:

string value = ((string)dic["key"]) ?? "";
\$\endgroup\$
3
\$\begingroup\$

My preferred option is using the ternary operator when you are doing operations such as that.

Here is a brief example:

string NewPosition = newValues["Position"] != null ? newValues["Position"].ToString() : String.Empty;

The way it work is like this:

variable = boolean expression ? value that gets assigned when true : value that gets assigned when false.

You must be careful not to make your code difficult to read when using the ternary operator since it puts it in one line, however I find in most cases it makes it more readable.

\$\endgroup\$
2
\$\begingroup\$

This is a pattern that will reduce the lines, and IMO, make it easier to read.

Old:

if (newValues["Position"] == null)
{
    NewPosition = "";
}
else
{
    NewPosition = newValues["Position"].ToString();
}

New:

NewPosition = newValues["Position"] == null ? string.Empty : newValues["Position"].ToString();

As for the integer field:

NewAppt = newValues["Appts"] == null ? 0 : 1;

Documentation: http://msdn.microsoft.com/en-us/library/ty67wk28(v=vs.100).aspx

\$\endgroup\$
2
\$\begingroup\$

Other than null coalescing operator, you could also simply add empty string

NewFirst = newValues["First Name"]+"";

For the following reason:

7.8.4 Addition operator

String concatenation:

string operator +(string x, string y); string operator +(string x, object y); string operator +(object x, string y);

These overloads of the binary + operator perform string concatenation. If an operand of string concatenation is null, an empty string is substituted. Otherwise, any non-string argument is converted to its string representation by invoking the virtual ToString method inherited from type object. If ToString returns null, an empty string is substituted.

This will not work for integers/booleans/etc. Mostly just for strings or anything the .ToString() overload gives you that you actually want. For other cases i'd suggest null coalescing operator or some other approach like other answers provide here.

\$\endgroup\$
  • \$\begingroup\$ This is an old hack that relies on the storage variable to be a string. It is not an improvement. Over the years, this trick has caused by problems in two projects. \$\endgroup\$ – AMissico Sep 27 '12 at 1:03
  • \$\begingroup\$ This makes is shorter at the expense of introducing a hack (relying on coincidence). Sometimes this is valid but in this case I would disagree. It is not needed. \$\endgroup\$ – usr Sep 30 '12 at 19:52
1
\$\begingroup\$

This code could be refactored like this:

HashTable newValues = e.Values;
string NewPosition = string.Empty;
string NewFirst = string.Empty; 
string NewLast = string.Empty;
string NewEmail = string.Empty;
int NewAppt;

NewPosition = (newValues["Position"] ?? string.Empty).ToString();
NewFirst = (newValues["First Name"] ?? string.Empty).ToString();
NewLast = (newValues["Last Name"] ?? string.Empty).ToString();
NewEmail = (newValues["Email"] ?? string.Empty).ToString();
NewAppt = newValues["Appts"] == null ? 0 : 1

Things to note:

  • you don't need to declare the int as 0 - it defaults to 0, NB: C# structs (like int) are not nullable.
  • If you are using a recent version of c#, use the var keyword to improve the readability of your variable declarations.
\$\endgroup\$
  • \$\begingroup\$ This will just result in null reference exceptions when the values are null \$\endgroup\$ – Servy Sep 25 '12 at 19:45
  • \$\begingroup\$ Now it's just like several other existing answers... \$\endgroup\$ – Servy Sep 25 '12 at 19:53
  • \$\begingroup\$ @Servy: Doesn't deserve downvote though imo. \$\endgroup\$ – abatishchev Sep 25 '12 at 20:00
  • \$\begingroup\$ Why initialize the local string variables when they are instantly replaced? Extra coding and loss of performance. \$\endgroup\$ – Jesse C. Slicer Sep 26 '12 at 14:48
  • 1
    \$\begingroup\$ @Kryptonite Actually, as it seems these are all done as local variables in a method, they won't be null if you remove the initialization altogether. They'll be undefined until assigned, which the assignments immediately do. \$\endgroup\$ – Jesse C. Slicer Sep 27 '12 at 18:36
0
\$\begingroup\$

Given you're using c# 4 I'd suggest to create a static extension method for your values.

public static class MyExtensions
{
    public static string ToValueOrStringEmpty(this object myString)
    {
        return string.IsNullOrEmpty((string)myString) 
               ? string.Empty 
               : (string)myString;
    }
}

Your code now is:

NewPosition = newValues["Position"].ToValueOrStringEmpty();
NewFirst = newValues["First Name"].ToValueOrStringEmpty();
NewLast = newValues["Last Name"].ToValueOrStringEmpty();
NewEmail = newValues["Email"].ToValueOrStringEmpty();
\$\endgroup\$

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.