Revised attempt to audit code #425
Conversation
|
One thing I should apologize for: there was an automated change to get rid of "new " in favor of just "new"; the spacing rules in .editorconfig are vague on this. The changes to new() do make sense; otherwise, it's some extra cruft you probably didn't wanna sort through. |
|
Thanks, I am going through them, but will probably take a while. Most are good, things I welcome but never gotten to due to the sheer amount of files. I also am not a big fan of inline LINQ, I mean the for via keywords. Sometimes I prefer the extensions, sometimes I rather have it with a normal for each / loop, as debugging LINQ is not easy. What also needs to be noted, is that LINQ is usually much worse performance as plain for loops. So I need to think about those changes a bit. |
|
Understood. I think we can revert some of the syntax changes via an
.editorconfig preference & a re-scan; I don't remember off-hand if there
was already one in place. There seems to be love-hate with LINQ:
anything I read on it sings its praises; but you're not the first one to
mention that it might have a significant performance issue.
Thanks for taking a look at this. Hope all is well.
Michael Adams
https://unquietwiki.com/ <http://www.unquietwiki.com/>
------ Original Message ------
From "Robin Krom" ***@***.***>
To "greenshot/greenshot" ***@***.***>
Cc "Michael Adams" ***@***.***>; "Author"
***@***.***>
Date 7/30/2022 12:40:38 PM
Subject Re: [greenshot/greenshot] Revised attempt to audit code (PR
#425)
Thanks, I am going through them, but will probably take a while.
I need to check every file, as I really prefer to see what the changes
are.
Most are good, things I welcome but never gotten to due to the sheer
amount of files.
But some got this "we just change everything because resharper or
whatever tool is used, says so" feeling.
I do not always like the ternary syntax, that is the ? : notation.
Because things do not get better readable, at least there are quite a
few cases that this suffers.
I also am not a big fan of inline LINQ, I mean the for via keywords.
Sometimes I prefer the extensions, sometimes I rather have it with a
normal for each / loop, as debugging LINQ is not easy. What also needs
to be noted, is that LINQ is usually much worse performance as plain
for loops. So I need to think about those changes a bit.
—
Reply to this email directly, view it on GitHub
<#425 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHV7P6WJYWOZE5K6AMQCQ3VWWALNANCNFSM53TT5LSQ>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
… |
Hey @Lakritzator : hope the past few months have found you well. So I finally found some time to look at this again; being less aggressive on changes & avoiding the runtime upgrade. I know back then you were worried about Office plugin support; I think that should work, since I didn't touch the Dispose patterns. One thing that came up in my testing: it looks like there are missing OAuth hooks / allowances for uploading to Google, Imgur, etc; maybe there's something I didn't set up beforehand, but thought I'd mention it. I also didn't mess with the InnoSetup on this; currently just running this as Debug build for now.