GitHub Copilot writes correct code...with lots of code smells

Brian C Hart

Dear Microsoft,

Love Copilot in Visual Studio 2022 in Agent Mode. I am just getting started with it but it has a lot of promise. It can be a big timesaver...or should I be addressing this to Anthropic? :)

Using Claude Sonnet 4, and the GitHub Copilot Chat window in Agent mode, on a WinForms project, I asked for a toolbar button that can toggle a listview from displaying items in ascending or descending order -- clicking the button repeatedly should toggle between the two.

This is what vibe coding gave me (with Copilot in Agent mode and using Claude Sonnet 4):

/// <summary>
/// Sorts the projects alphabetically in the list view.
/// </summary>
public void SortProjects()
{
    try
    {
        // Toggle sort order
        _sortAscending = !_sortAscending;
        
        if (_sortAscending)
        {
            _displayedProjects = _displayedProjects.OrderBy(p => p.Name).ToList();
            
            // Update the sort button text to indicate current state
            if (View?.Toolbar?.Items["SortButton"] is ToolStripButton sortButton)
            {
                sortButton.Text = "Sort Descending";
                sortButton.ToolTipText = "Sort Descending";
            }
        }
        else
        {
            _displayedProjects = _displayedProjects.OrderByDescending(p => p.Name).ToList();
            
            // Update the sort button text to indicate current state
            if (View?.Toolbar?.Items["SortButton"] is ToolStripButton sortButton)
            {
                sortButton.Text = "Sort Ascending";
                sortButton.ToolTipText = "Sort Ascending";
            }
        }
        
        RefreshProjectList();
    }
    catch (Exception ex)
    {
        // dump all the exception info to the log
        DebugUtils.LogException(ex);
    }
}

Listing 1. Vibe-coded solution with Claude Sonnet 4.

The code compiles perfectly but it has a lot of smells. There is no null-checking, there is no use of the ?: operator (when there should be), and there is repeated code.

These are pretty bad code smells IMHO. What I might potentially write, instead, would be Listing 2:

/// <summary>
/// Sorts the projects alphabetically in the list view.
/// </summary>
public void SortProjects()
{
    try
    {
        // Toggle sort order
        _sortAscending = !_sortAscending;

        _displayedProjects = _sortAscending
            ? _displayedProjects.OrderBy(p => p.Name)
                                .ToList()
            : _displayedProjects.OrderByDescending(p => p.Name)
                                .ToList();

        // Update the sort button text to indicate the current state
        if (View?.Toolbar?.Items["SortButton"] is a ToolStripButton
            sortButton && sortButton != null && !sortButton.IsDisposed)
        {
            sortButton.Text = _sortAscending
               ? "Sort Descending"
               : "Sort Ascending";
            sortButton.ToolTipText = sortButton.Text;    // UI/UX convention
        }

        RefreshProjectList();
    }
    catch (Exception ex)
    {
        // dump all the exception info to the log
        DebugUtils.LogException(ex);
    }
}

Listing 2. This is an example of code that a senior dev, such as myself, would write.

Listing 2 shows tight, professionally-written, C# code with much fewer code smells. The sortButton variable is checked for null, and .IsDisposed == true before being used. I know the is keyword is supposed to implicitly check for null, but that is not the case -- it simply attempts to do a cast; if the cast is not successful, then it sets sortButton == null. Furthermore, just in case this method is getting called while the window is being closed, then we must also make sure that sortButton.IsDisposed == false. Copilot did not write that code. Since the same code was in both if` branches, it was moved to the outer scope...this is a big code smell on Copilot's part.

Finally, using if and else branches is not necessary -- using the ?: operator is cleaner code and a better practice. For example, the _displayedProjects field can be assigned using the ?: operator. As another example, the sortButton.Text can be set using the ?: operator, and then the sortButton.ToolTipText property is set equal to the sortButton.Text property (which is a WinForms UI/UX convention, but not mandatory.

There is are code smells in Listing 1 and Listing 2, pop quiz --- can you find them all?

:-)

