Hugoware

The product of a web developer with a little too much caffeine

Copyin’ and-a Pastin’

with 2 comments

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…

  1. Add 5 more settings as properties
  2. Change the path of the settings file to ‘D:\settings\storage.xml’
  3. Change the location of the setting from the root of the document to ‘user/settings/personal’
  4. 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..

Anyways, just something I was thinking about today – Go out there and code!

Anyways, just something I was thinking about today – Go out there and code!

… just kidding…

Advertisements

Written by hugoware

March 12, 2010 at 12:21 am

2 Responses

Subscribe to comments with RSS.

  1. This falls into what I call the 2 am rule: it’s 2 am, you have code where the weak points are not isolated, and you have read, read, read it to understand it. Oops, I fat fingered a directory path.

    If it’s 2 am, I want you second example. I can easily recognize the points where it could be failing. The past “me” did the present “me” a favor by minimizing points of change and thinking about maintenance.

    ActiveEngine Sensei

    March 12, 2010 at 6:52 am

    • I’ve actually used the ‘2AM Rule’ with some of the devs I teach – I also have referred to it as the ‘6 months later’ rule – Are you really going to be able to understand your code when you come back to it? Have you minimized the decision points in your code? Are you repeating yourself?

      Always a fun discussion but some of these lessons just have to be learned by the developer when they get bitten by it.

      hugoware

      March 12, 2010 at 10:31 am


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: