Even as an experienced software developer, it is amazing when you discover some really trivial things, or discover some interesting quirk of a programming languages.
I was looking at a Code Review the other week and I saw some code that looked really pointless. It was testing some code throws a ArgumentNullException.
A NullReferenceException is an incredibly common mistake and probably the first problem new developers encounter. If you have a reference to an object, but the object is null, you cannot call instance methods on it.
Therefore if logger is null, then you cannot call LogWarning without an error being thrown.
So on first glance, this test looks like it is testing the basic fundamentals of the C# Programming language. However, this is testing for ArgumentNullException rather than NullReferenceException.
LogWarning was actually defined as an extension method, and this actually does allow you to call methods on null references. I’ve never realised this or even thought about it. It is the case because extension methods actually pass the reference in as a parameter.
So if you have an extension method (as indicated with the this keyword):
public static bool IsNull(this object x)
{
return x == null;
}
This can be called like this:
static void Main()
{
object y = null;
Console.WriteLine(y.IsNull());
y = new object();
Console.WriteLine(y.IsNull());
}
Which would output true, then false. Which illustrates that the extension method does not crash if the reference to y is null, and the logic correctly works by returning true when y is null.
Conclusion:
Understanding NullReferenceExceptions is basically day 1 of learning to code in an Object Oriented Language like C# but I’ve never even considered there is an exception to the rule. A method call on a null reference won’t cause a NullReferenceException if the method is an Extension method!
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.
As a software developer, you are always given projects without knowing the contractual details involved. However, there was one project that I was originally assigned to do, and was forwarded some documents about the project. In the document, there were some fairly formal documents which included some pricing.
The project was actually straightforward because we already had the functionality for users in England and they wanted users in Wales to use similar functionality. It was the same for the most part, but there was some minor customisation required. So it mainly involved deleting or tweaking a few files to remove the validation based on the country. Then there would be some testing involved to make sure the feature really did work when configured for Wales.
Some Senior Developers and Architects had estimated the project at 6 months which was a bit extreme, and reckoned the cost of development was £442,404, then some miscellaneous costs for “platform, network and migration” which would take the total to £445,620!
On the face of it, that sounds expensive. But when I think of the labour cost involved, where I work, a Junior might earn £25k a year, then Seniors are more like £32k-£45k. So if you have a few developers and testers on a project, with some managers involved, and it really does take 6 months, then the costs soon add up. Then you want to make a decent profit on it too.
I guess the cheeky thing is, the customer might not know what you already have; so you could charge as if it was new but you are just recycling/reusing existing code.
The end result is the same for the new customer isn’t it?
What I didn’t understand in the document is that there was a line that said:
“The requirements described within this CCN must be delivered by January 2024 in order to support a proof of concept with a limited number of users in a live environment. Once the proof of concept is complete, an implementation plan will be defined by the programme team to determine the pace of the national rollout, to be complete by January 2026.”
My question is, does it make sense to create a proof of concept (POC) that works well enough, but then have 2 years to actually complete the work?
Well people don’t have any experience of what they are suggesting so are just making it up. I agree though, if you have a proof of concept you’re kind of almost there. Depends on how hacky the POC is I suppose
Robert (Senior Developer)
Even more confusing is that we didn’t deliver the POC by January, but we did deliver the completed feature by the end of March.
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.
“Development environments” allow software engineers to view a particular version of the software and control who has access to it.
If you go back about 20 years ago, there was much less maturity with how changes were rolled out to customers, and every company had a different process. In some companies, developers would just immediately roll them out, straight into production. This might work for the most part if you have a very small and trusted team without the process or contractual requirements. These days, the standard is that most companies need a minimum of three separate environments when developing software.
Even where I work, people used to tell me it used to be much less professional, and without as much legal scrutiny – allowing them to drop DLLs onto a server to quickly fix a bug for a single customer. Now there’s all kinds of people who need to sign it off, and 99% of changes are released as an official version to multiple customers.
Development Environment
The main development environment is a place where an individual engineer can actually work on the software in isolation from any other stakeholder such as a Tester or Customer. Instead of making changes directly to a live website/application, the changes can be run on a local copy on the developer’s machine. The developer can begin looking at the impact of the changes he’s making, and will use test data. Any problem introduced only affects the single developer.
It could be as simple as a copy of a database and a version of the website. A more sophisticated program may have even more dependencies, so need multiple websites or services configured – so you could leverage modern technology like “cloud services” or “containers” to easily deploy this setup. The decision of how this looks depends on the complexity of the software architecture, team size and process.
Staging Environment
The “User acceptance” test environment, sometimes known as a “staging environment”, is another copy of the application, but developers aren’t making any changes to it, and it is also using test data. The purpose of this environment is typically to run exhaustive testing to find any issues before deploying the changes to the end users.
When companies rely on manual testing, and don’t have modern “continuous delivery” processes, an unreleased version may sit in the staging environment for an extended period of time. In a more automated environment, automated tests can be run, and the version easily deployed to customers if passed. This could take as little as fifteen minutes to an hour depending on the tests, software complexity, and process involved.
Production environment
The Production environment, or Live environment, is where your customers use your product; so the actual website or application.
Demo environment
You can have other environments like a Demo environment. This is another copy, but not used by Developers or Testers. The version can be “work in progress”, or a version that is deemed to be ready; but the purpose is to show customers upcoming features and to gather feedback.
Capacity testing
You could create a special kind of test environment for capacity/load testing. This is meant to stress test the software and see how it’s performing under heavy load. Systems can have increased traffic at specific times of the day, e.g. users all logging in at 9am to start the day, increase in website hits during lunch breaks, more shopping traffic during Christmas period etc. If users are paying to use your service and it goes down; it’s a huge problem. If you sell a product/service on your website and it goes down; then it can cause lost sales. So performing this testing can be essential.
One of the problems we have where I work – is not releasing fast enough. When you read about Software Development, you hear of these companies that can release minor updates every week. Maybe it is more of a Web Development thing rather than an Application Development one, but there are also contractual reasons why we cannot release faster.
However, over time, the release time has continuously crept up to 4-6 weeks which causes problems.
If there is more time for each release, it means that the scope of the current release often increases further. For example, if there is a fix that needs to go out within 3 weeks to meet the SLA (Service Level Agreement) and the next release will go out in 4 weeks, then you have little choice but to get it in the current release. If you are checking it in close to the deadline, then you might end up delaying the release in order to test it. The more you delay, the more chance of someone else needing to get a fix in the current release and it grows further.
If there’s many big projects targeting the same release, each team developers in their own code “branch”, then will merge into the Main branch for release. Since it’s not really feasible to all merge at the same time, you end up taking it in turns and resolving any conflicting changes. To be honest, it’s quite rare that we will change the same files for the main feature changes, but there’s certain files with a lot of churn, mainly ones containing XML. To merge in projects, it usually takes a few days, then all the bug fixes on top. The Testers can’t really begin testing until it’s all merged so it’s a lot of overhead to manage the release.
When the releases are large, the Testers insist on running more Regression Tests which increases the Testing phase and can cause further delays.
“I think we spent about 2 months on that regression. It was madness. It was a HUUUGE release”
Software Tester
So smaller releases are much more manageable, take much less time to test, incur less risk, and have lower scope for scope-creep.
Our Software Delivery team made an announcement about this (basically just saying the same things I have just discussed), and desire to plan in quarters but release in a couple of weeks.
In the past, we would scope a single release looking at the features, fixes and minor enhancements we wished to deploy. We would follow a process of merging everything into our main release branch before undertaking testing. This was a two-phased testing approach, integration/functional testing each feature and fix, and then regression testing to ensure pre-existing functions continued to work as expected. We would then spend eight to ten weeks deploying the release through Controlled Roll Out and Customer User Acceptance Testing.
This approach brought with it a number of challenges. Merging everything in was time consuming, issues or blockers with one feature would slow down or block other features, regression testing was a challenge, and this also put pressure on the roll out and deployment through pushing out a number of changes in one go.
To try and mitigate some of these challenges, we are now adopting a strategy of breaking these large releases down into smaller updates.
Working in quarterly cycles we will scope what we wish to deliver over a 12 week period. Each feature will be analysed and risk assessed for size and complexity by our Engineering Leads, and have a business value determined by our Product Management and Commercial Teams.
Using this feedback we will then determine an order in which we wish to deliver each feature. We will then merge them into a release and test them one at a time (potentially two if both are small and low risk), before signing over to Release Management to commence deployment.
We will then deploy the full scope over a series of smaller releases rather than in one large release.
The last update in the cycle will be a maintenance release to address the backlog of prioritised service work.
The objective behind this approach is to have our users benefit by taking elements of the release scope earlier than they would have before, whilst also simplifying the testing approach and hopefully enabling us to push code out across the estate quicker.
Last year, my employer paid for a “security audit” for our software, and any “issue” on their report was then a high priority to fix.
I think the majority of the issues they stated were incredibly unlikely to happen, or there would be an easier means of acquiring such information. One of them was that UDP messages, which are only sent on the user’s local network – had an unencrypted username. But if the attacker was already inside the user’s building, it would probably be much easier just to look at the user’s screen rather than plugging into their network and running a packet-sniffer.
Problem 1:
Shortly after we released some “security fixes”, we had a few bugs reported, one of which was:
“Users unable to reset their own passwords using the self-service password reset process”
So the security improvement created a Security major incident. Brilliant.
Problem 2:
A colleague was explaining another problem which I only had a vague understanding of. But it was to do with the version of a protocol being sent in the header of a message. It is classed as a security flaw because an attacker can look up known security flaws for that version and try to exploit the system that way. I suppose they can still guess what the version is, and try different “attack vectors”; but I suppose the less information you give them, then the more hassle it is. As my colleague explained it, a change had been made to remove the version, and tested on a specific version of SQL server, but we have multiple versions on our live infrastructure. When it came to demoing it with a client, they discovered a particular feature was broken on a different SQL Server version. A little awkward, but at least it didn’t go live.
Potential Problem 3:
When it comes to authentication failures, if you tell the user that the login attempt has failed due to the username being wrong, or the password being wrong – you are making it easier for malicious users to attempt to gain access to someone’s account. So if an attacker is guessing usernames and normally sees “Invalid username”, and eventually gets an “invalid password” message, then the attacker knows the account exists and now just needs to get the password.
There was one API call that returned an error code along with the message “Invalid username”. So as advised by the security audit, the developer changed the message to “Invalid username or password”.
On the code review, I pointed out that the developer also needs to change the Error Code too, and they replied saying the security audit stated that the message was the problem. They definitely hadn’t thought about the actual problem. If Error Code 40 is “Invalid username” and Error Code 41 is “Invalid password”, and you change both texts to say “Invalid username or password”, then it’s not really any more secure is it? since we are still returning 40 when the username is wrong, and 41 when the password is wrong. What we need to do is make 40 and 41 obsolete, and make a new Error Code for “Invalid username or password”. However, can we actually do that? When you have third parties using your API, if they have written code which relies on certain return values – then you break their software when you change yours. So we would need to inform everyone that the change is coming, then release this change.
UDP and ChatGPT
A senior developer questioned why we were encrypting elements of the UDP message. The developer struggled to explain and then incorrectly used ChatGPT. It was obvious by the increase in technicality and vocabulary in his response.
George
Out of interest, why are you encrypting this?
Karthick
According to the system audit query, MAC address and Sending user display name and the location name should be encrypted.
George
But Mac Address is sent in plaintext as part of UDP - I can see it in your screenshot above
Karthick
It is encrypted at Controller before sending.
George
Yeah I know you encrypt it in the payload, that was my question, why are you encrypting something that is in plaintext elsewhere on the network traffic?
Karthick
In most cases, the visibility of the sender's IP address is not a concern. IP addresses are public and essential for routing data across networks. While it's possible to use techniques like VPNs or proxies to obscure your real IP address from the recipient of the data, these techniques are not directly related to the encryption of the UDP packet payload.
George
Did you ask ChatGPT for that?
Also, I'm talking about MAC Addresses, not IP addresses.
I don't really care if you encrypt it or not, I was curious what decision led to you encrypting a MAC address.
Karthick
ChatGPT gave a good answer 😃
I cannot see MAC address of Wireshark UDP packet trace done in my laptop.
Govind
From the Pen test, it is reported that the
"Disclosing MAC Addresses and display names of users provides a wealth of information to a potential attacker"
via panic alerts.
That's the reason we are encrypting the Mac address.
And I believe the Mac address showing on the Wireshark, is the system Mac address(the interceptor machine) on which the Wireshark tool is running.
George
All you had to say was "Pen testers" and I would have been happy 😄
Alright, it’s a somewhat clickbaity title, but I did cause a lot of confusion to one person with one simple feature, and she still didn’t understand when I felt I had perfectly explained it.
I had made a grid which shows a person’s name, alongside a list of items they have ordered, in alphabetical order; as per the requirements.
Later on, we would add alternating row colours to make different people’s orders more distinctive. In the example I came up with, there were only 2 orders on a page, and I had left the row selection highlight on, so it actually looked like we did have alternating colours.
The examples from the UX team only had a few items per person, so it wasn’t clear how they had aligned the text. I left it centred which looked a bit weird for large orders, which is the example I came up with.
Person
Items
Lisa
phone
CD
hat
lamp
James
phone
shorts
t-shirt
towel
I don’t see how I can accurately illustrate this with WordPress. Imagine Lisa’s row is fully highlighted
Just to show my progress on this new project, I posted a screenshot to generate a bit of hype 😉
Olivia was confused about why it wasn’t in alphabetical order – but it is in alphabetical order!
Olivia: why is “phone” at the top?
Me: Ulrika's designs or my screenshot? Mine is alphabetical. Phone is for a different Person. We still need to add the alternating colours, and maybe don't centre the name to make it clearer
Olivia: this one <sends screenshot of top row>
Me: Different Person
Olivia: I'm confused. Lisa's name is the same row as Phone
Me: There's 2 People there. Just that the second person has loads of items, and because we have centred the Person name, it's halfway down the row. We can use alternate row colours as Ulrika’s design, and we can stop centering the name to make it clear.
I message Tim:
Me: I can't believe the confusion that has arisen even though I have explained what it is.
Tim: I got your back
<Back in the main chat>
Tim: Phone is for Lisa there
Tim: Everything else is for James
Tim: Which is when the alphabetical order kicks in.
Olivia: Thank you Tim, That was seriously messing with my mind. The name needs to be at the start of the list of items.
<Back to private message with Tim>
Me: I cannot understand why my explanation wasn't good enough. Is it because I am a developer nerd and cannot communicate with people?
Tim <jestingly>: the trick is to keep your answers on one line. Colours and shapes help too.
I’m so baffled. Why couldn’t they understand what I wrote? I wrote a perfect explanation then Tim just put his explanation in short messages, and he was thanked for his explanation. It’s like I don’t exist!
Some teams post fortnightly updates on what they have done on our internal social media platform Microsoft’s Yammer (Now named Viva Engage). No matter what they post, managers seem to respond with positive comments.
So here is an example. On team stated they had completed:
3 user stories. 1 dev investigation. Improved code coverage.
Then attached was there code analysis report which stated:
0 Bugs
0 Code smells
100% coverage on 1 new lines to cover
0% duplications on 8 new lines
I never know how to make sense of these stats. The duplications mention 8 new lines, but yet the code coverage mentions 1 new line. It can’t mean it is still to cover when it states they have 100% coverage.
Their video demo stated it was “Sprint 8” which means it’s after 16 weeks of work. They logged into their application and there are 2 buttons (not functional).
My Work
I’ve been working on a game in Unity and have also reached the 16 week mark. I have only been working on it sporadically. Early on, I might have worked a few hours and many days per week, sometimes working several hours at a weekend. Other weeks, I have just worked on it for 3 hours or so in one afternoon only.
I have written loads of code for the AI state machine. Your units can automatically move in a formation, break away in certain conditions. Then the defending group of units have their own behaviours, and work together in their own way. Some behaviour is configurable so I have the logic and UI for the player to change it. There’s controls for the cameras, game speed, pausing, save and reloading. A few options for volume and a few graphical effects. There’s some miscellaneous aesthetics such as animations, particle effects.
I am a single developer working fewer hours, and I have to play-test it myself. Compared to a team with a few developers, a few testers and different managers assigned – all working a 9-5 day, 5 days a week; and all they have is a menu and a title banner.
Hackathon Progress
Another comparison is to compare the progress of developers during a “hackathon”, or “game jam”. Developers can often put together a prototype, or a fairly fun game within a few days…
Developers during hackathon: We built an entire application in just 3 days.
Developers after hackathon: Adding that icon is going to take 3 weeks.
Developers during hackathon: We built an entire application in just 3 days. Developers after hackathon: Adding that icon is going to take 3 weeks.
Mark Dalgleish
Conclusion
Recently, I’ve been thinking about this a lot. I think if you start with no code, then any code you write has a much bigger impact. You also don’t have to work out how it works alongside other code.
So the more code there is already, then the longer it takes to write. So if the Hackathons are productive, then you can write something fast. Same situation with my Unity game, and since I am working on it myself, then I have no one slowing me down.
The other reason why working on my own is great is that I can set the process. Due to the 90/10 rule, I think it is best just to get it 90% done, then move on. If you change your mind of how something works, it isn’t as big of a time waste. If it turns out to be good, then you improve it later, alongside other features when you are changing code in that area.
So going back to this original team who did 16 weeks work and got nowhere – even though it’s a new software product, and should be able to code fast – I think they are slowed down by their insistence that everything has to be perfect and has all the extras like Unit Tests. So they could get a feature 90% complete quickly, but then they are told to spend several days perfecting it for that last 10% of quality. Then they are messing around with non-functional stuff like the Deployment Pipelines and miscellaneous processes. When you are months away from releasing, I don’t see the point in dedicating so much time to how you are going to deploy it, so I have been 100% focussed on features and gameplay.
I think this could illustrate how process can severely slow you down, and striving for perfection can also lead to inefficiencies. I think my idea of “90% quality is good enough” could be a winning approach, then you can always perfect the features if you are ahead of schedule. If I keep working on my game, I will worry about how to release it when it is close to a releasable state (ie how to get your game listed on Steam).
Stranger Coding Tings 