2
\$\begingroup\$

I am writing a small application to generate the domain name of computers, from their known properties, and business rules.

My Asset class is:

public enum AssetSite
{
    Rotterdam,
    Sydney,
}

public class Asset
{
/// <summary>
/// Gets the Asset Tag of the asset.
/// </summary>
public string AssetTag { get;}

/// <summary>
/// Gets the site to which the asset belongs.
/// </summary>
public AssetSite AssetSite { get; } 

/// <summary>
/// Gets a value indicating whether the asset is a laptop.
/// </summary>
public bool IsLaptop { get; }   

/// <summary>
/// Gets the name of the asset.
/// </summary>
public string Name
{
    get
    {
        var nameBuilder = new StringBuilder();
        nameBuilder.Append('I'); // Convention: All names start with I
        
        // Second letter indicates the location of the PC.
        if (this.AssetSite == AssetSite.Sydney)
        {
            nameBuilder.Append('S');
        }
        else if (this.AssetSite == AssetSite.Rotterdam)
        {
            nameBuilder.Append('R');
        }
        else
        {
            throw new ArgumentOutOfRangeException("Invalid site");
        }
        
        nameBuilder.Append("PC"); //Then, all PCs have 'PC' in their name.

        if (this.IsLaptop)
        {
            nameBuilder.Append("L"); //Laptops get a "L" in their name
        }
        else
        {
            nameBuilder.Append("D"); //Desktops get a "D"
        }

        nameBuilder.Append(this.AssetTag); //Unique identifier of the computer

        return nameBuilder.ToString();
    }
}

Asset objects are created through the deserialization of a JSON file

I am worried about my implementation of the Name getter. It feels way too long and complex, but at the same time, extracting each letter generation to a dedicated method feels way to complex as well.

My main concerns here are readability and maintainability.

\$\endgroup\$
  • \$\begingroup\$ Not worthy of a full review, so I'll put it here: comments should not describe what but why. Things like //Desktops get a "D" should be clear from the code, otherwise you'd need to rewrite your code. \$\endgroup\$ – BCdotWEB Jun 13 at 9:30
2
\$\begingroup\$

I would highlight two things:

  1. It is not really common to throw exception inside a property getter
  2. Because the Name has only just a getter then you can consider to expose it as a method instead.

From implementation point of you, you can improve readability if you do something like this:

var nameBuilder = new StringBuilder();
nameBuilder.Append('I'); 

var siteWhitelist = new AssetSite[] { AssetSite.Sydney, AssetSite.Rotterdam };
if (!siteWhitelist.Contains(this.AssetSite))
    throw new ArgumentOutOfRangeException("Invalid site");

string site = this.AssetSite switch
{
    AssetSite.Sydney    => "S";
    AssetSite.Rotterdam => "R"; 
};

nameBuilder.Append(site);
nameBuilder.Append("PC"); 

string computerType = this.IsLaptop ?  "L" : "D";

nameBuilder.Append(computerType)
nameBuilder.Append(this.AssetTag);

return nameBuilder.ToString();

Of course you can further improve it, by introducing a Dictionary<AssetSite, string> where you can store the mapping between sites and letters.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

It is hard to tell because the AssetSite enum is small. Is the logic for generating Name is to take the first letter from AssetSite? If so, no need for ifs or switch case. And you don't need to update the function when a new site is added to the enum.

Error handling

In general, the creation of class should fail if it's data is invalid.

Tomorrow you will add more code that use AssetSite, you don't want to handle invalid AssetSite in all the places that use it.

What is the validation for AssetSite? Can it only be only from a closed set of values? Are you sure it should be an enum and not a string?

Conciseness

Maybe it is a matter of taste, I prefer to call property instead of this.Property.

| improve this answer | |
\$\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.