There was a code review submitted in a project branch. I had no reason to review it because I wasn’t assigned to the project, but I often like to be nosey and see what is going into the releases.
I noticed a few classic mistakes like server-only code placed in “common“ that gets installed to both client and server when deployed. There was also a caching problem where the first call will store the data in the cache, then a call from a different user will then go into the cache and grab the other user’s data! It’s the classic mistake of forgetting about how the app servers are shared between many users and many organisations.
For years I have flagged these problems up. Maybe we should have it as part of an induction process to go through common misunderstandings. Anyway, I send them on to some developers (Dean and Mark) that I have discussed issues with or joked about them in the past. Each developer then decided to also leave comments on the review even though they also weren’t assigned to it.
In a private chat, Dean said “why isn’t the lead developer spotting these mistakes and teaching them?“.
I assumed he might have actually then put an official complaint in, because Gary, the Senior Developer assigned to the project; then left an angry comment.
“why are you commenting on a project level change? I thought you were struggling for time and this is a project branch that I haven’t yet reviewed”
The thing is, he hadn’t said that to Dean and I, but to the other developer Mark. It’s like maybe they had some kind of previous arguments and now it’s flared up.
Mark rightly responded:
“You’d added comments. This isn’t the place to debate such things, please contact me by message if you have a problem”
I think it could be the case that Gary hadn’t fully reviewed it, just glanced at it, left some trivial comments, and meant to come back. In my opinion, if someone does the review for you, then doesn’t that save you a job? Many developers seem to hate code reviews since they would rather be writing code, not reading it. So really he should be grateful. But he has taken it like a personal attack like we didn’t trust Gary to point out these major flaws in the code.
It’s also beneficial to spot problems as early as possible. There’s nothing worse than aiming to get the project in a release, then when you want to merge it in, then the experts then look at the code and tell you that it has major flaws and cannot be released. At least we have flagged it early in the project and they have plenty of time to address it.
When working with Indian developers, their English skills can vary. You also need to be aware of certain words exclusive to Indian English; some of which I actually like. For example they have the word “prepone” which is the opposite of “postpone”, but in UK English, we don’t seem to have a single word for that.
Some phrases seem more like poor grammar. An example of that is “Can able” or “Can’t able” when we would say “I’m able/unable”.
“i think you can able to see the second image is it?”
“I can’t able to find any relationship between those two codes”
“still we can’t able to recreate the issue”
“For the same” is an interesting phrase because it just refers to something earlier in the sentence without having much meaning. It’s similar to when they say “do the needful” which just means “do whatever is required” but often doesn’t really add anything to the instruction; if they have requested something from you, then surely you will do it if you can.
There’s a few strange greetings like saying “good noon” which I’d assume is just a shortened version of “good afternoon” rather than being appropriate for a very specific time period. There’s a few people that have a strange greeting of “Ho!”
“Ho!! Is it please can you share those knowledge with me…”
To take time off, they like to “avail”. As a bonus, here’s a strange requests:
Morning Team, I have picked up fever and heavy cold. Availing AL today. Please conduct stand up and end call. Available over mobile for any urgent issues. Thanks and Regards, Jeeva
I’m glad you told me to end the call Jeeva, because I’d have stayed on it all day otherwise.
Indian Pull Requests
When it comes to the Code Review process aka Pull Requests (PRs), it can be hard to ask them why they are making certain changes. Sometimes asking questions can just lead to further confusion. Also, sometimes I’m sure some developers try to blag and hope you move on.
I was discussing this with a Lead Developer and he agreed that asking questions can either result in
Blagging
Revert the code and hope it works
Or you actually get a good answer. But then if it’s not clear why the code was written like that, then maybe it does need a code comment or some documentation so others don’t get confused in future.
Even though I often got frustrated with their comments, in recent times, a lot of them use AI like ChatGPT to rewrite their responses, or sometimes I get the impression they just put your question into the AI and hope it comes out with a good response. So instead of poorly written English, it’s all robotic and a blag of jargon. So you can’t win really.
Row
“Refresh on special while saving special note, row background, Radio button alignment based on include exclude”
Blagging with Words on PRs
I questioned their pointless try/catch blocks which were catching an exception then rethrowing the exact same type of exception.
“Yes, as I couldnt use the dll in the resourcepicker project, so we can thrown the exception and catched it in resourcepicker class”
And
“The resources can be used due to filecahe, but no changes can be saved, when service is down. The above message is already used in Picker solution.”
Then when their project was being merged into the main branch, another developer questioned the same code. This time they said:
“To restrict that, have drilled up the ux tree and displayed the error message.”
Observation
“Found an observation while testing 12602 in 9.3.6 branch”
what does that even mean? I assume “observation” means “bug” or “potential problem”.
Bad Refactoring
He refactors some existing codec but also changes the return type of the method, which means the caller’s logic will have to be changed so was causing cascading changes which weren’t really relevant to his main change. Also, the logic didn’t look equivalent so I wouldn’t call it refactoring:, more like introducing a bug. He then claims he hasn’t changed it…
Me: "is this equivalent? It was checking >1 not >=1" Them: "Actually, I haven't attempted to modify that as the logic written working as per acceptance criteria, and it already tested" Me: "I don't understand, this method has been changed in this PR" Them: "Just used expression for methods as commented by Andy. Apart from that i haven't changed any logic around that."
Down Merge
Vignesh Here after no comments fixed against assurance branch? Just need information about down merge
Andy: sorry I'm not sure what you mean?
Vignesh Two comments pending for our side... if any one raise PR I will raise PR also. Because of down merge... Incase only I will raise PR again do down merge that's why I am asking
IsMobileEnabled
IsMobileEnabled needs to return boolean value, so removed exception caused by null and also the GetResources during Trigger prompting needs to include Template also along with Protocols.
Didn’t Launch The Portal
me: “where is this used?”
developer: “This is used at TryLaunchPortal()…. At this point of time we never know the portal type to compare and verify the condition because the user didn’t launch any portal“
walkie talkie comms going on here
This reminds me of walkie-talkies, stating “over” so you know it’s the end of the message.
Roshni give line break after method over
Shoban Ok Roshni, Updated the changes
Shoban Completed with the Changes
Roshni give line break after method over not before the method over
Shoban Thanks Roshni, Got your point. Made Changes
Roshni and again please remove the empty line no 267
Shoban Code changes completed as mentioned
Welsh
PR: Updated the Walsh text
Description: Updated the resource file with Walesh text
Do you think the text is gonna be accurate if he can’t get the title correct in English? It should say “Welsh text” as in “the Welsh language”.
Customer
Merge from Curomer first branch to main
Accelerator Keys
To define an accelerator key (allows you to use Alt key to select it), you place an & character before the letter. So Export has E defined. Edit can’t use E because Export has taken it, so they have chosen D. Cancel seemed an odd choice of N.
SonarCloud is a static analysis tool. It runs on your codebase and points out where code can be written to “best practices”, possible bugs, and code coverage.
When my employer bought a SonarCloud licence, there was a large emphasis that they wanted existing errors fixed, and no errors in new code. I knew this would be a bad idea to emphasise because of the classic “you get what you measure”. If you tell people the goal is to not have errors, then they will do whatever they can to not have errors. The idea of a tool like SonarCloud is to improve code quality, but since the metric is a number, then the number becomes the goal rather than code quality. However, the aim should be to improve the quality of the code, optimised for readability, maintainability, scalability, and code coverage.
As more people began working with SonarCloud, we saw more changes with the title along the lines of “Fix Sonarcloud issues“.
Human Analysis
SonarCloud is just a static analysis tool, it can only spot problems and suggest pre-defined improvements. These improvements are not always in the best interest of the code base. You have to objectively ask yourself “Does the change I’m making make the code better or worse?” Before you can do that, you need to understand why Sonar thinks there is a problem with the code. Then you can decide if Sonar is right or not. Some of the rules are not applicable everywhere.
I have seen a number of changes where the code is actually made worse by the changes to satisfy Sonar.
Example 1: 7 Argument Limit
A constructor or method that contains loads of parameters/arguments can be a sign of bad design, and maybe some of the parameters can be grouped together inside an object. Once you reach 8 arguments, Sonar will flag this. A simple fix is just to create a class and throw in a couple of parameters in there. It then satisfies the rule but it doesn’t make logical sense unless the parameters are in fact related. Adding a new class when it is single use could just make the codebase more cluttered and seemingly more complicated. I would rather see a class with 8 values, than some complicated class with extra types in the way “Just because of Sonar”. Mark Seemann has a good blog post about Curbing code rot with thresholds.
Example 2: TryParse
Another example, I once wrote some code with the following:
var ok = int.TryParse(a, out var x) & int.TryParse(b, out var y);
//something with x and y
Sonar complained about the use of the bitwise & and it was confusing and suggested I use &&. However, if I did that then “y” wouldn’t always be defined because of the short-circuiting and the code wouldn’t compile. I was about to reject the Sonar issue as “Suggests code that doesn’t compile” and just keep my version.
Then I thought, “if Sonar cannot understand my code to make a decent suggestion, maybe other developers can’t either“. I was trying to be too clever with my code.
Instead I changed the code to:
var ok1 = int.TryParse(a, out var x);
var ok2 = int.TryParse(b, out var y);
var ok = ok1 && ok2;
//something with x and y
It wasn’t as terse as my original version, but it was certainly easier to read and understand, and Sonar didn’t have a problem with it any more either.
Example 3: Cyclomatic Complexity
When a method has loads of If statements, this creates loads of permutations that can be executed which means if you are aiming for true 100% test coverage, you have loads of tests to write. It can easily make the code hard to read and understand too. At a certain point, Sonar suggests breaking the methods into smaller methods. I have seen people take this extremely literally and you end up with a design that looks like
So there is no logical grouping for what goes in part 1 and part 2, it’s just that enough code gets placed in the first method then everything else in the second. Now the original single method is half the size with no logical reason other than to satisfy the Sonar rule.
Me (rhetorically) Are these fields logically grouped or is this just to satisfy sonar?
Brijesh It is just to satisfy sonar
Example 4: Making The Code Worse
Nullable DateTime
DateTime always has to have a value. However, You can declare a nullable DateTime in C# by appending a question mark like DateTime?
The existing code was checking a standard DateTime for null, which can never happen.
if (startDate == null)
throw new ArgumentNullException("startDate");
The Code Analysis report was correctly flagging this code as completely unnecessary. Instead of removing the code, the developer then changed it to
if ((DateTime?)startDate == null)
throw new ArgumentNullException("startDate");
The method was still accepting the startDate as a non-nullable DateTime so could never be null. But then it was being cast to a nullable DateTime so the check against null is technically valid.
Me: Why are you casting to nullable datetime?
Chandeshwar dateTime is value type , it can never be null value. if check dateTime == null, it's always return false .
Me: Yes, that is correct Chandeshwar. That’s why you can delete the code completely. Your code is always false too.
Sometimes, this type of scenario leads to multiple attempts. Either
because they make some changes and they still get the same Sonar error,
they introduce a new problem,
or maybe the reviewer dislikes the changes and wants them to change it back.
So there’s plenty of files where our Change History looks like this
Fix Code Smell Issue <- doing what they think Sonar wanted them to do Additional Static Analysis Fixes <-they messed up, so tried again Addressing code review comments <- I pointed out it still wasn’t correct
Example: Magic Numbers
Magic numbers should not be used
A magic number is a number that comes out of nowhere, and is directly used in a statement. Magic numbers are often used, for instance to limit the number of iterations of a loop, to test the value of a property, etc.
Using magic numbers may seem obvious and straightforward when you’re writing a piece of code, but they are much less obvious and straightforward at debugging time
That is why magic numbers must be demystified by first being assigned to clearly named variables before being used.
-1, 0 and 1 are not considered magic numbers.
Vignesh changed code like
dosagesCount == 1
to
dosagesCount == Constants.Single
Me: Constants are for a different purpose! They are not for replacing all the numbers in the codebase. It's a good idea to give numbers clear names, and if the same value is used multiple times, it means if you need to change the number, it will update in all places.
Vignesh This is the rule we follow (quotes the magic number description shown above)... and I got comments from my senior level.
WHAT DOES IT SAY, VIGNESH? “-1, 0 and 1 are not considered magic numbers”
And you have replaced 1 with the word “SINGLE”
Example: Lack of pragmatism
It frustrates me so much that so many developers even don’t agree with Sonar, or don’t understand the change, but still attempt to change it anyway. Their sole goal is to remove the error that they lose sight of the true aim to write great code that works.
Dave OK, but why did it suggest it, and why does it make the code better? Did you understand the suggestion or just blindly do what it said?
Pavel I think I understood it. The comment was: "Add the default parameter value defined in the overridden method". Default arguments are determined by the static type of the object. If a default argument is different for a parameter in an overriding method, the value used in the call will be different when calls are made via the base or derived object, which may be contrary to developer expectations. That's why it suggested it. But in my opinion this change is redundant and doesn't make the code better.
There was some code which had loads of if statements. It was checking the types of “nodes” in a tree structure, and so attempted to cast the type. If successful, it would go into that code block, else it would attempt to cast to a different type.
Even though the developer didn’t need to change this code, he did change it to attempt to resolve Sonar issues with regards to the casting. However, he only updated some lines and not others which meant it was inconsistent so the codebase is more confusing, and still contains Sonar errors. He also took some lines out of the if statements and then performed all the casting at the top of the method. So now it was actually more inefficient because once you have found a match, you do not need to keep attempting to cast.
{
var creterionTag = row.Tag as LinkedCriterion;
var relationshipTag = row.Tag as Relationship;
var attributeTag = row.Tag as Attribute;
var resultSetRuleTag = row.Tag as ResultSetRule;
var conceptOrderingTag = row.Tag as ConceptOrdering;
Me why change to "as casts" for most, but then not for Table, Concept and SharingDisplayValue? I'm not even sure if there is an advantage to doing it this way. We now have several variables where only 1 will be set, and the rest are null. Might be better spending more time refactoring it out to get rid of the branching. Pattern matching is probably the neatest way for now. https://docs.microsoft.com/en-us/dotnet/csharp/pattern-matching
Kalya I just simply resolved some existing SonarQube issue which not raised because of our changes. It is a kind of help to resolving existing issues, It is very difficult to resolving all the issues as of now
So he tried to help, was too difficult, so gave up, but still decided to submit the changes for review, despite making the code worse.
Example: Just Suppress it!
Of course, you can still get rid of the error without actually fixing anything. But merely hide it from the managers that are only looking at the figures:
#pragma warning disable S1075 // URIs should not be hardcoded
public const string InfoUrl = " http://med.info";
#pragma warning restore S1075 // URIs should not be hardcoded
Conclusion
The goal isn’t to make Sonar happy, the goal is to write good clean code, sonar is a guide to help you do that, but it doesn’t guarantee success.
On a code review, a Senior Developer, Lee questioned why there was no database changes when the Developer Neil had removed all the related C# server code. Neil replied that he “wasn’t sure how the patching process worked” (despite being here years, and was in a team with experienced developers), and wasn’t sure if there were any backwards compatibility issues to consider.
So what was his plan? just hope it gets past the code review stage unchallenged? Then we would have some obsolete stored procedures, and unused data lingering in the database for years?
I initially thought his claim for backwards compatibility issues was nonsensical but from an architectural standpoint, it makes sense due to how it works in our system. The server code doesn’t call the other’s server; it goes direct. So that means if the old version calls the new version, then it would expect the stored procedures and data to exist. However, for this particular feature there were no cross-database calls at all.
I suppose being cautious and not deleting the data makes sense from a rollback point of view. It’s hard to restore the data if it is lost, but easy to restore the C# code. I have never seen us use this approach though.
The Senior Developer said :
This is very concerning to hear, can you please work with your team lead to understand how our versions are deployed, and if they are unable to answer all the questions, please reach out to someone. We do not support any version changes by default, though there are cases where we do have cross version server/database calls, but these are for specific cross organisation activities. You can safely remove these columns, update these stored procedures. There is no value in leaving something half in the system, if it is no longer needed, remove all references, database rows/columns/tables, class Properties, etc.
In my previous blog, I discussed Project vs Domain Teams. This is kinda linked in the sense that specialising in a certain area of the system means you gain knowledge of the functionality and architecture of that area. There would be less chance of this scenario happening where the developer is questioning if there could be backwards compatibility issues. However, he could have also found this information out by raising questions.
This example does cover many topics I have discussed on this blog:
Poor communication
Bad decisions
Funny quote from a senior developer ”This is very concerning to hear”
We use a software library from Atalasoft in our product to allow users to add annotations to PDFs.
One of our Indian developers posted on Slack to ask a question about a bug he was assigned to fix. It was quite hard to understand what he wanted but it sounded like the quality of the users PDFs were lowered to a point that they were blurry and unusable.
Hi Everyone, Here is my doubt was more of a generic one. In Document Attachment Module, I’m trying to attach a PDF. The attached PDF gets depreciated in the doc viewer.. After analysis came to a conclusion that, the Atalasoft Viewer we are using Document Attachment viewer should pass only with 96dpi(dots per inch).
However in the Atalasoft Documentation itself was given that inorder to increase the quality of the document inside the Document Viewer of Atalasoft we need to pass on the default or hardcoded resolution as attached.
With respect to this have attempting a bug in which need to fix this depreciation not in a hardcoded format.
Is there any way to calculate a PDF file’s DPI through its file size. (Note: Since PDF file was vector based and doesn’t posses any information related to dpi).Can anyone please guide me on this ? Apart from hardcoding and passing on a resolution value.
After struggling with it, another developer started working on it, but then went on annual leave so yet another developer took over. None of them had put much thought into what they were doing because when I asked them to explain the code, they couldn’t seem to. I then googled the code and found it on the Atalasoft website. https://www.atalasoft.com/kb2/KB/50067/HOWTO-Safely-Change-Set-Resolution-of-PdfDecoder
using (var annotateViewer = new AnnotateViewer())
{
annotateViewer.DataImporters.Add(new Atalasoft.Annotate.Importers.PdfAnnotationDataImporter { SkipUnknownAnnotationTypes = false });
using (var pdfDec = new PdfDecoder())
{
pdfDec.RenderSettings = new RenderSettings { AnnotationSettings = AnnotationRenderSettings.RenderAll };
Atalasoft.Imaging.Codec.RegisteredDecoders.Decoders.Add(pdfDec);
SetPdfDecoderResolution();
}
annotateViewer.Open(filePath);
var printer = new Printer(annotateViewer);
printer.Print(printerSettings, documentName, printContext);
}
static readonly object pdfLock = new object();
private static void SetPdfDecoderResolution()
{
int standardResolution = 300;
lock (pdfLock)
{
foreach (Atalasoft.Imaging.Codec.ImageDecoder rawDecoder in Atalasoft.Imaging.Codec.RegisteredDecoders.Decoders)
{
if (rawDecoder is PdfDecoder)
{
//By default PdfDecoder sets to lower resolution of 96 dpi
//Reason for PDF depreciation
((PdfDecoder)rawDecoder).Resolution = standardResolution;
return;
}
}
Atalasoft.Imaging.Codec.RegisteredDecoders.Decoders.Add(new PdfDecoder() { Resolution = standardResolution });
}
}
The code instantly stood out for being convoluted because we are creating a PdfDecoder called pdfDec, then instead of just setting properties on it, we add it to the RegisteredDecoders, then call our SetPdfDecoderResolution which loops through the decoders to find the one we added. If it can’t find it (which surely is impossible) it will add one.
I was talking to a Lead Developer about a completely different bug fix, and he says
“People just don’t think about what they write”
Lead Developer
So I decided to bring up this Atalasoft problem…
He said
When I saw the lock I wanted to ask, “Which Stack Overflow post did you find this on?”
Lead Developer
So I told him they got it from the Atalasoft website!
So they had blindly pasted this Atalasoft code in without thinking. They could just set the Resolution property in the existing code since we already create the object; so already hold a reference to it. If this code can mean we can add multiple decoders (which you aren’t supposed to do), then we could create a method similar to SetPdfDecoderResolution where it checks if there is a decoder or add one if none exists. Then we ensure all the correct properties are set.
They need to think
Lead Developer
I think the problem the Lead Developer had with the lock is that you use lock when you want to guarantee that only one thread is accessing a resource/section of code at any time; but this code wasn’t used in a multi-threaded context. So by blindly pasting in code without thinking, they were adding redundant lines and creating confusion.
The actual fix was just
private const int highDotsPerInch = 300;
pdfDec.Resolution = highDotsPerInch;
But to reach this outcome, it took 3 developers to look at it, then 2 to review it.
We have an area of our system that is a major pain for memory usage. We allow users to create what we can generically refer to as “Resources” and new users will then download the entire set of them, which are then cached locally.
The initial download is very large, then they are loaded into memory the next time the application loads. Most of the time, it is on-demand, but it can be slow and be very memory consuming.
Another problem is that due to various bugs, sometimes these resources can be missing/corrupted and have to be downloaded again.
Over time, the area of code has been cobbled together by developers that don’t really understand the systemand so has perpetuated the inefficiency and bugs, which then becomes an endless cycle of making the system worse.
There was a big push to improve this area of the system, but no one has learned their lesson, so many juniors got assigned to fix the problem.
When it comes to code reviews, code can be surprising, and the surprise either comes from the fact that the developer is significantly smarter than me, or maybe significantly dumber. So sometimes I find myself looking at it and wonder if it really is bonkers, or some genius understanding of code that I need to learn. So sometimes it’s best to ask your colleagues to check your understanding.
I don’t remember seeing a cast in a property before:
public IEnumerable<MissingResource> MissingResources
{
get { return _missingResources; }
private set { _missingResources = (List<MissingResource>)value; }
}
So it’s either incredibly smart, or incredibly dumb.
“That cast is mental!
You could set it to anything that implements IEnumerable<MissingResource> – but it better be a List<>
Dave (translation of what the code is saying)
Is the following just a lack of trust that .Net won’t clear the old objects? To me, this code makes it seem like there is a bug which they are working around, or are just going wild nulling everything out to save memory.
public void ClearData()
{
NewResources = null;
ExistingResources = null;
MissingResources = null;
SkippedResources = null;
NewResources = new List<IResource>();
ExistingResources = new List<IResource>();
MissingResources = new List<MissingResource>();
SkippedResources = new List<IResource>();
IndexedResources = new List<Guid>();
}
trust understanding
Dave
Does that finally block do anything? it’s a local variable so should be marked for garbage collector at that point anyway
finally
{
bulkResources = BulkResource.Empty();
}
Yes it does something.
That something is worse than doing nothing!!!!
the finally allocates another instance and loses scope of the current one, meaning there are 2 things to GC now
Dave
I do wonder if sometimes they don’t really know what you are asking but just change stuff anyway. So after I point out their use of null didn’t do anything good, we now create some empty lists and clear them if they are not null. (which they aren’t null, and are definitely empty because we just created them).
public virtual bool PrepareItemsForImport(ImportProcessParameters parameters)
{
DialogService.SetProgressFormText("Preparing to import...");
_newResources = new List<IResource>();
_existingResources = new List<IResource>();
_missingResources = new List<MissingResource>();
_skippedResources = new List<IResource>();
_indexedResources = new List<Guid>();
ClearData();
_importStartDateTime = DateTime.Now;
_mappingInformation = RemappingService.MappingIdentifiersForOrganisation;
return true;
}
public void ClearData()
{
NewResources?.Clear();
ExistingResources?.Clear();
MissingResources?.Clear();
SkippedResources?.Clear();
IndexedResources?.Clear();
}
Does “ClearDataInViewModel” do anything? You call it right before the view model goes out of scope and is eligible for garbage collection anyway?
Me
using (var dialogService = new DialogService())
{
var viewModel = new ImportDetailsDialogViewModel(dialogService);
viewModel.InitializeFromImportProvider(importProvider);
var dialog = new ImportDetailsDialog();
dialog.DataContext = viewModel;
Application.ShowModal(dialog);
viewModel.ClearDataInViewModel();
}
Remember what the point of this work was. It was to reduce memory leaks, and also improve performance in other ways (fixing bugs in the caching, reduce server calls, remove redundant code). What they have done so far is to add more redundant code and show a complete lack of understanding how/when the garbage collector in C# works and runs. The garbage collector is the way that memory (RAM) is freed up.
Me Retrieval.GetMobileTemplateHeaders doesn't seem to return TemplateHeaders anymore
Jaz Fixed this
Me You are still taking the output from a method called GetMobileTemplateHeaders and converting them to TemplateHeaders. Seems like the method should be renamed, or the return type changed
Jaz It is returning template headers enabled for mobile. So it was named as GetMobileTemplateHeaders.
Me This was the code before. It's of type TemplateHeaders List<TemplateHeader> headers = Retrieval.GetMobileTemplateHeaders().ToList();
This is the code now IEnumerable<UserTemplateDefinition> mobileUserTemplateDefinitions = Retrieval.GetMobileTemplateHeaders(); It isn't of type TemplateHeaders but you want TemplateHeaders. So you then take the output of Retrieval.GetMobileTemplateHeaders and convert it to TemplateHeaders, storing it in a variable called mobileTemplateHeaders.
The code looks strange to have a call to GetMobileTemplateHeaders then the line straight after it creates a variable called mobileTemplateHeaders.
Surely we expect the code to be more like IEnumerable<TemplateHeader> mobileTemplateHeaders = Retrieval.GetMobileTemplateHeaders();?
Jaz Change done.
Another developer pointed out they had introduced another inefficiency by grabbing ALL resources and not just the ones they were interested in. So they aimed to cut down memory usage but actually managed to increase it!
Gary Are you sure you want to do a get bulk resources to only just get the templates out?
You are getting all types of resources ~20k+ items etc to only throw the majority of that data away?
Jaz Checked with the team and changed the approach to get templates only
Conclusion
It is very easy to understand why this particular area of the system is a massive problem area. If you tell the developers to look into improving performance, they just end up changing random bits of code and hope it somehow works. Then when it is a half-decent change, they won’t put much thought into the naming, so then it’s hard and confusing to read.
What we need to do is actually assign some smarter developers to the project; ones that understand how memory leaks can occur, look at the number of resources being loaded at certain points, and analyse the SQL queries to do the initial retrieval.
A few years ago, I was talking to my manager about how we have a lack of people that like to perform Code Reviews, and it needed to change. He said since I had gained a reputation of being a good code reviewer, maybe I should do a presentation. I said it probably wasn’t that easy to come up with something generic and it needs a lot of thought. Maybe I would attempt to write a guide one day. Over the years, I have bookmarked a few blogs, collated some notes and ideas; but never put them together. So here is my attempt.
What Does Code Review Mean?
When a software developer has code they think is ready for Testing, they need to commit these changes to the main branch. Before this happens, there is often an approval process by their peers. This is the Code Review. The Github terminology is Pull Request. I’ve always found that to be a strange name.
Andy: pull requests?! since when do we use that term? Me: when nerds go dating
I suppose you could say the aim is to attempt to perfect the code. But “Perfect” is not defined; does perfect code even exist? The goal is to always write better code.
There’s the 80/20 rule of Project Management which states that 80% of the feature can be written in 20% of the time; but then to perfect it, it then takes the remaining 80% of the time. Sometimes you have to be pragmatic between programmer effort and actually delivering features on time.
Code Reviews are a great opportunity to learn either as a reviewer or the reviewee. Receiving questions, suggestions, or instructions from your team members help you learn about the programming language, design patterns, common practices, alternate ways of approaching problems, or learn about the actual domain. As the reviewer, you are practising critical thinking and essentially debugging in your head which is a skill you will be doing every single day; since more time is spent reading code than writing code.
We all benefit from an extra pair of eyes. Sometimes you think you have written some great code, and keep reading over it and think it is fine. Then another team member glances at it and spots improvements which you agree on. I’ve seen plenty of smart developers write poor code which I know they would flag up if their colleagues wrote it.
Looking at Code Reviews helps team members gain context on other pieces of the code base. Having more awareness of what code is changing gives you a headstart in fixing future bugs when you know exactly where to investigate.
Code Reviews take time, and time is money. Formal reviews are costly; that cost has to be justified by the nature of the software project. Delaying new features and bug fixes can have an impact on users, but also other team members due to higher code churn causes merge issues when it is checked in. The longer code changes stay in a branch, the more likely you run into these merge issues.
Readable code
I have heard some developers say “reading code is way more difficult for me (than writing)”. Given that programming actually involves more reading than writing, then I think developers that struggle to read code need to be questioned!
Since code is read many more times than it is written, you need to write code that is easily read by humans. If we were writing code just for the computer, we’d be writing in binary. Programming languages are meant to communicate to humans first, then computers after that. Although you don’t want to be too verbose, if you are optimising for the fewest lines of code; you are probably optimising the wrong thing.
Tone of your comments
The art in reviewing is to flag all possible problems, but you need to be careful how you word comments because finding problems in someone’s code can be seen as a personal attack, and people can feel like you are questioning their coding abilities. Reviewers can easily come across as condescending and demoralising their team members. The aim of the review is to improve the code, and grow your team member’s skills. It’s not about the reviewer’s ego or a chance to show off.
However, developers should take pride in their work, so attempting to submit some code that took no thought probably deserves a wake-up call. The severity of bad code can differ between systems. Does the software involve the potential loss of life (e.g. a medical device, vehicle safety system) or the handling of millions pounds of assets? Or is it a simple command-line utility that your colleagues use? In these severe situations, I can understand how reviewers lose their patience and write brutal comments. See Brutal PR Comments section.
The reviewer often holds a senior position, but could also be on the same level as the author. Regardless of any power dynamics, you need to bear in mind the author may have way more context and be involved in other meetings about the work; leading them to write the code in the way they did. Given that possibility, it’s better to phrase comments as questions rather than stating something with authority. Instead of “You should do it Y way” you could say “Can you talk about your reasons for choosing X? In most cases I’d use Y, is there a reason not to here?“. This approach comes across as more collaborative and friendly. In the case your suggestion was correct, they should realise when they try to answer. In the case that you are wrong, they can explain the additional information they have; and so all participants can learn.
You don’t always have to be negative as well. You can even add comments to add praise, or to admit that you learned something by reading their code. A bit of positivity helps negate any negative tone you had in previous comments.
Reviewers are empathetic that the recent joiner might not be aware of all the coding guidelines especially ones that are informal ones (not written down anywhere). Reviewers are also well aware that a new joiner is still ramping up with the codebase, and won’t be up-to-date with conventions and functionality.
Where To Start:
Sometimes I just dive straight into the code and start reading. Sometimes this can give you the best judgement to whether the code is clear or not. If it isn’t immediately obvious, or the code looks/”feels” strange, then I will try to gain more context. I read the Title, I read the description provided. I look at the linked Bug/Requirement and read the information that the developer had, and understand the problem they were trying to solve.
To get an overview of the change, you can glance at which files were changed/added/deleted. Sometimes reading the folder names gives you more context. Are you looking at client or server code? Are there database changes? etc. Do the files match up with what you expect; maybe they added a file by mistake. Maybe they added some temporary “hack” they made to test their code and haven’t deleted it.
Read In More Detail
Two questions to keep in mind are
• How could I improve that?
• What could go wrong?
You can review it by two approaches: readability and functionality.
If you can’t understand that code now, in one year you won’t get it either. If code is hard to understand, then it is harder to change, and more error prone. An easy thing to look for are typos, unclear names (“Variables with intent”), ambiguous names, wrongly named code.
Small functions are easy to read, are less likely to need code comments, and also easy to test. You can look for large functions and consider if they can be broken down.
If there are code comments, are they needed? Do they add value? Comments are good to explain hard-to-read code, or weird design decisions the author made because you can’t think of a better solution. Maybe there is a way to make the code more readable without the comments.
Does the code conform to your “coding standards”. An example can be casing eg:
// camelCase
// PascalCase
// snake_case
// kebab-case
Your team may have other rules about styling such as:
“returning early from methods”,
using certain syntax,
Keep argument list small.
However, if a given piece of syntax should never show up in your codebase, you should really add an automatic “linter” rule that will either flag it, or automatically fix it. It’s a waste of time to make this a manual process and it doesn’t provide a ton of value. You could say “if it’s not worth it to add the rule; then it’s probably not worth it to highlight it in the code review either”. Not all things can be linted though such as coming up with good names for variables/methods/classes.
Sometimes, you may have a recommendation that should not prevent the code from moving forward, but you want to note anyway. Marking these things with a prefix such as “NB” or non-blocking can be a great way to flag a small annoyance that the author can ignore if they don’t want to fix now. You might do this if you don’t feel too strongly about the issue, or think it’s not worth holding up the change. You always need to remember to be pragmatic.
A little code review habit I appreciate:
When my teammates make a minor suggestion, they often prefix it with "Nit:"
It's a short, polite way to say "I know this is minor, but I suggest changing this."
Useful for misspellings, typos, naming suggestions, etc
The functionality approach would be considering code for how it meets the requirements, as well as “non-functional” requirements like scalability and security. Is there any code that is redundant or duplicated? Are there any obvious bugs like the classic Null Reference or Index out of bounds? You could also ask yourself “How would I have done it? Is it my way better? Could that logic improve the current one?”
Has the person added any Unit Tests, and if not, can they? If tests have been deleted, is this the correct thing to do?
Does this change impact another system?
Are errors handled correctly?
Is the functionality over-engineered? Are there new third-party dependencies that are unnecessary?
Are they using a design pattern incorrectly?
Does the feature cause problems as the number of users scales? It might work on their machine, but will it work in live?
What should I do when I don’t know the language of the code?
There can be scenarios where you don’t know about a certain coding language or technology, but you have to review it. You can make it clear that you have limited knowledge before making comments. If there is no automatic linting on the code, a good starting point is the superficial review: look for typos and well-name of variables. Try to ask questions so you can learn, and also check their understanding. Sometimes asking questions gets them thinking and they find flaws in their own code.
A related point to make here, is that if someone is writing in a language they aren’t fluent in, they can write against the convention. We had a developer writing C# but was fluent in C++, so we often see him write If statements backwards like: “if (false == value)” which is a c++ naming convention.
“If you’ve ever seen Java code written in a C++ style or vice versa, you’ll know what I mean. I’ve previously referred to this in terms of speaking a language with an accent – you can speak C# with a Java accent just as you can speak French with an English accent. Neither is pleasant.” – Jon Skeet
Approve/With Suggestions/Reject
Once you have written your comments, you can set an overall status of the review. The terms differ depending on the system (ADO/GitHub etc) but it generally follows Approve/With Suggestions/Reject.
It’s possible to select an overall status that doesn’t match your comments.
Andy rejected the pull request The change is implemented perfectly, I’m just thinking we could alter the design slightly to provide better flexibility.
One developer explained his process of what he selects. So he can Approve, but also leave comments. But that is a different message from when he uses the “With Suggestions” and leaves comments.
The way I do code reviews is as follows · Just comments – I’m starting a conversation on the change, and will Finish it later, usually I am expecting you to reply, so feel free to reply to my comments. I usually choose a “finish reason” after discussion. · “Looks Good” – just check it in. · “Looks Good” + Comments – just check it in but I had something to say. · “With Comments” + Comments – there are minor things, style/formatting that I'd like changing, please make where appropriate (I can be wrong) and check in. I don't need another review. · “With comments” + No comments – I am agreeing with someone else’s comments, or if I was first, I probably clicked the wrong button - check with me for clarification. · “Needs work” + Comments – Please make the suggested changes and submit a new Code Review. · “Needs work” + No comments - Agreeing with someone else’s comments, or if I was first, I probably clicked the wrong button - check with me for clarification.
Brutal PR Comments
John If you want something done, do it yourself. Yesterday at 10:06
John Well, this shows that you did not even attempt to OPEN the DB Utility Tool, let alone test via the DB Utility Tool. It would crash opening this. Line 351 | 5 hours ago
John I have not reviewed any of the C# code, I expect it to be as depressing as the DB code though. 5 hours ago
John What the hell is this doing in here? Also why have you stopped inserting the ONE important thing from this trigger - the change to the organisation! Line 101 | 5 hours ago
When To Do A Project Review
When it comes to merging an entire project, this can consist of hundreds of files. Despite being reviewed by team members, the final merge will be reviewed by an Expert. We have tried to get Experts involved early in the projects but since it can take a long time and the deadline can be far away, they aren’t inclined to do it. Then when you want to release it in the next few weeks, they will review it and dump loads of comments on it, blocking the merge.
“This is taking a long time and there are quite a few problems with it, nothing that can’t be fixed in a week or so, but some things that should have been flagged up by someone / thing before it gets to me. This process has to change.” – Expert
You probably need to ensure that each team has an Expert Reviewer in the team, so that quality reviews are done throughout the project. We often didn’t have teams structured in this way.
“they need to stop having teams that don’t have at least one person who knows what they’re doing” – Dan
One of my colleagues wrote the following about this issue. He often gets blamed for holding up projects when he is being asked to review with limited time. Any feedback he gives then blocks the project, and if he doesn’t review in time, then he also blocks the project:
Mike’s Pull Request (PR) ideas
For the most part we are doing a good job with pull requests, but occasionally I feel we can do better. I’ve thought of some useful guidelines, that will ensure quality, again most people are following these, so great job, but please keep them in mind PR Guidelines
Your team should be self-sustaining:
As a developer you should always be able to send your PR to someone in your team for a thorough review.
If you’re regularly having to pull in external resource to review your code, you should make your team leader/scrum-master aware, so they can discuss this with resource managers.
Code should always be reviewed internally before someone external to the team is added to the review, this ensures that the external reviewer only sees code which has survived the initial review pass.
If external expertise is required:
Let your team leader/scrum-master know that expertise is required, identify the person with expertise and contact them to let them know you will require their time, preferably a sprint in advance, so they can discuss with their team and prioritise time in the next sprint.
Your PR is not “urgent” unless its SLA is at risk of expiring.
You are not to refer to external reviewers as “a blocker”. If external expertise is required, then it is an essential part of the development process, and they are only seen as blockers due to poor planning.
Draft PRs are not very useful to external reviewers, since you can only comment on them: not approve, but they’re great for sharing day-to-day progress updates between remote developers.
They should be used to update your team’s senior developers and technical architects on your progress, and receive feedback.
I would say in a well-oiled team, that developers should share code each day, by some mechanism that makes it visible to their seniors for feedback, this ensures valuable short feedback-cycles and is the most cost-effective way of ensuring quality during development
Respect the reviewer
I think a key takeaway from this idea is that you need to respect the reviewer. They are kindly dedicating their time. You also need to understand that the review process is there to improve code quality and not just a box-ticking exercise.
I find that sometimes people create a review, then instantly message you about it – even though you are often notified through alerts you have set up, or will check your review list when you have free time. Being nagged is not nice.
There have been times where I have submitted comments then am messaged a few minutes later asking to re-review. If you ask that quickly, then I know that you didn’t even build your changes never-mind test them to see if they work. Should I really approve something you took no care with? (Maybe a 100% unit tested solution would mean that this is possible though).
We also usually have a rule that 2 people need to review, so even if I approve it; then it still cannot go in, so I hate being nagged to approve when there is time. Sometimes code needs more thought to consider if there’s more aspects to it than initially thought. A rushed review isn’t a quality review.
Making statements like “please approve this, it needs to be released tomorrow” isn’t good for the reviewer. I want to be able to review it properly, leave comments as I wish, and even Reject it if I really don’t think it will work.
Conclusion
If you see reviews as just a box-ticking exercise, then it defies the whole point of the review. It really needs buy-in throughout the team. If you want quality and continuous improvement, then support the review process. If you want speed, then it can be sacrificed at the expense of quality, and other benefits.
The code review process has a wide range of benefits and outcomes: teams see improved code quality, increased knowledge transfer within and across teams, more significant opportunities for collaboration and mentorship, and improved solutions to problems.
As a developer, you need to “commit” your work into source control. When you do this, you need to provide a message that describes the change. A bug fix or enhancement can consist of a few commits. When you use a source control like Git, you can even “rewrite” the “history” by using squash/rebase commands.
When I am writing code for my job, I often put a bit of thought into the wording to make it clear and professional. If it is for a personal side-project, I sometimes write meaningful messages, but sometimes I may group together multiple fixes, then come up with something generic like “Bug fixes”, or might commit more experimental work-in-progress under a name like “attempt” or “x feature part 1”.
It’s quite frustrating though to see colleagues write generic messages like “bug fix” which doesn’t describe what it is fixing, or how it is fixing it. Seeing messages littered with spelling mistakes is also annoying and unprofessional.
Examples include:
“EventNotificationBugFix After Resolving James’ Comment”
“bug resolved”
“Dev test changes from tester” (literally what does that mean?)
Updated the findCorrectCodeSp to findCurrectCode.
Taken off completly the fix Amanda done for the fix for 25477 and fixed. Included the fix for 8017 as well
Fix for SQLCrop issues (should be SQL Cop, our Code Analysis)
Fioxed further typos (ironically creating more typos when fixing typos)
fixed the failed unit testes (testes rather than tests. Brilliant)
“Post Christ’ comments re coding standards” (it’s the second coming of Christ to teach us coding standards! They meant to write Chris.)
There was a guy who worked in our short-lived Scotland office who sounded like an absolute nutcase and I have never seen someone not take their job seriously like this:
instructions unclear, got dick stuck in tfs
what a waste of eyes
but fuck wasps
be nice to bees
what if there were landmines in dinosaur times
A colleague recently showed me this website https://whatthecommit.com/. I don’t know if they are based on real messages, but it shows you a new message every time you refresh. Most are pretty basic along the lines of “does this work”, but there’s some more outlandish ones if you persevere and refresh many times.
One of my team members recently submitted a change that was labelled “Crash when cancelling out of the dialog”. That describes the bug that he fixed, rather than what he fixed. Another team member provided the following good advice:
The change looks good, but your commit message isn’t describing the change. Could you just reword it so that it completes the sentence “When applied, this commit will…” please. Another way of looking at it is that every commit should read like an instruction in a to-do list. I’d use something like “Return empty collection if user cancels out of the dialog”.
One of our Principal Developers loves linking people to this guide https://cbea.ms/git-commit/. One interesting idea is to “Use the imperative mood in the subject line”, whereas most people would write in the past tense.
Having a clear history of file changes helps you when it comes to finding which change caused an issue, and also gives you a better understanding why it changed.
Recently, a developer had fixed a bug with title similar to “Tax Number is not populating in Save Registration dialog”
The original code looked like this:
public CarIdentification CarIdentification1
{
get { return _carIdentification1; }
}
public CarIdentification CarIdentification2
{
get { return _carIdentification2; }
}
if (!string.IsNullOrEmpty(transaction.TaxNumber))
{
_carIdentification1.Value = transaction.TaxNumber;
}
if (!string.IsNullOrEmpty(transaction.RegistrationNumber))
{
_carIdentification1.Value = transaction.RegistrationNumber;
}
if (!string.IsNullOrEmpty(transaction.NewRegistrationNumber))
{
_carIdentification1.Value = transaction.NewRegistrationNumber;
}
if (!string.IsNullOrEmpty(transaction.NewTaxNumber))
{
_carIdentification2.Value = transaction.NewTaxNumber;
}
So a quick glance at the code and you see that 3 of the 4 statements are using “_carIdentification1” then the last one sets _carIdentification2. So without putting much thought into this, I would expect one of the first 3 should actually be using _carIdentification2. But what does 1 or 2 actually mean? Are they representing TaxNumber and RegistrationNumber, or is it representing Old and New?
If it is the former, I think _carIdentification2 would represent TaxNumber so the first one was wrong.
If the latter, then I think _carIdentification2 would represent New so the third one is wrong.
I do know that the concept of TaxNumber was added into the system later on, so most likely it would be: “_carIdentification2 would represent TaxNumber”
However, the developer, Govind had changed it to the following:
public CarIdentification CarIdentification1
{
get { return _taxNumber; }
}
public CarIdentification CarIdentification2
{
get { return _registrationNumber; }
}
if (!string.IsNullOrEmpty(transaction.TaxNumber))
{
_taxNumber.Value = transaction.TaxNumber;
}
if (!string.IsNullOrEmpty(transaction.RegistrationNumber))
{
_registrationNumber.Value = transaction.RegistrationNumber;
}
if (!string.IsNullOrEmpty(transaction.NewRegistrationNumber))
{
_registrationNumber.Value = transaction.NewRegistrationNumber;
}
if (!string.IsNullOrEmpty(transaction.NewTaxNumber))
{
_taxNumber.Value = transaction.NewTaxNumber;
}
So they have named the fields _taxNumber and _registrationNumber which is much clearer. Notice though that they have deemed carIdentification1 to be _taxNumber which means they are saying the second, third, and fourth statements were wrong!
An additional thought: if a “transaction” comes into the system, what fields do you expect populated? if RegistrationNumber and NewRegistrationNumber are populated, then _registrationNumber will initially be set to RegistrationNumber, then instantly changed to NewRegistrationNumber! So I’d say this logic needs to be if/else so it only sets _registrationNumber once and _taxNumber once.
I point this out to the developer, but I find a large number of Indian developers just seem to respond with a sequence of meaningless words and you have no idea what they are thinking.
Me:
if NewTaxNumber and TaxNumber are specified, won't it just use NewTaxNumber?
Govind:
taxNumber identifier has to updated with new taxNumber value for transactions
Me:
if it is intentionally written this way, it should be if/else
and if it wasn't intended that way, then there is a bug here, similar to the one you are fixing
They ignored me and checked it in. I don’t know if just the original code was mad, or if this new code is mad too. Let’s hope it doesn’t cause more problems when it goes live.
One problem we often talk about at work, is that software projects are sent to be Code Reviewed a month before release, and they often get torn apart. This is because many people at the company don’t seem to like doing code reviews, or aren’t good at critiquing code.
As the project goes along, each check-in requires review but the review is done within the team. When the project is complete, and needs to be merged into the Release branch, the review is done by people outside the team (by people of higher skill, or by people who review under more scrutiny).
It’s still strange when the project is led by a senior, but is then torn apart at the final Code Review stage. Or in this case, that I am covering today – Gerald, a Principal Developer; someone who you would consider an “expert”. 114 comments were left on this particular project.
Some comments were coding standard violations (but given the Principal has several years experience working here, it was hard to believe he overlooked them), general bad formatting, code comments left in code, loads of mutable classes (bad design, but often very easy to fix), then a few classes spamming the error log (we have a separate problem which another development team are trying to fix – about monitoring storage too high, mainly down to excessive/unnecessary logging)
Problems
Excessive Logging
Me
Is this creating 6 log entries per call?
Gerald
Yes , maybe even 7, but that's if the config value is set to debug. We have turned off by default
Sean
How do you change the logging level? Do you need a new release to change it? What is your plan for changing it?
Me
shouldn't it just create 1 row with all the information in?
A single entry would be good, I point out it is excessively adding 6 rows per call, and he is like “no! you are wrong, it is worse than that!”
In another part of the code, Sean flags up the same problem. Weirdly, he then agrees it is a very bad thing to do, and his tone sounds like it is an obvious thing. Wasn’t he leading the team? why didn’t he notice the problem during the months he was working on it?
Sean
Won't this write a lot of entries to the Error Logs?
We are actively trying to stop that
Gerald
the audit logger can be turned off.
I agree here though we could not write to monitoring if there's no records
Sean
There's an ongoing project to stop that amount of unnecessary, unused and ignored things to the error logs.
Gerald
Agreed. especially from the client. because that clogs up networks - a lot worse than this.
Does the code work?
//Would this work? Or how do we get the organisation ID?
He left this code comment in the code he sent to review. If the comment is doubting that it works, what confidence do I have? Instead of writing any words, I send a simple emoji. 👀
Gerald
Not sure what this comment means, could you please elaborate
Sean
Might be the fact that there's a question in the code asking if it works or not.
What a cliff-hanger.... Well does it? 😄
Gerald
I'll remove the comment. Thanks for clarifying Sean.
Gerald
Removed the comment. I think the team was not aware that we can get the organisationid from the usercontext.
Why did it need clarifying? Did he even read the line I commented on? why do we need to explain that a comment like that shouldn’t be in code we send to production?
YAGNI
There’s a software development principal known as YAGNI: You Ain’t Gonna Need It. It’s about only writing features you know you need. If you write features that aren’t used, any code changes you make need to support this feature. It’s a maintainability problem. There were actually a few instances of this in his project, although it was a case of maintaining obsolete code.
Changing obsolete code just in case it is used, but yet it wasn’t tested because it wasn’t used…
Sean
This looks like dead code...
Gerald
Yes on the off chance it was used, we now redirect to the new stored procedure
Sean
How did you test it?
Gerald
We spoke to the other team and they said it wasn't used, so no testing required. I am sure.
Sean
So you leave it in case it is used, but don't test it because it isn't?
Gerald
Yes sort of. In case this was ever resurrected the change was there for them to use
Sean
It won't be
Not knowing which server runs his code
I also found it odd that he thought some of the code was run on the Scheduler, when in fact the Scheduler never has much logic; its role is to send “jobs” to the Application Servers to process them. Therefore, the code his team wrote is executed on the application server…
Sean
There must be a better way than making a Remoting Call to get this information!
Gerald
We're stuck here. The enablement is on the Application Server. The scheduler service is running this, and we don't want to make direct DB calls.
Sean
No it isn't. This code is running on an Application Server
I was playing around with it
case when (BeforeXml is null and datalength(BeforeXml) = 5)
This code is always false. It cannot be null (empty) and also be 5 characters.
Gerald : Good spot I will make this change now. I was playing around with it
The Major Incident
After the project went out, the release got “Red Flagged” which means we have to produce an urgent fix, and the release is halted from the rollout.
“Please can we red flag the current release. Unfortunately the database Schema Patch 8037 altering the Audit table filled up TempDB (60gb+) resulting in the database server Failing over.”
The change he made seemed very rushed. From the Principal Developer’s own words, he said that managers were demanding the change as quickly as possible. He should know better from his experience. Which is better: a delayed but correct fix, or a rushed broken one which would cause yet another red flag?
His new, rushed fix was inefficient, mainly from not understanding LINQ-to-SQL. There were also some database changes which the logic was wrong, and there was a typo in the SQL script which would have caused it to fail.
If it doesn’t work, it’s not gonna get past testing, and won’t release on time anyway, so what is the point rushing it?
Sean: This can't be very efficient…
Gerald: Might need an index on that, but it's linq to sql - a mind of its own. i can try profiling this, but seems pressured to get this in
Sean:
And if you slow down the saving of new records, you will find even more pressure to get a fix out
Also, Linq-SQL doen't have a mind of its own…
Additionally, a tester had a look at his changes and pointed out yet another error in his new database patch. If Gerald had run his changes through the database tool we have, it would have flagged the error. Absolutely embarrassing. But that’s what you get for rushing.
Stranger Coding Tings 