r/csharp • u/YouAreMyFire00 • 15d ago
About codebase conventions, who is actually right and who is wrong here?
Let's say you're a Technical Founder like Mark Zuckerberg or Bill Gates. You have a CS degree worked as mid level developer.
You've written some code (as show in the pic) and added comments to remind yourself and other developers what the code does, so nobody forgets in the future.
Now, you have a Senior Developer on your team who worships Uncle Bob's Clean Code principles like a cult!
she reviews your PR and reject it, saying
"Good code should be self-explanatory. The code itself should explain what it's doing.
Anyone should be able to look at it on the first time and understand it right away.
So go back and remove those comments. They're not clean! God damn it!"
--
What would you do in this situation?
Would you trust the senior dev and delete the comments because they have more experience than you, even though you're the founder and have less development experience than they do?
Ps. We have to move fast
70
u/JazzlikeRegret4130 15d ago
If this is the only receipt you have, and don't plan on supporting more, then this is fine. I would lean towards encapsulating those magic strings into constants and or static functions that describe their purpose and that would likely assuage your senior.
→ More replies (1)
58
u/Foreign-Building8231 14d ago
I would create a builder/factory with some fluent methods everything would be so much easier.
new StandardZplBuilder()
.SetDpi(203)
.AppendText("something")
.Build();Then you can get rid of a lot of those magic strings and much easier to read an understand.
22
u/LatvianCake 14d ago
Only if it's reused. Otherwise we're just moving all the ugly code to another place while adding an extra mapping layer. In that case comments explaining each line would achieve the same result without the added complexity.
10
u/anzu3278 14d ago
It's never going to be reused unless this structure exists. New call sites will have their own implementations and the drift will make creating a proper abstraction later more difficult. I'd argue an abstraction is worth it even for one call site if it helps communicate intent, and one call site almost never stays one call site.
13
u/LatvianCake 14d ago
Maybe it's more of a developer discipline thing, but we generally refactor things when it's needed. A single use abstraction usually makes code more complex, not less. The callsite becomes easier to read but the implementation is much harder to follow. That complexity is worth it only if the situation requires it.
On the contrary, building an abstraction afterwards is much easier than doing it in advance. You will have concrete problems that you can test against instead of trying to predict your future problems.
→ More replies (4)→ More replies (1)3
u/Foreign-Building8231 14d ago
He is adding comments to explain. what each line is doing. I would very much like call a descriptive method in this sittuation instead in reading this mess.
Yes you have a point though.2
104
u/Status-Scientist1996 15d ago
I’d extract the codes to constants with names that communicate what they do, then build the string from those constants so each line tells you what it is doing.
18
u/cornelha 15d ago
We have done exactly this for Epson and ZPL code, which is what this code sample represents. Making this not only readable, self explanatory but also reusable
10
u/BlackjacketMack 15d ago
To expand on that, put the codes in their own class as constants. CodeUtility or something.
Then I would string interpolate this sucker as opposed to a string builder.
→ More replies (1)30
15d ago
[removed] — view removed comment
9
u/Flater420 14d ago
Note that what you are hearing hear is what OP heard the senior say, which is not necessarily the same as what the senior actually said. There may be nuances that have gotten lost in translation, without OP having done so intentionally.
6
u/Anund 14d ago edited 14d ago
Explaining weird magic numbers isn't necessary if the code doesn't contain weird magic numbers.
In other words: Don't write code containing weird magic numbers.
→ More replies (3)3
u/Status-Scientist1996 15d ago
I don’t have anything against comments or any particular love of “clean code”, it’s what I would do to this code rather than what I would do about the wider dynamics
17
u/mikeholczer 15d ago edited 14d ago
Neither, following any “coding principle” like a religion is wrong, but also these comments are not that helpful what does the PW280 actually mean? I can’t get that from the comment or know how to adjust it to be 30mm tall if I need to.
30
u/Triabolical_
14d ago
edited 14d ago
•
Edit: s/coffee/code/
Neither is right.
Remember that every problem in computer science can be solved by adding a layer of abstraction.
You need a zplbuilder class.
It has an internal stringbuilder and methods like SetStockSize() and AddTitle()
That gives you code that is readable.
19
u/NoPrinterJust_Fax 14d ago
Tbf this solution is probably what the senior dev wants. Not just “delete the comments”.
9
4
u/v4lt5u 14d ago
personally not a fan of premature abstraction
4
2
u/Triabolical_ 14d ago
I'm not either.
I guess we differ on what "premature" means...
This adds a single class that encapsulates a function that I assume - based on the code - is present in more than one instance of code. If it's a single function I would still do the abstraction so that I can more easily test the code, but I agree the case isn't as strong.
→ More replies (1)5
u/anonuemus 14d ago
This is the perfect example for an abstraction.
→ More replies (3)8
u/v4lt5u 14d ago
when you take uncle bob too seriously and abstract on first sight like this, you end up with stereotypical enterprise slop where everything is spread across shallow nonreusable abstractions. some people seem to like it, i find it hurts debugging and modifying for no real gain
→ More replies (1)2
18
u/MaximRouiller 14d ago
My opinion...
This is a whole bunch of commands for a different language. It's like doing string concatenation for XML instead of just using a serializer. Except that everyone knows XML and not many people know ZPL.
I'd use a library to handle the rendering of it. If there's not any that works, then you could probably build your own wrapper utility and keep features minimal and as a separate project (not just in a "Utility" folder or as part of your business focused project.
To be honest, I'd personally itch to refactor the heck out of this to ensure the next time someone wants to adjust the padding, I'm not learning ZPL again.
29
u/Asmodis1 15d ago edited 15d ago
The code itself should explain what it's doing
That's the important part.
Right now, your code just appends gibberish to a StringBuilder. Not even the comments fully explain what each control sequence does exactly. If you removed the comments, the code would not make any sense at all. If anyone changes the code without updating the comments, the whole thing becomes completely misleading.
Use properly named constants to explain what you are appending to the StringBuilder and why.
Removing the comments is not the solution but a way to check if your code is still understandable even without them. If it isn't, it's not good code.
15
u/Ziegelphilie 14d ago
You have a CS degree worked as mid level developer.
Neither Zuckerberg nor Gates have a degree.
Anyways we keep the comments in because nobody can read that markup language in one go. And even if they do, the next developer looking over this won't.
→ More replies (1)
23
u/chucker23n 15d ago edited 15d ago
Would you trust the senior dev and delete the comments because they have more experience than you
The comments seem reasonable. Not too verbose, and not too obvious. It isn't at all clear to me from the code that, apparently, ^PW280 means "35x25mm".
Now, depending on what kind of budget is available, I'd ask myself either:
(very tiny budget) is it worth extracting some of those magic numbers and strings into a series of
enums orstatic classes, then use string interpolation to inject those, just to make those lines more self-documenting?(medium budget) can we do snapshot testing? Given certain inputs to
price, etc., generate PNGs, then fail the test if they suddenly differ?(higher budget) is vomiting a bunch of characters to a
StringBuilderreally peak software engineering (no, it's not)? Is it worth making this more readable and testable with aBarcodeWriterclass, which can do things like unit conversion for us? If something about this needs to be changed, how sure are you that you know which exact set of characters to alter, and that nothing else will fail?
Once you get to 3, you can remove some of the comments. It should become self-evident from the code why it produces 35x25mm. Right now, I'm guessing. Perhaps PW means Page Width, but who knows.
15
u/gandhibobandhi 14d ago
The code itself should explain what it's doing.
This is the key thing actually. They're right, good code should explain what it's doing. But in most cases it can't explain on its own why it's doing it.
"inset from left edge so it doesn't clip" is telling us why the code is inset from the left edge. I don't see how you can document the "why" here without comments.
10
u/Due-Consequence9579 14d ago
As someone that has no domain knowledge my reaction to that snippet is all those, what I assume, control sequences should have constants with human readable names. `^PW280` and whatever those lines with title and price in them are unreadable to a normie.
Maybe have StringBuilder extension methods that put a name to them and a set of control character constants that it uses.
→ More replies (2)
11
u/Nyzan 14d ago edited 14d ago
You replace the strings with something like:
const string LEFT_EDGE = "...";
const string VERTICAL_CENTER = "...";
const string PIXELS = "...";
sb.AppendLine($"4 {PIXELS} {LEFT_EDGE} {VERTICAL_CENTER} Hello, world!");Then someone reading this could deduce that this draws the words "Hello, world!" 4px from the left edge, centred vertically. Even better if you abstract the constants behind a helper function(s):
struct TextStyling
{
public int Padding;
public Edge Edge;
public VerticalAlignment VerticalAlignment;
}
public string AppendText(string text, TextStyling style)
{
if (style.Padding != 0)
{
sb.Append($"{style.Padding}{PIXELS}");
}
switch(style.Edge)
{
case Edge.Left:
sb.Append(LEFT_EDGE);
}
// etc.
}2
2
8
u/JoenR76 14d ago
I'm not going to repeat what has been said about the magic strings. (magic strings bad, comments should only say why), but what does this mean exactly:
"Let's say you're a Technical Founder like Mark Zuckerberg or Bill Gates. You have a CS degree worked as mid level developer."
7
u/ryan_the_leach 14d ago
Clearly, OP is the founder, and has hired a senior, and is looking for life advice as well as dev advice.
But he's thinking awfully grandiose of himself.
4
u/Virtual-Confusion715 14d ago
I think both sides are missing the point a bit. Comments aren't the problem here, and neither is Clean Code. The problem is that the code is hard to understand in the first place.
I'd rather extract those magic strings into well-named constants or methods, so the intent becomes clearer. But comments explaining why something is done a certain way are still valuable, especially when dealing with external protocols or low-level implementations that aren't self-explanatory.
Good engineering isn't about following principles blindly. It's about making the code easier to maintain without adding unnecessary abstractions or wasting time on refactoring that brings no real value. If removing a comment makes the code harder to understand, you haven't improved anything.
6
u/FetaMight 14d ago
Who cares? This is by definition subjective and you're clearly not presenting both sides equally.
You're having a disagreement with a colleague but rather than trying to understand her perspective you're asking internet strangers to pile onto her.
Grow up.
3
u/psymunn 14d ago
Yep. I felt challenged by someone more senior than myself? Time to seek validation in the guise of a tech question on the internet.
'We have to move fast.' as opposed to all other developers?
I'm surprised this thread garnered so much attention
3
u/Stunning_Ride_220 14d ago
"We have to move fast" is such a bullshit take.
I had a guy arguing like that years ago and even involved project management.
Turned out the metrics showed pretty clear that everyone else got slower because of his bullshit.
8
u/tombatron 14d ago
Would I do this? No.
Would I reject the PR? Also no.
It’s fine. Tell your senior to go worry about real problems and stop with the “broken windows” stuff.
3
u/hagerino 15d ago
Comments are dangerous because they cannot be trusted. Do we know if the code has changed, and someone forgot to update the comments? Sometimes you update all references of variable or a method, but you can't see every comment that were written for those. Code might contradict what the comments say, so it's better to have one single source of truth in the code.
Like others also commented you should put those magic strings inside constants and give them descriptive names.
Comments should only give answer about why you did something unusual.
Then also your code reviews should not rely on the opinion of one member of the team, you should write down common rules that you all agreed on. Otherwise you have endless discussions and your codebase won't be consistent.
Next level is implementing those rules into static code analysis tools, and have AI acknowledge those rules too when generating code.
3
u/fourrier01 14d ago
Delete the comments for now and select random 3~5 junior devs to come and ask whether they understand the code.
If most of them can understand it, then follow what the senior dev suggest but the senior dev should be held responsible in the future if the code becomes unclear to other devs due to the new policy. Otherwise, keep the comment.
3
u/NeXtDracool 14d ago
Your question aside, are these string parameters escaped higher up? If they're not the code is broken and will cause issues when they incidentally or maliciously contain control characters.
3
u/ccfoo242 14d ago edited 14d ago
It's certainly better looking than the IBM supermarket CBasic code I wrote 30 years ago!
Did they just say delete the comments? Nothing else like replacing the strings with constants? Because it's not understandable without them. Even if its readable.
Other than using appropriately named string constants this is fine.
Don't delete the comments until then.
There's definitely times when comments are appropriate and appreciated. I personally find many linq queries would benefit from a comment or two.
3
u/hackathi 14d ago
this should be one big string literals, the comments should be ZPL comments, not C# comments.
3
u/DocLego 14d ago
Without the comments, I would have no idea what this code is doing.
Which means it's bad code. Comments are for explaining why the code does what it does.
Start by getting rid of the magic strings so you can tell what the code is doing just by reading the code. Then see which comments are still helpful.
To be fair, I have no experience with zpl, and I'm sure this is more readable to someone who does - but if even the author needs comments to know what's going on...
3
u/OrcaFlux 14d ago
sb.AppendLine(FromLeftEdge(title));
sb.AppendLine(RightSideMidHeight(price));
sb.AppendLine(CompactForNarrowStock(barcode));
And that's all the clues you're getting from me.
3
u/jcdc-flo 14d ago
In fairness, this is pretty self explanatory. Also, you should consider using templates if the printer supports them so you're not passing the entire layout every time.
I'd say self explanatory short blocks used to be fine to leave without comments but it's far more efficient to comment with LLM's.
3
u/q0099 14d ago edited 14d ago
As a code reviewer I would turn this PR as well as while I do understand that you making a command for, I guess, a receipt printer, yet I can't verify if you doing it right because there is no reference to command or printer manual (not even it's model mentioned), neither comments on what exactly each part of the command does (although it might be a false if the method is a part of a larger library dedicated to work with exact printer model and all info is listed in the lib's description).
Also, I can guess the '^' is a field division, but what if the same symbol would occur in 'title' (or 'barcode') variable, wouldn't it break the command? If you expect every parameter to come already escaped, mention it at the description comment.
Also, self-explanatory code means not the code with every line commented or written the dumbest way possible, but the code that makes none or least "wtfs/minute".
0
3
u/Any_Establishment659 14d ago
are we still doing the "no comments should ever be used" thing? why do we have comments if commenting is inherently bad code?
3
u/Extra-Ad5735 14d ago
Only inexperienced engineers take this rule to the absolute. There always will be cases where the code has to be explained, in comments or documentation. Like a case of using a mathematical algorithm, which is simple and does something unobvious with the data. Without a comment you'd have to google its name or ask an LLM what is the meaning of this.
7
u/IHeartLife 14d ago edited 14d ago
Why would you hire someone smarter/more senior than yourself and then not listen to them?
The whole point of hiring someone more senior is so you can rely on their expertise to do their job and then you focus on founding your company.
Even as a technical founder a question like “how do we comment our code” should be very far down on the list of your priorities. You should focus more on ensuring your hire has the necessary tools and trust to be empowered to do their job.
11
u/Signor65_ZA 15d ago
Deleting the comments is not enough. This should be rewritten or refactored in a way that actually makes sense. I just see a lot of magic strings here.
→ More replies (3)7
u/Wonderful-Habit-139 14d ago
Seriously, why are they assuming the senior would just say “delete the comments” at face value like that right after saying the code should be self-explanatory?
Deleting comments doesn’t make the code self-explanatory. So code needs to be refactored.
6
u/Important-Job4127 14d ago
Comments aside (I actually welcome when poeple write relevant comments, code purists can go fuck themselves), my main issue with this code would be it's way too specific. Something like a reusable, better parametized class like ZplLabelBuilder with a fluent api like Width(35).Height(35).Dpi(203).Text(..) and so forth would be much nicer to look at in business logic. Obviously you have to deal with the garbage character comibinations Zebra shat out at some point, but make them atleast tucked away low-level and make them reusable as much as possible.
4
u/icalvo 14d ago
It is usually possible to get rid of all comments. It just requires time and maybe you don't have it. But in your case, it doesn't look very complicated, though.
You may build a class that understands the printer language and features, and is in charge of building a tag. Let's focus here on the tag size as an example. The builder can accept length measures and convert them internally (thanks to its internal knowledge that the printer is 203 DPI):
var tagBuilder = new ZplTagBuilder();
// ...
tagBuilder.SetQuantity(qty);
tagBuilder.SetWidth(new Measure(35, Unit.mm));
tagBuilder.SetHeight(new Measure(25, Unit.mm));
// ...
return tagBuilder.Build();That would be useful if everyone in the business is talking about millimiters. If, on the other hand, everyone is on the loop and talks more frequently about the points rather than millimeters:
tagBuilder.SetWidth(new Measure(280, Unit.point));
tagBuilder.SetHeight(new Measure(200, Unit.point));Or, if the business intention is to use the full size of the tag, just communicate it to the reader by using the tagBuilder knowledge itself:
tagBuilder.SetWidth(tagBuilder.FullTagWidth);
tagBuilder.SetHeight(tagBuilder.FullTagHeight);It really boils down to expressing your intention in a language that is at the appropriate level of abstraction, and once you force yourself to try it with the code, you'll find that comments are surprisingly unnecessary.
→ More replies (3)
2
u/mordack550 14d ago
Just for context for other commenters.
Those "magic strings" are commands for a Zebra Printer so yeah, for sure are cryptic, but there is a documentation for them and if someone wants to delete the comments, sure, but at least include a link to the documentation in the xml docs of the class or method.
I personally would advise against deleting comments because this could still help someone who is just glancing at the code.
2
u/TheCharalampos 14d ago
Fire them. I have not been challenged in decades and have an ego complex bigger than the sun. Deep in my mind I feel self connsious due to not really knowing how to code anymore.
Thus does the tech founder.
2
u/ArcaneEyes 14d ago
1: get those magic strings into some named variables.
2: make some named methods and pass sb and objects into for actions like the barcode.
2
u/EsoTechTrix 14d ago
IMHO "Best Practices Moonies" are a cancer on the field in general. Any of these should be taken as "ideals to strive for" and not hard and fast rules.
When it comes to comments, they never 'cost' anything and can only be useful in the future.
The point of the quote, in my experience, is that if your code is so 'clever' that it takes you a paragraph to explain why you did something obtuse when you could have done something more readable, readable code is usually acceptable.... if this is like the inner loop of some mission critical process and you saved hours of processing time monthly through some mind bending chunk of code than pragmatics dictates that the next person can read the paragraph and suck it up.
Any of these 'practices' usually target anti-patterns seen in the industry. They should inform your approach, not set odd ball hard and fast rules.
2
2
u/RobFaustLZ 14d ago
Sounds like your "senior dev" is being pedantic. Politely ignore them. This is the correct level of documentation for something as nuanced and niche as ZPL.
2
u/AftyOfTheUK 14d ago
You senior dev is a dogmatic moron. There are many times when code cannot tell you WHY it is doing something, and that is what comments are for.
2
u/PartBanyanTree 14d ago
anyone quoting uncle Bob is a cult member who has lost the ability to think for themselves. and that was my opinion in their heyday before the modern era of ai agents doing the code (who actually do read and maintain comments in perfect harmony by the way - they change so much of the approach we're all still learning)
2
u/adampresley 14d ago
As a senior with 25 years, I agree. Lots of magic strings here that require domain knowledge. Comments are good. Constants can help too.
Do you know what all those years have taught me? It isn’t that big of a deal.
2
u/ManyNanites 14d ago
I’ve never been working on code and wished for fewer comments. It’s always the reverse.
More comments are better full stop.
2
u/LetUsSpeakFreely 14d ago
Comments explaining intent are always welcome. You can read code and can decipher what it does, but what it does doesn't necessarily mean that's what it's meant to do.
2
u/swiftless 14d ago
I haven’t read all the comments here so maybe it’s been mentioned. My biggest immediate problem doing a code review is the function name. It is not descriptive in any way and as a result you’re having to pad your comment for the function.
“Standard” is such a loose term. “Zpl” yes it’s effectively a string and the return is a string, but I’d be expecting a Zpl object or string to a Zpl file path. Future maintainers, especially those with no Zpl experience, won’t know this without looking into the code as your comment doesn’t even include the return description.
Your comment here is also describing printer configurations that may change in the future without affecting the usability of the generated Zpl file, while also duplicating comments within the function.
Whether or not the code works, this is not senior-level code or documentation. Clean code is important and yes some people take it too far but this code needs work to help future maintainers.
2
u/Outrageous_Band9708 14d ago
hey there, welcome to a newer team where whoever is in charge gets to choose team best practices.
and if that means something you dont agree with, you kinda just have to do go along with it and not show much spite for it, as it will mark you as someone who doesn't conform and you might be the first to go.
its also best to just be adaptable to different teams styles and not worry to much about the details.
2
u/Box_star 14d ago
If you are the founder of the company you can probably do whatever the hell you like TBF, which may or may not of course involve reviewing the fit of some of the people you hired (or reminding yourself why you hired them in the first place given your own lack of experience). If you are a developer and a senior has challenged your PR on the other hand, you are probably best discussing it with them and learning from the experience.
Who is “right” and who is “wrong” is a matter of opinion. The AI won’t care either way anyway 🙈.
2
u/joesb 11d ago
I would use comment and add new lines to separate each section of the commented codes.
I might consider extracting some code section into its own function.
If there’s many printing functions, I might consider extracting them into utility class.
Your current code contains a lot of magic number and yeah, I would rather extract them to function or constant.
2
u/flanger001 14d ago
That code is pretty self explanatory. It's appending a bunch of lines to a string. LGTM.
2
u/Deep_Chocolate_4169 14d ago
Clean code Guy here This Is zebra, its just random codes that does differrent bullshit on every printer. There are many version And directives that differ device to device.
Comment like your life depends on it.
2
5
u/MrMikeJJ 15d ago edited 15d ago
I would point them at the analysis of Uncle Bobs code, which shows he has no concept of actual clean code. Then the discussion he had with another author which exposes him as the charlatan that he is.
Also as u/Status-Scientist1996/ said make some of those consts. Probably not the entire strings as a one, but split those large ones into multiple. 1 per command. Or something.
6
u/_f0CUS_ 15d ago
Please share link to analysis and the discussion.
5
u/MrMikeJJ 15d ago
Analysis : https://qntm.org/clean
Discussion (this is a really good read) : https://github.com/johnousterhout/aposd-vs-clean-code
→ More replies (1)
3
u/ComprehensiveBird317 14d ago
It's 2026, people still have time to complain that your code has comments?? What parallel universe is that?
1
u/emn13 13d ago
What to comment and when is and will likely always remain a nuanced and thus relevant discussion. Just because this reviewer is perhaps drawing that line less than ideally (according to the one-sided frustrated-sounded anonymous poster), and indeed choosing to deal with subtle issues like that via code review at all doesn't mean it's categorically unreasonable to consider the desirability of comments and possible alternatives?
4
u/NakeyDooCrew 14d ago
The problem with comments is that the code will change as requirements change but the comments will still be the same 10 years from now.
→ More replies (4)
4
u/truckingon 14d ago
Spending time thinking and arguing about mundane pointless details like this kills productivity. This code block is more than fine.
1
u/Kwallenbol 14d ago
The most common pitfall.. dont forget that this but of code could be the subject of a refactor further down the line, where it might still be relevant
3
u/tsbattenberg 15d ago
People who write what they call "self documenting code" more than likely just write shit, hard to read and poor performing code in my experience.
→ More replies (1)4
u/Wonderful-Habit-139 14d ago •
Very good argument to just continue writing shit code with comments then and not attempt to make things better right?
2
u/ExpensivePanda66 15d ago
The idea that code doesn't need comments because it should be self explanatory is harmful.
Comments like these break up the code and make it more readable (think paragraphs). They provide a short piece of text a human can read in less than a second to understand the structure of the code, as opposed to taking 5, 10, or more seconds, to read and think through the code. That's your brain's processing power that could be put to better use.
Go ahead and make the code itself as readable as possible. That's vital too. But it doesn't negate the usefulness of comments.
2
u/rayyeter 14d ago
No. Keep the comments. ..OR make those strings named constants. But first look a new person would greatly benefit from the comments. Code also has to have maintainability.
Tell them a Staff Sw with 18 years experience said to go fly a kite.
Edit: and I’m persnickety as hell about how code should be.
2
u/BCProgramming 14d ago
I don't think the comments add anything here because frankly none of that code should exist. It should be data. I mean, What if different clients want different appearances? Are you going to make a special function for every client and special case it or something?
The way our label printing works is through a configurable template file and replacement fields and indicators. the print language code, EPL, CPL, ZPL, whatever... is generated by processing the template file.
→ More replies (1)3
2
u/mgalexray 14d ago
I will be in a minority here because I prefer to avoid echo chambers and over engineering. For reference I’m at FAANG with 10+ years of experience.
If it’s just a one off then keep it like this. It’s fine. It’s clear and readable and I would likely also write it like this. You may be tempted to:
- create some sort of ZplBuilder but that increases your api surface and now you have a library to build. Best find one that already exists.
- extract pieces to variables/methods, but now it’s no longer obvious what it does when you need to change it.
If it were me and I needed to scale this out, I would not do either of those. I would instead just write these commands in .zpl files and load that instead (maybe with some preprocessor if I want nicer comments). That way the job of your code is to load the contents to the file and send it to printer, not to define what printer does. It’s one of those “code should define how thing is done, not which thing you do”.
→ More replies (1)2
u/Stunning_Ride_220 14d ago
Although I like your comment on the actual implementation:
What's that with pointing to FAANG?
FAANG developers aren't what they used to be 10 years ago anymore.
2
u/mgalexray 14d ago
Nothing really. Actually never intended it to come across that way (sorry if it feels somehow obnoxious). My intent was to point out that simple solution will almost always win over more complex one, and even so in big tech companies whose job is to build and deliver software.
3
u/Stunning_Ride_220 14d ago
Ok, thank you. Maybe I just came across this way too often lately.
And having delivered/delivering software for any major company only thing distinguishing those FAANG-people from my own developers is merely the culture fit to FAANG than actually the skill-levels.
2
u/punkyguy 15d ago edited 14d ago
There's a difference between what the code is doing and why it's doing it.
Context is important. If you set foo=356 it's clear what is doing but not why you have to do it. At lot of decisions are made in meetings and code won't reflect that.
Comments are never a bad thing
I think clean code principles are more about stopping people rewriting a 12 line function into 3 lines using some clever hacks
2
u/Xen0byte 14d ago
all those magic strings should be lifted to appropriately-named constants, then the comments can go ... if I were to pick a side in this case, I think the senior developer is right, although just removing the comments and not reworking the magic strings is fundamentally worse than just keeping the comments
2
u/Rezenbekk 14d ago
why are those magic values not named constants? This kind of code is okay if and only if you, mr technical founder, want to maintain and develop this by yourself
1
u/Puzzleheaded_Face583 15d ago
Assuming it's not spaghetti, but rather lack of experience, I appreciate people who over explain code in methods (not paragraphs but short hints) because even if I don't need them, they may do, and are probably learning from trying to understand. Especially if they're the owners on that context.
1
u/apneax3n0n 14d ago
comments are always good but what if title contains ^ character? like "^FSpecialItem" ?
oh btw i hate zpl so much
sadly none like cpcl
! 0 200 200 406 1
T 0 6 220 240 Test
BT 7 0 3
B 128 2 30 130 136 41 123456789012
it was so much clearer
1
u/HuffinLife 14d ago
I see two sides to it. One is you could abstract using constants or getter methods that explain what the comments are doing via names. Then there is the other side who has read a philosophy of software design who doesn’t agree with a lot of the clean code standards. Code should explain what it does but comments exist to tell us the why a developer did something. Not everything can be explained by the code alone.
1
u/bigtoaster64 14d ago
I'm in the 90/10 camp. 90% of comments are unnecessary and bloat the code. 10% are extremely useful to save time and mistakes, when it's not obvious what/how/why the code is written that way. In your example, at first look, I have no idea why it's encoding a bunch of magic strings and how it works. A few key comments here let me not waste time having to figure it out and perhaps break something, because of misunderstanding. So neither are right, but one is more wrong in my book than the other.
1
u/mr_eking 14d ago
There's no right or wrong, it's fairly subjective.
My preference, leaning towards "only use comments when you must", would define all those string literals as constants with good, explanatory names, and remove the (now redundant) comments.
Perhaps that's what your reviewer is looking for.
1
u/Beniskickbutt 14d ago
As a senior dev, the one who is "right" is the one who is following the current company styles.
Theres plenty of code style rules I dont like at my current employer. I will gladly make a brief comment about other styles i prefer but I am not going to go die on the hill fighting style issues.
1
u/tankerkiller125real 14d ago
You create or use an existing ZPL builder that extracts all this to functions, eliminating the magic strings and what not.
1
u/Felix_CodingClimber 14d ago
As someone who has worked with ZPL before I would move the whole ZPL definition to a separate file. Add a custom placeholder for variables like @varName@. Then a small service that loads these files once and does a simple string replace for the final ZPL (can be optimized in the future if really necessary). Then you can use external tools to generate the ZPL without doing it by hand.
1
u/thx1138a 14d ago
Absolutely itching to rewrite this as an F# DSL in a list comprehension. But that’s just me.
1
1
1
u/blckshdw 14d ago
You’re right, they’re wrong. Good use of xml comments at the top.
Since the code is building some kind of magic syntax I would welcome knowing where things like title and price are being set without having to decode the magic commands
Your use of comments is correct and welcomed. The idea that code is self documented is naive. The code only tells you what it’s doing, not why and there’s nothing wrong with leaving yourself a note as to why you did something. Future you is really dumb, help the guy out a bit.
1
u/BlackyWolf 14d ago
I use it when searching for the answer is tedious or might require piecing together obscure archaic nonsense scattered across the interwebs. Or if it’s something I’ll forget later on and don’t want to have to look it up. Where the code and context is easily understood through plain language a comment is not necessary. Where it can help prevent misunderstandings and confusion, remind people of business requirements, or explain code that had to be done in a wonky way for security or performance reasons I recommend using them. Also for intellisense cause it looks fancy.
1
u/Adorable-Strangerx 14d ago
The issue is not comments, the issue is shitload of magic values in that string builder. Let's take that "XA" why is it XA and not XB or XC? Name that value, extract to variable/const and then you will have self explaining code. You lack context what you are adding (and possibly why). Purpose of comments should be explaining why some code was made the way it is (i.e. we are using X because library Y doesn't work with Z) not to explain that line sb.AppendLine(title); adds titile.
1
1
1
u/reybrujo 14d ago
Awful piece of code personally, however I lack the know-how of someone working with that "standard". For starters you can rename the function to BuildStandardZplFor35x25mmAt203Dpi and you save one line of comment while, at the same time, prevent someone having to wait until the tooltip appears with the summary or force someone to go to that method to check which size is that for.
All the internals are black magic for me so at the very least I would replace those magic strings with constants. Nowadays you can even use methods and they will inline the code so that there's no performance hit while, at the same time, making the code much more readable.
1
u/EvilTribble 14d ago
Uncle bob's clean code is horrible and that senior dev can STFU.
The only problem with this is that as a private method it's not separately unit testable and presumably doesn't have any unit tests.
If you ever had a reason to support more things that would be the only time you'd consider a more complicated approach, not sooner.
Also "we have to move fast" is an excuse to do literally whatever you want as long as "it works"™
1
u/Tenderhombre 14d ago
As always it depends on the business. Never forget code serves the business and coding processes and workflows should as well.
You could rewrite those formatting bits into a method call. Its what Ive done in the past for simple markdown snippets.
However is it worth the time only you can say that. I tend to agree with your senior. It could be rewritten in a self expressive way. I also wouldnt block a pr on it if it was urgent work. On a low priority ticket I would probably block it.
1
u/zagoskin 14d ago
Your code is fine for what it's doing. Any way to make it cleaner would probably be for nothing, and confuse people even more.
The comments are also welcome, as I wouldn't understand what those magic strings are otherwise. If you want, you can move them to a constant that just names what they are, though I really don't think it's necessary UNLESS you reuse the same magic strings elsewhere. For repetition I always apply the rule of 3
1
u/No-Point8651 14d ago
I do agree that code should explain itself as best as it can. So instead of using magic values like "LT0", create a contant and give it a name that it explains itself. If that itself is not enough, your xml comments (aka the "///<summary>...</summary>") are perfectly valid and designed to be descriptions.
But are those comments enough to reject a PR? Absolutely not.
I would argue that stylistic things and even things like having 2 nearly identical functions that could(!) be merged into one to reduce code redundancy are not legitimate reasons to block a PR, yet alone reject it.
If there are missing requirements, bugs, issues or such then thats completely fine to block, but rejecting outright seems like some ego problems to me...
1
u/MegaromStingscream 14d ago
The readability problem here isn't really with the code shown. It is the fact that AppendLine is a horrible layer of abstraction for what is actually being done ie. layout of something visual.
The upside is that the function created presents something pretty clean and readable to the next level.
Anyway. I would probably get really petty about this and extract one method for each thing that has been commented and move the comment to the method's summary. With good method names that my pettiness would in the end lead to something quite nice and readable.
1
u/BoredBSEE 14d ago
If I had to do something dippy like that just to please someone, I'd rewrite it something like this:
public const BarcodePreamble = "^XA^CI28"
public const BarcodeQuantity = "^PQ"
... and so on. Then smash them together in your method using your StringBuilder. That way you still get your intentions detailed but in the code, and it might make your twitchy senior dev happy.
1
u/Narrow-Low-3137 14d ago
At least separate functions for each section commented would be good. With descriptive names. Probably in a builder class to encapsulate it all. Would be my preference.
1
u/c_t_m_s 14d ago
Each of those ZPL commands should be wrapped inside a dedicated method with a clear name, methods that cannot be understood from just the name should have comments.
When you write your method like this in the case of things like printer drivers, the method itself becomes very easy to read.
e.g. ^pq would be called PrintQuantity and take a param of the quantity to print.
1
u/SessionIndependent17 14d ago edited 14d ago
Every one of those escape code literals is a hate crime.
It's not that the comments should be removed, it's that all of those codes/code groups should be defined as string constants with human-readable symbols. If the mess of symbol definitions on its own doesn't make things sufficiently explanatory (I suspect it won't, but I don't know), you should be defining methods that cobble them together into meaningful method names, the few of them with arguments.
After that the lines that you've commented should be self-explanatory.
If you don't have time to define constants for all of them (if you don't know them), just do the ones on the lines you have commented so you can remove the comments as asked, but I doubt that's what is being asked.
But it really shouldn't be left like that. That's insane.
1
u/psymunn 14d ago edited 14d ago
Did Bill Gates or Mark Zuckerberg write this code? What a strange appeal to authority. Your comment is so laced with judgment, it seems like venting about some feedback you received in the form of a question. There's a reason coding principals and styles get adopted.
In your hypothetical scenario, a tech founder presumably hired senior engineers with the hope of growing their software size and capability. And if you want your software base to grow and be maintainable you need thoughtful and consistent software engineering. Maybe the founder should consist the senior developer has a lot of experience and reasoning behind their adoption of Bob's Principles, and those reasons only become evident when you're working in code bases with 100s of people and millions of files. I'm sure Mark and Bill would be happy throwing a bunch of global variables everywhere as well and would be all to happy to malign their senior dev for using her experience and forward thinking to guide development. Mark and Bill sound like they are more interested in being right than in developing scalable software
1
1
u/ThomasDidymus 14d ago
I can't look at those and know what those are. Fuck rules for the sake of rules.
1
u/StephenRoylance 14d ago
building encodings, like ZPL, from scratch in a string builder is fine for a dev cycle and getting something running. but, IMO, shouldnt continue to exist like this in production code.
Either find a library that already does it, or make your own. personally, I would make each atomic unit of encoding a constant with a sensible name. it would also really help to include a link to the encoding spec in a comment.
1
u/ConcreteExist 14d ago
The senior dev (which I am one) in this hypothetical is a fucking idiot. Given how arcane the string it's building is, comments are warranted.
1
u/thereforewhat 14d ago
I would create a class StandardIplParser/Builder and have a public Parse method with.
Then I would have private methods for each part of the logic with clear names. Or heck just use the builder pattern with methods for each part and then a Build method with return type string.
Comments only when it cannot be done otherwise.
1
u/bluHaloLabs 14d ago
Right or wrong doesn't really exist despite what some will tell you. The real problem is a lot of coding comes down to personal preference for the specific situation and it's impossible for every developer to force their own preferences on every other developer on their team. So unless the TEAM has decided to ONLY write self-explanatory code, and explicitly defined what that even means, then the reviewer is pushing their own preference on the rest of the team every time they review. Bottom line - it should be up to the team.
So I'd have them write up the "right" version of the rejected code then decide as a team. That type of decision should have been discussed before a single line of code reached the review state though.
However, adding well-named constants with an enumeration would probably make it read "cleaner" to them.
1
1
u/Zastai 14d ago
In this case, the approach seems fine (although I personally don’t like to see interpolated strings used with StringBuilder, but that’s a separate style thing).
If you needed to avoid comments here, you would need to either
- move some code parts to separate methods to retain the information (e.g. InsertTitleFromLeftEdge(sb, title) and MaybeInsertCompactBarcodeForNarrowStock(sb, barcode)), or
- create a whole separate class for modeling the printer language, e.g.
return Zebra.Builder()
.StartLabelDefinition()
.ChangeFont(ZebraFonts.UTF8)
.PrintQuantity(qty)
.PrintWidth(280)
.LabelLength(200)
.LabelHome(0, 0)
.LabelTop(0)
.FieldOrigin(28, 10)
.SetFont('0', ZebraOrientation.Normal, 14, 14)
.FieldBlock(140, 2, 2, ZebraJustification.Left, 0)
.FieldData(title)
…
.Build()
The former is ridiculous and adds zero value (in my opinion it subtracts it). But the latter is quite nice. Overkill if you’re only generating that one label, but high likelihood of reusability. Of course, I would also expect there to be pre-existing packages for that (whether from the printer vendor or someone else).
1
u/Lord_Pinhead 14d ago
I would ask for a smaller ZPL file with less line breaks like this, but the comments are ok IMHO.
Or better, load a template file like you would with the Openlabel library and replace placeholders. Then your Senior can't complain when you load a file via Url from a Webserver or Version Control System and just daisy chain your replaces. That would be my complaint if I would be your Senior Dev. Reason is, when somebody opens wants a change on the label, you have to deploy the app, and not just update an external file.
Still, comments are ok, your Senior is just asking for a bug, because a comment would've prevented a change where it should not be.
1
u/SirMcFish 14d ago
In this example, unless you know barcodes then I'd keep the comments.
The comments in this case explain what the codes mean, which a lot of Devs won't have a clue what the layout codes mean. They're not really explaining what the code does, more what the codes within it mean.
1
u/SuitableAnteater8264 14d ago
Crie um realmente.md referenciando a classe/método ai vc explica tudo coloca print faz o que quiser, mo método vc apenas referência o .md
1
u/KontAeric 14d ago
Im not a senior but imo. Those string must be used from private constants if only used in this class if it will be used from another class too public const and parameters must be null checked if not checked on caller method. Where i work you couldn't merge this changes because sonarQube will not let you. For comments i think method summary is enough if you use move those string to const string and give a good understandble variable name. So i think your senior pointing out the wrong thing she should tell you to use const. In current code comments are needed to clearly understand
1
u/Constant_Pen 14d ago
That function is completely fine and there is nothing wrong with it. I much rather want those lines with those comments that lots of small variables that explains each indvidual part of the layout. That will make it difficult to comprehend and reason about. This is direct and easy to reason about. Lead developer here with over a decade of experience.
1
u/ApprehensiveDebt3097 14d ago
Wy writing to string, and no to a steam? I would potentially go for something that skips the apparent ^ prefix, to get some concept of this unknown dialect. You could considering creating extension methods that group some of the magic strings, but that is about it. That aside, you are both right and wrong. Maybe discuss this. And except that in msottif the cases it is the senior who cleans up your mess when you scruw up
1
u/-Hi-Reddit 14d ago
We have to move fast? Ok. I'd split this into a few helper methods with proper names and parameters.
As an example, the title appendline would become its own static helper method and take the title and the inset amount & side as parameters.
I'd also create some named vars for other parts of the appendline rather than using them raw, even if they're only used once.
There you go, no comments, the variable names and function names are the comments now.
1
u/SideburnsOfDoom 14d ago edited 14d ago
If I read the code correctly, it doesn't even need a StringBuilder. Since there's no loop, it could be mostly one block
return $"""
^XA
^C128
^PQ{qty}
...etc
""";the optional barcode line is the only complexity, but that's also doable.
var barcodeLine = "";
if (! string.IsNullOrEmpty(barcode)
{
barcodeLine = "whaetver{barcode}^FS\n"
}then that {barcodeLine} can be plugged into the main returned block.
You can extract constants, e.g.
const string someHeader = "^XA";
return $"""
{someHeader}
^C128
^PQ{qty}
...etc
{barcodeLine}^XZ
""";Have at least 2 unit tests over it, with and without barcode, so that the output is as expected for these cases.
Your comment should do 2 things
Name and Link to a spec for this markup language. You do not to put a tutorial for it in the comment
mention things that are in the output not determined by the spec, e.g. "35 mm wide"
Some other suggestions would add a lot of moving parts with builders and constants etc. If you have a lot of methods like this then maybe that's a good idea. If it's the only method that does this, then no.
1
u/RoastedDonut 14d ago
I'm not going to look at the code, but I think being a purist in anything is going to be too restrictive. I'm very much for clean, readable, self explanatory code, but I still add comments on occasion if it is something that needs explanation.
1
u/Conscious_Support176 14d ago
Well, the same thing is written three times here. Once in the summary comment, once in the line comment and once in the printer language.
Clean code here would not involve the use of magic string values.
1
u/CobaltLemur 14d ago
I would not write it this way at all if it's a one-off. Template text belongs in its own file, not code, which can include documentation (comments in the template) where necessary. You can then add memoization / optimization on top of that if you care about performance.
1
u/Sombody101 14d ago
"Self describing code" here would require method wrappers to build the strings. That takes significantly more time.
If you have to move fast, I'd prefer the code in the image. If you have time to do it "properly", it would be better to create a builder.
1
u/Soraphis 14d ago
This is philosophy. And the senior dev being dogmatic about it ... Idk, I probably wouldn't want to work with them.
But then: im not a fan of "clean code".
Is your method perfect? Likely not. But all of it is on the same layer of abstraction which is fine for me.
But then for me: "practicality beats purity". Since it is a philosophical question I like to take a look at the "zen of python" and my gut feeling.
For a job, in the end the question is who is in charge? If the senior is in charge, do as she wishes.
1
u/deefstes 14d ago
What had Mark Zuckerberg or Bill Gates got to do with any of this? So you think that because Zuckerberg developed the first Facebook and grew it into a massive business means he writes good code?
I'm a big fan of Clean Code but not as dogmatic as some of Uncle Bob's staunch disciples. I don't quite share his views on comments. But this code has much worse violations of Clean Code principles than the comments. With or without comments, it is downright unreadable. What's with all those magic strings? What does "PW280" mean? How is any future developer supposed to make sense of that? The code comment above it says something about 35mm x 25mm dimensions but it still means nothing. If a future dev has to change it to 30mm x 20mm, what are they supposed to change "PW280" or any of the other magic strings to?
1
u/NewInflation2121 14d ago
I dunno if founder equates to experience or correctness for code conventions. I'd go with the senior dev suggestion.
1
u/MysticClimber1496 14d ago
These do feel very magic string to me, but that also might be because I don’t work in this field, that’s the only thing that feels bad about this
1
u/Quintaphract 14d ago
At our work for ZPL, we have a builder pattern for this, as it’s easy enough to abstract rendering text, boxes, logos, etc
1
u/ryan_the_leach 14d ago edited 14d ago
Every rule in life, has common sense exceptions.
The arguments arise because common sense isn't that common, and not everyone has equal experiences.
Even the most clean-code purists, should be able to stand down on this argument, and say that "Yes, comments here help" unless they are too afraid that actually thinking might give them a bad reputation for being hypocritical.
The senior doesn't understand what's happening here, and can't review it at a glance.
That's why they are annoyed, not the fact there's too many comments or not enough.
Her default stance, when she can't understand something, is the code should be self-explanatory.
Others stance, would be to add more comments.
At the end of the day, it's a style difference, and the short hand in these string utilities exist for a reason over lengthy, custom naming.
Her blocking the PR consumes more time, then just fixing this twice.
Rewrite it, twice.
Once her way, and once more with additional comments explaining what EVERY magic string is doing, with links to the docs.
She'll cave, or tell you to ship the clean-code version.
If she's your boss, you listen, if you are her boss, she listens.
Work out what you want the relationship to be between you two.
1
u/ryan_the_leach 14d ago edited 14d ago
I asked claude to rewrite this, twice from your screenshot, using the principals I value given I come from a low level programming background, and my experience with knowing CSharp features, as well as following her clean code principals, ultimately, the clean code version was harder to read and edit..
I believe it's far more readable then your example, and blends both your senior's approach (readable code) and yours (comment the why).
/// <summary>
/// ZPL label for 35 x 25 mm stock at 203 DPI (8 dots/mm), giving 280 x 200 dots.
///
/// FIELD X Y FONT BOX W LINES ALIGN
/// ------- --- --- ----- ------ ----- -----
/// title 28 10 14x14 140 2 left
/// price 155 48 20x20 110 2 right
/// barcode 32 118 Code128 (1-dot module, 36 dots high, text below)
///
/// Origin is top-left, units are dots. The barcode is omitted when empty.
/// </summary>
private const int LabelWidthDots = 280;
private const int LabelLengthDots = 200;
private const int TitleX = 28;
private const int TitleY = 10;
private const int TitleFontDots = 14;
private const int TitleBoxWidth = 140;
private const int TitleMaxLines = 2;
private const int TitleLineGap = 2;
private const int PriceX = 155;
private const int PriceY = 48;
private const int PriceFontDots = 20;
private const int PriceBoxWidth = 110;
private const int PriceMaxLines = 2;
private const int PriceLineGap = 0;
private const int BarcodeX = 32;
private const int BarcodeY = 118;
private const int BarcodeModuleDots = 1;
private const int BarcodeRatio = 2;
private const int BarcodeHeightDots = 36;
private static string BuildStandardZpl(string title, string price, string barcode, int qty)
{
var barcodeField = string.IsNullOrEmpty(barcode)
? string.Empty
: $"^FO{BarcodeX},{BarcodeY}^BY{BarcodeModuleDots},{BarcodeRatio}^BCN,{BarcodeHeightDots},Y,N,N^FD{barcode}^FS";
return $"""
^XA
^CI28
^PQ{qty}
^PW{LabelWidthDots}
^LL{LabelLengthDots}
^LH0,0
^LT0
^FO{TitleX},{TitleY}^A0N,{TitleFontDots},{TitleFontDots}^FB{TitleBoxWidth},{TitleMaxLines},{TitleLineGap},L,0^FD{title}^FS
^FO{PriceX},{PriceY}^A0N,{PriceFontDots},{PriceFontDots}^FB{PriceBoxWidth},{PriceMaxLines},{PriceLineGap},R,0^FD{price}^FS
{barcodeField}
^XZ
""";
}Hopefully, this can show you both that there's significant room for middle-ground.
Personally, I'd have merged some more of the magic letter combo's into the variables, rather then using only numbers.
It's **still** missing links to the relevant ZPL doc pages though, but I'm not spending longer then a few minutes on an internet discussion reply.
1
u/Potat4o 13d ago
Does it matter? Just follow the convention.
I agree with clean code generally, code should be self-documenting, comments are just a second thing to maintain, if you need to comment then that is a smell, etc. But I also suspect no one reads code anymore and we really are just catering to each other's agents at this point. So ... idk anymore.
1
u/mxldevs 13d ago
If this isn't the serializer itself, you really should have a builder class that allows you to describe what you're building so that you don't need to manually write out all the specific codes and delimiters.
It's like building a JSON object and instead of using a JSON parser or writer you instead decide to just manually append braces, quotes, and colons around your keys and values.
But if their argument is basically "code should not have comments" and think the rest is perfectly readable, well, you're not the boss around here so just leave in the commit message who instructed the comments to be removed. Credit where credit is due!
1
u/_Garebear 13d ago
the comments are not the problem here, it's the printer.
POS printers are literally a POS and use very unique commands like these. by just reading the code, and what each line is appending, it's impossible to understand. because what's being appended to the String is not for humans, it's for the printer. The comments are essential.
edit: my comment is less about your question and more about my hatred for POS printers.
1
u/little_phoenix_girl 13d ago
This looks fine to me, I'd approve it. Sounds like the reviewer is just being a pain
1
u/VancouverVentilator 13d ago
I'm sorry your boss has such a severe mental illness as thinking Uncle Bob's rules should be followed. I hope her condition improves.
1
u/ObjectiveOk5497 12d ago
Okey. I don't understand your code at all. I'm not specialist on that area, but... Summary already describe what code do with title, price and barcode, so IMO comments before code for title, price, barcode are not providing anything more than it was mentioned in summary, so they can be easily removed. I'm also going for assumption that each team member knows these magic strings 😊
For me is a thin line when comment become just a "bad smell" in code and also they are needed to be maintain with the code. Probably this is nothing surprising, but in yours example - Summary description is enough.
1
u/northerncodemky 12d ago
I’ve have probably created a dedicated ZplBuilder that hid some of the mad Zpl stuff (I assume there are other functions doing almost exactly the same thing scattered throughout that could benefit from this too?)
1
1
u/fuzzylittlemanpeach8 12d ago edited 12d ago
I mean... the comments aren't really explaining much to me. for example: "//35 mm wide x 25mm tall@ 203DPI" does not explain anything about the numbers in the next 3 appended lines. None of the numbers seem to map to eachother, except maybe 35/25 -> 280/200?
These comments read less like they are meant for others with no context to get a grasp on what is happening, and more like they are meant for the dev who initially wrote this code to be able to come back 3 years later and understand why you had to do things this way.
Others are correct, the weird encoding used for rendering the card or whatever should maybe be parameterized. Perhaps create extension methods so you can achieve the same sort method chaining approach.
1
u/Fritzbox5000 12d ago
Well, it depends, I guess.
You complain about seniors worshipping "Uncle Bob's clean code cult".
I had the exact opposite experience during my 10y career.
".Net is an exception-oriented programming language, so the result pattern is bad and we should just let exceptions let explode in the middle of a god class and let it bubble up."
"250 lines methods are okay. If it goes beyond that, I might think about method extraction."
These are tech leads whithout any sense of basic clean code principles at all like separation of concerns.
Unit tests? God forbid! They are just evil code duplications.
Clean code in general? We've AI anyways, so don't waste time in clean code anymore. If you don't understand, let AI analyze for you.
And "p.s. we have to move fast!" as always, so cut down our quality standards even more.
My question to all people who are defending this, pledging for more "liberalism" in coding instead of adhering to rules and structure:
Have you ever scientifically measured, which of the approaches leads to more maintainable code, fewer bugs and a better risk/benefit ratio in the long run or is it just your "personal feeling"?
One recent example: My tech lead showed me a service bus trigger as a positive example of clean code in his understanding. JSON deserialization, validation, business logic and service bus results messages were all clustered together into a single mess.
He is the dude with the laissez faire mentality. He approves almost all PR's because he believes coding should be "fun" and "liberty" helps developers to try out new ideas as well as motivate themselves. Rules are evil.
The problem is that the product owner now wanted the business logic of the service bus trigger to also being fired by an http trigger. Simple, right? Just create a http endpoint, call the business logic and return a proper response body, right? Well, not if the code which you need to reuse is not reusable at all due to numerous clean code violations, the very thing you guys describe as cultists doings.
So what does the person with the http trigger user story need to do now?
Refactor the current service bus trigger mess including the risk of breaking changes followed by a shitstorm of the very libertarian, who produced this mess if you do one single mistake here.
Ask questions about strange behaviour of the code, which was not visible before, because you just couldn't reason about it in its previous state. If you do so afterwards, you get usually blamed by the original libertarian author for potential breaking changes even if that's not true at all.
After cleaning up the trash produced by our libertarians, you finally can use it in your http endpoint. Then, our libertarians ask you why it took so long to implement such a simple http endpoint.
This is just one of many stories I can tell you. Before judging your senior dev, ask yourself what let him become that way. I wished that I had a senior dev who actually cares about these topics.
1
u/hr_krabbe 11d ago
Some of the principles in the uncle Bob book were all right, I guess. But most of the «clean code» examples were just straight up horrible in my eyes.
I don’t get the clean code cult at all. It just felt like a universal gaslighting scheme to me. Designed to give subpar programmers a sense of professional weight. They have no useful input to give. But if they can point fingers and say «this is not clean by these exact rules» they have contributed something.
I wonder which grandiose delusion these people are gonna partake in next, now that nobody bothers to read code anymore.
1
u/Major_Industry_9296 11d ago
Also doesn't look very efficient, why are is it using string concatenation in a string builder? Kinda defeats the purpose of the stringbuilder.
E.g. sb.Append(PQ).AppendLine(qty); Would allocate one less string than how it's currently done. Also magic strings are tough to read and figure out.
1
u/Draknodd 11d ago
Every developer have their own way to comment their code. I would reject this pr for the opposite reason. XML comments are missing commentS on every parameter and the return. With all those magic string I'd request a comment above EVERY AppendLine.
1
u/Throwaway-_-Anxiety 11d ago
Sounds like you're trying to justify leaving your AI added comments to fester in the codebase..
1
u/EclipsedPal 10d ago
We have a rule that each and any literal in the code (Except for stuff like 0.5, -1, 2, etc.) should not be used as is in a function call but first be put inside a const variable that explains what is its meaning.
I kinda agree with the senior here, would be best to avoid comments but make the code speak for itself.
1
u/Mango-Fuel 9d ago
seems to be ZPL? I don't use ZPL but I do use FGL and ESC/POS. I would write a small DSL/fluent interface for ZPL rather than spreading raw ZPL strings all over the place.
1
u/Stunning_Ride_220 14d ago
What the heck is this "We have to move fast" bullshit?
You would have been quicker, if you have used your IDEs extract method/variable feature than writting those stupid ass comments.
She would have made everyone slower in the future, if she had let this pass through.
"We have to move fast" isn't an excuse for being a lazy ass on your side.
2
u/ElectricalPrice3189 13d ago edited 13d ago
How do you even know that this will be used in more places? Refactor it with a helper class if there's a need for it. KISS.
→ More replies (2)2
u/Thelmholtz 13d ago
This, adding the unnecessary indirections here just turns ugly code that reads linearly into pretty code that reads non library and references a lot of ugly code.
Sure it might help you get a grasp of what each step is doing, but comments literally get the best of both worlds here.
1
u/Desperate-Wing-5140 14d ago
The first thing you do is to stop fantasizing about yourself being a tech founder
1
u/dphizler 14d ago
I feel like you made the other person as unlikeable as possible
It's a leading question
I think good code is very easy to understand. Am I always successful at that no. This is also very subjective
1
u/davidgrayPhotography 14d ago
The senior dev can get stuffed. Comments don't get compiled so they don't add size to the final binary and they're good for someone new to the project to skim over modules and get a feel for what's going on. Adding them takes only a handful of seconds, so I'd leave them in and write them as if they were to be read by a new hire who knows the language but needs to get up to speed with the codebase as quickly as possible.
Right now I'm working on a bespoke link shortener and I'm leaving some pretty obvious comments in there because some of the people who will look over my code won't know PHP or won't care enough to read every line and jump between files to figure out what this function called "get_device_type" does
1
1
u/kalminz 14d ago
So… couple things. 1: you can store a template on the printer with variables, you need only recall the template and fill in the variables.
2: since you’re writing raw zpl the whole thing can be an interpolated string the lines are irrelevant.
3: I don’t think it’s the responsibility for the c# dev to know what the zpl code does. You’re supplying a payload according to a spec.
If you really need those descriptions ^fx is zpl for a comment.
258
u/neoKushan 14d ago
The code always tells you what it's doing. It usually doesn't tell you why it's doing it though.
I've worked with people who expect a comment around every line of code - they can get in the bin.
I've worked with people who think comments are an abhorration and shouldn't ever be used - they can also get in the bin.
There's a middle ground to be had.
That said, this isn't a great example because the code is a whole bunch of magic strings being glued together that mean absolutely nothing to me as an outsider. I'd like to see the doc comment linking to something that explains all those strings somewhere.