Copyin' and-a Pastin'

How often do you use Copy and Paste in your code -- because you really shouldn't...

I'm not really a very opinionated person when it comes to software development but this is one point I tend to be adamant about with other developers. What you ought to be doing is Cutting and Pasting your code.

If I happen to catch a new developer copying some code I normally lead off with that sentence to which I get that raised eyebrow, "what is this dude talking about?" look but it is always a fun discussion.

You're probably already realize the difference between the two, duplication as opposed to refactoring, but so far every new developer I've encountered hadn't even heard the term before. Maybe it is just a matter of coincidence but I doubt it.

Normally, I show a simple example of how by refactoring you can limit errors and reduce how much work you have to do. A lot of you are going to see the problem with this right away...

//a user settings file
public class UserSettings {

    //holds the settings for the user
    public XDocument Settings { get; set; }

    //the users font color
    public string FontColor {
        get {
            if (this.Settings == null) {
                this.Setting = XDocument.Load(@"c:\settings.xml");
            }
            return this.Settings.Root.Element("fontColor").Value;
        }
        set {
            if (this.Settings == null) {
                this.Settings = XDocument.Load(@"c:\settings.xml");
            }
            this.Settings.Root.Element("fontColor").Value = value;
            this.Settings.Save(@"c:\settings.xml");
        }
    }

    //the size of the font
    public int FontSize {
        get {
            if (this.Settings == null) {
                this.Setting = XDocument.Load(@"c:\settings.xml");
            }
            int size = 0;
            int.TryParse(this.Settings.Root.Element("fontSize").Value, out size);
            return size;
        }
        set {
            if (this.Settings == null) {
                this.Settings = XDocument.Load(@"c:\settings.xml");
            }
            this.Settings.Root.Element("fontSize").Value = value;
            this.Settings.Save(@"c:\settings.xml");
        }
    }

    //the users font color
    public string FontFamily {
        get {
            if (this.Settings == null) {
                this.Setting = XDocument.Load("c:\\settings.xml");
            }
            return this.Settings.Root.Element("fontFamily").Value;
        }
        set {
            if (this.Settings == null) {
                this.Settings = XDocument.Load("c:\\settings.xml");
            }
            this.Settings.Root.Element("fontFamily").Value = value;
            this.Settings.Save("c:\\settings.xml");
        }
    }

}

A bit extreme? I doubt it - In fact, I'm willing to bet some devs have found code like this floating around in a project. So after discussing and refactoring we normally end up with something like this...

//a user settings file
public class UserSettings {

    private const string SETTINGS_PATH = @"c:\settings.xml";
    private const string SETTING_FONT_COLOR = "fontColor";
    private const string SETTING_FONT_SIZE = "fontSize";
    private const string SETTING_FONT_FAMILY = "fontFamily";

    //holds the settings for the user
    public XDocument Settings {
        get {
            if (this._Settings == null) {
                this._Setting = XDocument.Load(SETTINGS_PATH);
            }
            return this._Settings;
        }
    }
    private XDocument _Settings;

    //saves the settings file
    public void _SaveSettings() {
        this.Settings.Save(SETTINGS_PATH);
    }

    //gets the value for a setting
    private string _GetSettingValue(string name) {
        return this.Settings.Root.Element(name).Value;
    }

    //modifies a setting value
    private string _ChangeSetting(string name, object value) {
        this.Settings.Root.Element(name).Value = value;
        this._SaveSettings();
    }

    //the users font color
    public string FontColor {
        get { return this._GetSettingValue(SETTING_FONT_COLOR); }
        set { this._ChangeSetting(SETTING_FONT_COLOR, value);  }
    }

    //the size of the font
    public int FontSize {
        get {
            int size = 0;
            int.TryParse(this._GetSettingValue(SETTING_FONT_SIZE), out size);
            return size;
        }
        set { this._ChangeSetting(SETTING_FONT_SIZE, value); }
    }

    //the users font type
    public string FontFamily {
        get { return this._GetSettingValue(SETTING_FONT_FAMILY); }
        set { this._ChangeSetting(SETTING_FONT_FAMILY, value); }
    }

}

... And then the blank looks appear -- two lines? We saved a measly two lines? However, this is the part where you ask them to make a change both of the code samples. Here are a couple samples I like to use...

Add 5 more settings as properties
Change the path of the settings file to 'D:\settings\storage.xml'
Change the location of the setting from the root of the document to 'user/settings/personal'
Adjust the code so that you don't get an error if the element name isn't found

Of course, the point isn't that using copy and paste is bad - but duplicating a bunch of lines instead of focusing on the architecture of your code can quickly lead to unmaintainable code. It is always worth your time to review your code and look for the areas that are repeating themselves..

March 11, 2010