Respectfully,

Brian Hart

PIEBALDconsult

How was that Kool-Aid?

Graeme_Grant

@Brian-C-Hart said in GitHub Copilot writes correct code...with lots of code smells:

This is what vibe coding gave me (with Copilot in Agent mode and using Claude Sonnet 4):

Prompts are what determine the code that is generated. Code smells usually come from the quality of the prompt and the length of the session. You have not given your prompt, nor mentioned how many prompts you used to achieve the output. Yes, in my Experience, Claude Sonnet 4 is the best at large tasks, followed closely by Grok, then Gemini, and finally by GPT-4, GPT-4.1, and GPT-5 at the rear.

Recently, Github released a Spec Kit CLI tool to help devs engineer prompts to impove quality of code generated. Here is their release video Introducing Spec Kit for Spec-Driven Development! and Microsoft did something similar: VS Code Copilot Extension is Open Source.

honey the codewitch

They did an initial study on developer productivity vs perceived productivity whist using AI tools. Turns out devs thought it made them faster, but it actually makes them slower. I guess good prompting is a huge time suck.

I won't typically use Claude for anything other than readily disposable/rote/boilerplate code i don't have to think about, and won't require a lot of testing, or for things that I simply cannot do at all otherwise. Like when I didn't understand the ASU direct DFA construction algo in the dragon book. Claude got me there, but only because (a) time wasn't an issue (b) i knew enough about the subject to catch it when it was bullshitting me.

Caveat emptor.

honey the codewitch

Speaking of which, I need to cancel my Anthropic subscription. I haven't used it for weeks. It was useful at first, but once i solved what i needed to i was done.

Richard Deeming

What type is the _displayedProjects field? If it's a List<T>, or can be changed to one, one immediately obvious improvement would be to replace the .OrderBy(...).ToList() with a .Sort(...) using an appropriate comparison function:

_displayedProjects.Sort(_sortAscending
    ? (x, y) => string.Compare(x.Name, y.Name)
    : (x, y) => string.Compare(y.Name, x.Name));

Sure, it would get a little more complicated if you needed to sort by multiple fields. But it would avoid reallocating the same list over and over.

As far as AI goes, when you've lost Eric Lippert, you're obviously doing something wrong! 🤣

https://bsky.app/profile/ericlippert.com/post/3lyv76akqw22f
Since I've started using VS again, ignoring the bad suggestions being constantly made has unfortunately become second nature.

When I was in devdiv my team worked hard to make systems that did NOT suggest that I introduce subtle bugs a dozen times an hour.

Please do better.

PIEBALDconsult

In my opinion...

Use of .ToList and .ToArray are rookie mistakes.

Graeme_Grant

The guys over at VisualStudioCode just dropped an update video on Spec Kit: More Spec Kit for Spec-Driven Development - focusing on prompt engineering.

honey the codewitch

I refuse to call interacting with an LLM "engineering"

That used to mean something.

Sander Rossel

As a programmer who didn't write either pieces of code* I feel it is my sacred duty to say... Both pieces of code suck, I'd do it better!

Can't call yourself a programmer if you don't thrash other people's code :)

On a more serious note though, I do think your code is more clear (although the check for IsDisposed seems like overkill, how is this code going to run if the underlaying view is, well, not in view anymore?), but a lot comes down to personal preference as well. Using a ternary operator is basically just a single-line if-statement, so you're still using if's, in a way. It's also not the kind of code that requires huge rewrites if you ever want to change it. At this level I wouldn't call it tech debt. So both are pretty okay I think.

Or even looked at them, which is optional for this most holy of programmer tasks.

realJSOP

I found that I have to specifically say something like "write the function as a const function", and "provide types for all variables, consts, parameters, and properties", when it should already assume that I want that when I tell it I'm using typescript.

Essentially, garbage in - garbage out...

PIEBALDconsult

It still does; which is why I never call myself one.

I have no patience for they who learn a bit of Python and think they suddenly qualify.