How can I make this cleaner?

Question

I know there are some nifty ways out there to clean this up, I'm just not sure what they are. I'm not familiar enough with c# to know of the best ways. Anyone out there better than me and can help me! :)

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;
}

Answer 1

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;

Answer 2

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(); 
}

Answer 3

NewFirst = newValues["First Name"] == null ? "" : newValues["First Name"].ToString();

Answer 4

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"]) ?? "";

Answer 5

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.

Answer 6

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

Answer 7

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

Answer 8

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.

Answer 9

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.

Answer 10

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

NewFirst = (newValues["First Name"] == null) ? "" : newValues["First Name"].ToString();


NewLast = (newValues["Last Name"] == null) ? "" : newValues["Last Name"].ToString();

NewEmail = (newValues["Email"] == null) ? "" : newValues["Email"].ToString();
NewAppt = (newValues["Appts"] == null) ? 0 : 1;

Answer 11

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();