testfx icon indicating copy to clipboard operation
testfx copied to clipboard

[Style] Target-typed new in the codebase

Open Youssef1313 opened this issue 2 weeks ago • 7 comments

We have excessive use of target-typed new, which (IMO) makes the code much harder to read in many cases where the type isn't apparent.

I would consider switching the code style to even disallow its usage completely. It's a personal opinion though and there is no right/wrong answer here.

Youssef1313 avatar Dec 04 '25 09:12 Youssef1313

Sometimes I find it hard, sometimes I don't. thinking about it, it is maybe harder than it was with var before.

Additionally I would really like to see the use var when type is obvious relaxed. Because almost everywhere else tells me to not use var.

nohwnd avatar Dec 04 '25 13:12 nohwnd

Sometimes I find it hard, sometimes I don't. thinking about it, it is maybe harder than it was with var before.

It's not only about var, it's also about patterns like the following:

https://github.com/microsoft/testfx/blob/aaa16e9321c4344b439324378ddb23b60e1d3461/src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/ObjectModelConverters.cs#L74

which IMO should be really avoided.

Youssef1313 avatar Dec 04 '25 13:12 Youssef1313

I see. There I can still kind of guess what it is doing, but looking at the code without ide support or trying to quickly understand is definitely more difficult than if the whole type name was spelled out.

nohwnd avatar Dec 04 '25 13:12 nohwnd

Very much IMHO... i still usually prefer target typed new. yes in some cases it is less readable. but i think there are better ways to make things clearer.

eg in the above code example i would write

var position = new LinePosition(testCase.LineNumber, -1);
testNode.Properties.Add(
    new TestFileLocationProperty(testCase.CodeFilePath, lineSpan: new(position, position)));

note it also removes a duplicate LinePosition instance

SimonCropp avatar Dec 08 '25 08:12 SimonCropp

Only mentioned copilot to implement the suggestion provided by Simon, and nothing else. But it did not listen to me apparently.

nohwnd avatar Dec 09 '25 11:12 nohwnd

@nohwnd wouldnt it be better to have an overload of TestFileLocationProperty that accepts a single position?

SimonCropp avatar Dec 09 '25 11:12 SimonCropp

Looks fine to me as is, better to keep the public api smaller rather than specialize it for that case. We are also not getting the column, and get a magic -1 number instead, even thought the item is probably not placed on unknown column, and does not take 0 characters.

nohwnd avatar Dec 09 '25 16:12 nohwnd