5
\$\begingroup\$

I am creating instance of the same object to populate its properties twice. I need to give this object to another developer who just don't need constructor at all, as he is sending me object through WCF service.

var a = new Earth();
a.ID= 11;
a.UpdateUniverse= true;
a = new Earth(a); // use age and Taxrate to populate rest of Earth's properties
// and call dataAccess as well to get some records
ManagerEarth.Add(a);

Here is the class you want to see:

[DataContract(Name = "Earth")]
public class Earth
{
    [DataMember]
    public int ID;
    [DataMember]
    public int PlanetID;
    [DataMember]
    public string PlanetUrl;
    [DataMember]
    public string UserID;
    [DataMember]
    public string Environment;
    [DataMember]
    public string Status;

    [DataMember(Name = "StartDate")]
    public string StartDateString
    {
        get
        {
            if(this.StartDate.HasValue)
                return this.StartDate.Value.ToString(Utils.C_DateFormat);
            else
                return null;
        }
        set
        {
            this.StartDate = Utils.ToNullableDateTime(value);
        }
    }
    public DateTime? StartDate;

    [DataMember(Name = "EndDate")]
    public string EndDateString
    {
        get
        {
            if(this.EndDate.HasValue)
                return this.EndDate.Value.ToString(Utils.C_DateFormat);
            else
                return null;
        }
        set
        {
            this.EndDate = Utils.ToNullableDateTime(value);
        }
    }
    public DateTime? EndDate;

    [DataMember]
    public bool UpdateUniverse;
    [DataMember]
    public string AuditAddress;

    public Earth()
    {

    }

    public Earth(Earth m)
    {
        if(SPContext.Current == null || SPContext.Current.Web == null)
            return;

        var planetX = new EarthDataAccess().GetByURL(SPContext.Current.Web.Site.Url);
        if(planetX == null || planetX.PlanetID < 1)
            return;

        this.PlanetID = planetX.PlanetID;
        this.PlanetUrl = planetX.PlanetHomeURL;
        this.AuditAddress = planetX.GoHomeUrl;

        this.ID = m.ID;
        this.StartDate = m.StartDate;
        this.EndDate = m.EndDate;

        this.Status = !string.IsNullOrEmpty(m.Status) ? m.Status : "Active";
        if(this.Status != "Destroyed" && this.EndDate != null)
            this.Status = "Ended";

        this.Environment = !string.IsNullOrEmpty(m.Environment) ? m.Environment : "1";
        this.UserID = m.UserID;
        this.UpdateUniverse = m.UpdateUniverse;
    }
}

Some other methods:

public static string C_DateFormat = "dd/MM/yyyy HH:mm:ss";

public static DateTime? ToNullableDateTime(string dateTime)
{
    if(string.IsNullOrEmpty(dateTime))
        return (DateTime?) null;

    var ci = CultureInfo.InvariantCulture;
    var formats = new[] { "yyyy-MM-dd", "yyyy-MM-dd HH:mm:ss", "M-d-yyyy", "dd-MM-yyyy", "dd/MM/yyyy", "dd/MM/yyyy HH:mm:ss", "MM-dd-yyyy", "M.d.yyyy", "dd.MM.yyyy", "MM.dd.yyyy" }
            .Union(ci.DateTimeFormat.GetAllDateTimePatterns()).ToArray();
    var dt = DateTime.ParseExact(dateTime, formats, ci, DateTimeStyles.None);

    return dt;
}
\$\endgroup\$
1
  • \$\begingroup\$ You are correct to believe this is a code smell. Do you have actual example usage that he would do, so that we may help find the weak points? \$\endgroup\$ Commented Aug 3, 2015 at 12:29

1 Answer 1

1
\$\begingroup\$

It sounds like Earth cannot exist without an ID and the UpdateUniverse boolean flag. As such, you need a constructor that enforces this:

public Earth(int id, bool updateUniverse)
{
    // the rest of the code that went in Earth(Earth m)
}

If you still need a parameterless constructor because WCF needs it, that's fine. You can extract that constructor into its own method:

public Earth(int id, bool updateUniverse)
{
    SetId(id, updateUniverse);
}

public void SetId(int id, bool updateUniverse)
{
    // the rest of the code that went in Earth(Earth m)
}

I'm also questioning why this class needs a reference to the data access layer at all. It actually looks like you need planetX for an Earth to even exist.

What is planetX, another Earth? I'm getting lost here. Which Earth are we talking about, and how many are there? The real problem is that this class is doing too much. It is doing data access, which needs to happen in the code using the Earth objects, not inside the Earth class.

Judging by your first code block, this seems more appropriate:

Earth earth = new EarthDataAccess().GetById(11);

// now just use a fully populated Earth object

Pulling an Earth object from the database should not involve more lines of code than that.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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