Coding Horrors: You can gather all your technical debt into one easy framework!

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›1503 »

ultramiraculous: Nov 12, 2003; "No..."; Grimey Drawer

The Laplace Demon posted:

Yeah, really don't get why people scoff at that. You, the compiler and I all know it's equal to 1. Sometimes I put sizeof(char) to show the units are "bytes" or for symmetry with other statements. It doesn't make my code any less correct or efficient.

If anything I'd prefer sizeof(char) to be in there for clarity in most cases. It's not like it's going to cost anything when compiled, and it helps someone casually reading/reviewing your code to understand what you're getting at (hell it might even help flag a bug).

Also this:

Subjunctive posted:

Similarly spelling out 4 * 1024 * 1024 for 4MB, or 60 * 60 * 24 for seconds-in-a-day. Express the significance of something, not just the resulting value.

# ? Feb 11, 2015 14:00

Adbot: ADBOT LOVES YOU

# ? Jun 6, 2024 11:02

Coffee Mugshot: Jun 26, 2010; by Lowtax

TZer0 posted:

Anonymized code:
code:
std::vector<SomeClass> varName;
ASSERT_EQ(0, (int)varName.size());
Someone must be paranoid.

That snippet is strange for other reasons like casting std::vector.size() to an int and apparently distrusting the result of std::vector.empty().

# ? Feb 11, 2015 15:26

Karate Bastard: Jul 31, 2007; Soiled Meat

I know a great feature for a STL vector class: prefill new instances with ~~garbage~~ things that users often have use for! Like pi, user's passwords, pids of running processes, you name it! Or nothing, which can be situationally useful, but that can be solved by introspection at runtime.

All you need to do is check whether your new vector is empty before using it, and prefill it yourself if the compiler didn't do it for you.

# ? Feb 11, 2015 15:49

carry on then: Jul 10, 2010; by VideoGames
(and can't post for 10 years!)

Karate Bastard posted:

I know a great feature for a STL vector class: prefill new instances with ~~garbage~~ things that users often have use for! Like pi, user's passwords, pids of running processes, you name it! Or nothing, which can be situationally useful, but that can be solved by introspection at runtime.

All you need to do is check whether your new vector is empty before using it, and prefill it yourself if the compiler didn't do it for you.

I would like all String instances to come pre-filled with the text of John Galt's speech from Atlas Shrugged, please.

# ? Feb 11, 2015 16:39

Steve French: Sep 8, 2003

The Laplace Demon posted:

Yeah, really don't get why people scoff at that. You, the compiler and I all know it's equal to 1. Sometimes I put sizeof(char) to show the units are "bytes" or for symmetry with other statements. It doesn't make my code any less correct or efficient.

Voted Worst Mom posted:

I think what he meant was using the literal 1 is a magic number in comparison to using sizeof(char) even if you know they are the same thing.

I think some bits of context would be helpful:

1: the context of my statement, which is that it's generally preferable to use identifier expressions as operands to sizeof rather than types.
2: the context in which sizeof(char) most often appears (at least in my experience), which is usually not to manipulate single bytes.

code:

char *s = malloc(length * sizeof(char));

I'm not suggesting that it be replaced by 1.

If the intent is to have a specified number of bytes:

code:

char *s = malloc(size);

is completely clear; adding an extraneous sizeof operation does nothing to improve readability for anyone who understands that malloc operates on numbers of bytes.

If the intent is to allocate enough for a certain number of elements, which happen to be char, but could maybe possibly in theory change at some point (perhaps to wchar_t to hold larger characters, or short or int to hold larger numbers, you probably want:

code:

char *s = malloc(length * sizeof(*s));

In short, if the quantity you're working with is already expressed in bytes, multiplying it by the number of bytes in a char is guaranteed to be harmless from a correctness standpoint, but is hardly clearer or needed to be consistent with other code using sizeof on different types. If the quantity you're working with is some number of elements, there is probably an identifier that you can pass to sizeof instead that will give you more safety.

Steve French fucked around with this message at 17:13 on Feb 11, 2015

# ? Feb 11, 2015 17:11

Subjunctive: Sep 12, 2006; ✨sparkle and shine✨

I would write malloc(count * sizeof char) where I was going to use the resulting buffer as a char *, for symmetry with malloc(count * sizeof double) or similar. I wouldn't bounce something in code review for being less explicit, though.

# ? Feb 11, 2015 17:47

TheFreshmanWIT: Feb 17, 2012

Steve French posted:

If the intent is to allocate enough for a certain number of elements, which happen to be char, but could maybe possibly in theory change at some point (perhaps to wchar_t to hold larger characters, or short or int to hold larger numbers, you probably want:
code:
char *s = malloc(length * sizeof(*s));
In short, if the quantity you're working with is already expressed in bytes, multiplying it by the number of bytes in a char is guaranteed to be harmless from a correctness standpoint, but is hardly clearer or needed to be consistent with other code using sizeof on different types. If the quantity you're working with is some number of elements, there is probably an identifier that you can pass to sizeof instead that will give you more safety.

I don't do very much C (though I review a TON of it), but I DO tend to prefer the sizeof(char) when talking about a c string. The reason I prefer it is the potential bug in the code quoted above! If you write sizeof(char), it signifies you mean to use this as a cstring, and likely forgot a +1 to account for the null terminator. If you just do malloc(length), I have to deeper dive to determine whether you mean for s to point to a general byte array or a string.

# ? Feb 11, 2015 18:02

rjmccall: Sep 7, 2007; no worries friend; Fun Shoe

Voted Worst Mom posted:

That snippet is strange for other reasons like casting std::vector.size() to an int

tl;dr: This is mostly fine: it's avoiding a warning in a very slightly dumb way.

0 has type int (which is signed), and size() returns a size_t (which is unsigned). There's a very common warning, spelled -Wsign-compare in gcc and clang, which warns about comparing values of different signedness. That warning has an important exception when the operand that's changing signedness for the comparison is obviously non-negative; this is why -Wsign-compare doesn't warn about comparisons like someUnsignedNumber > 0. But in the gtest framework, ASSERT_EQ is a macro which invokes a template function, and that template function is where the comparison occurs, and it has no idea about the value of its arguments statically (in fact, the same template function will be shared for all invocations with an int and a size_t as operands, and some of those really might be problematic).

So the extra abstraction causes this warning, and the test is avoiding it by making sure the template sees the same type on both sides. It would be slightly better to instead make the expected value unsigned instead of signed by writing it as 0U or casting it to size_t; this would avoid the (completely theoretical in this case, still basically unimaginable elsewhere) possibility that size() could return an exact multiple of 2^32 which, when truncated to int, would yield 0. But I can understand the instinct to add the syntactic burden to the more complex operand and leave the expected value a nice, clean literal.

# ? Feb 11, 2015 18:16

Steve French: Sep 8, 2003

TheFreshmanWIT posted:

I don't do very much C (though I review a TON of it), but I DO tend to prefer the sizeof(char) when talking about a c string. The reason I prefer it is the potential bug in the code quoted above! If you write sizeof(char), it signifies you mean to use this as a cstring, and likely forgot a +1 to account for the null terminator. If you just do malloc(length), I have to deeper dive to determine whether you mean for s to point to a general byte array or a string.

I was not advocating malloc(length) in the case of a buffer meant to hold a string with a particular number of characters, but rather in the case of a buffer with a particular number of bytes. It will hopefully be clear in context whether s is meant to point to a string (perhaps a nearby call to a string function, or well-named variables). But let's assume for a moment that it's not: using sizeof(char) is a good indicator that you're not meaning to allocate a buffer with a particular number of bytes, but so is sizeof(*s), and neither makes it clear that you're allocating a buffer to hold a cstring rather than an array of 1-byte signed integers.

Humor me for a moment, and show me exactly how you personally would allocate a cstring str to hold length characters?

# ? Feb 11, 2015 18:52

Soricidus: Oct 21, 2010; freedom-hating statist shill

Using sizeof(char) is like splitting an infinitive. It's perfectly legitimate, and there are times when it makes your intention clearer, but you should keep in mind that a lot of people will judge you for it.

# ? Feb 11, 2015 21:26

Kilson: Jan 16, 2003; I EAT LITTLE CHILDREN FOR BREAKFAST !!11!!1!!!!111!

I bet people are worried about the weirdo machines where a char is bigger than 8 bits, but they don't realize that sizeof(char) is required to return 1 no matter what.

# ? Feb 11, 2015 22:57

Edison was a dick: Apr 3, 2010; direct current only

Kilson posted:

I bet people are worried about the weirdo machines where a char is bigger than 8 bits, but they don't realize that sizeof(char) is required to return 1 no matter what.

They need to check CHAR_BIT for weird architectures like that.

# ? Feb 11, 2015 23:32

Knyteguy: Jul 6, 2005; YES to love
NO to shirts; Toilet Rascal

This is one of mine from a year ago or so:

C# code:

@if (didSomething == false)
{
    // check digit
    List<string> manufacturerNames = new List<string>();
    List<string> currentFilters = new List<string>();

    try
    {
        didSomething = true;
    	...

# ? Feb 12, 2015 00:49

sarehu: Apr 20, 2007; (call/cc call/cc)

One of mine, recently:

code:

uint32_t max_size = 0;
for (size_t i = 0; i < ...) {
  size_t size;
  ...;
  if (max_size < size) {
    size = max_size;
  }
}

# ? Feb 12, 2015 04:44

Yaoi Gagarin: Feb 20, 2014

Knyteguy posted:

This is one of mine from a year ago or so:

C# code:

@if (didSomething == false)
{
    // check digit
    List<string> manufacturerNames = new List<string>();
    List<string> currentFilters = new List<string>();

    try
    {
        didSomething = true;
    	...

Forgive me for being an idiot, but what's the horror here? Is it just that you're setting the flag before you actually do something?

# ? Feb 12, 2015 08:48

Vanadium: Jan 8, 2005

What does the @ do there?

# ? Feb 12, 2015 14:21

Pollyzoid: Nov 2, 2010; GRUUAGH you say?

Looks like Razor, so it just starts a C# (or VB.Net) code block.

# ? Feb 12, 2015 14:48

Bognar: Aug 4, 2011; I am the queen of France; Hot Rope Guy

VostokProgram posted:

Forgive me for being an idiot, but what's the horror here? Is it just that you're setting the flag before you actually do something?

The real crime is comparing a boolean to false.

# ? Feb 12, 2015 15:21

Polio Vax Scene: Apr 5, 2009

But readability!!!

# ? Feb 12, 2015 16:46

Mogomra: Nov 5, 2005; simply having a wonderful time

Bognar posted:

The real crime is comparing a boolean to false.

I do it all the time...

Because I'm forced to used languages without strong typing at work. :shepicide:

# ? Feb 12, 2015 16:55

ijustam: Jun 20, 2005

the horror is doing business logic in your views

# ? Feb 12, 2015 17:07

Knyteguy: Jul 6, 2005; YES to love
NO to shirts; Toilet Rascal

There's arguments for comparing to false instead of using !, but yea there's definitely some horrors in there.

The comment sucks and doesn't mean anything, the boolean variable name isn't descriptive at all (it could have been bool filtersAreSet or something), the view has business logic in it, and there's more I'm sure. The whole view could use some refactoring, because it's a mess. Luckily there's a bug in there (surprise) and I'll get the opportunity to do so soon.

Also the controller that creates the view: https://nopcommerce.codeplex.com/So...ogController.cs

Knyteguy fucked around with this message at 17:29 on Feb 12, 2015

# ? Feb 12, 2015 17:24

Spatial: Nov 15, 2007

code:

static_assert( CHAR_BIT == 8, "Here's a nickle kid, go buy yourself a real computer." );

Not a horror, just made me laugh.

# ? Feb 12, 2015 17:46

Polio Vax Scene: Apr 5, 2009

That's pretty good, I might have to steal it.

# ? Feb 13, 2015 00:44

raminasi: Jan 25, 2005; a last drink with no ice

Fix the spelling of "nickel" first :argh:

# ? Feb 13, 2015 00:52

Spatial: Nov 15, 2007

GrumpyDoctor posted:

Fix the spelling of "nickel" first

Marked as critical!

# ? Feb 13, 2015 01:32

pokeyman: Nov 26, 2006; That elephant ate my entire platoon.

Bognar posted:

The real crime is comparing a boolean to false.

I do a double-take in Swift every time I compare an optional boolean to true or false. It's necessary but it still feels weird.

# ? Feb 13, 2015 03:23

Bruegels Fuckbooks: Sep 14, 2004; Now, listen - I know the two of you are very different from each other in a lot of ways, but you have to understand that as far as Grandpa's concerned, you're both pieces of shit! Yeah. I can prove it mathematically.

Spatial posted:

code:
static_assert( CHAR_BIT == 8, "Here's a nickle kid, go buy yourself a real computer." );
Not a horror, just made me laugh.

# ? Feb 13, 2015 04:57

Linear Zoetrope: Nov 28, 2011; A hero must cook

quote:

[The] O(n) is log n

code:

void dynArrayAddAt (struct DynArr * da, int index, TYPE val) {
  assert(index >0)
  assert(da !=0)
  if(index >= capacity){
    struct DynArr *new = (struct DynArr*)malloc(sizeof(struct DynArr))
    new->capacity = da->capacity*2;
    for(int i=index; index<capacity; i++)
      new->data[i] = da->data[i];
    da = new
    free(new);
  }
  for(int i=index; index<capacity-1; i++){
    da->data[i+1] = da->data[i];
  }
  da->data[index] = value;
}

free(new) Whoops

Also, still a bug by reallocating the struct instead of the backing array, but I have to give full credit for that because the instructor's solutions make the same error. And his loop to shift the elements doesn't do what he thinks it does. To be slightly fair, this is a midterm so it's effectively whiteboard coding.

Linear Zoetrope fucked around with this message at 20:13 on Feb 14, 2015

# ? Feb 14, 2015 20:00

Karate Bastard: Jul 31, 2007; Soiled Meat

Jsor posted:

whiteboard coding.

Any of you whippersnappers actually got to do this?

# ? Feb 15, 2015 00:28

KernelSlanders: May 27, 2013; Rogue operating systems on occasion spread lies and rumors about me.

Karate Bastard posted:

Any of you whippersnappers actually got to do this?

It takes practice like anything else, but there's nothing fundamentally hard about it.

# ? Feb 15, 2015 02:53

Che Delilas: Nov 23, 2009; FREE TIBET WEED

KernelSlanders posted:

It takes practice like anything else, but there's nothing fundamentally hard about it.

Specifically practice choosing an appropriate size so that your code doesn't look like the board got into a head-on collision with something.

# ? Feb 15, 2015 04:15

Jabor: Jul 16, 2010; #1 Loser at SpaceChem

var hypotenuse = sqrt(opposite * opposite + adj^{_{acent * adja_cent)}}

# ? Feb 15, 2015 04:40

Linear Zoetrope: Nov 28, 2011; A hero must cook

The most important part of software engineering is designing the crumple zones.

# ? Feb 15, 2015 04:55

1337JiveTurkey: Feb 17, 2005

Jsor posted:

The most important part of software engineering is designing the crumple zones.

Now you see the hidden genius of Java.

# ? Feb 15, 2015 04:56

TheFreshmanWIT: Feb 17, 2012

Steve French posted:

I was not advocating malloc(length) in the case of a buffer meant to hold a string with a particular number of characters, but rather in the case of a buffer with a particular number of bytes. It will hopefully be clear in context whether s is meant to point to a string (perhaps a nearby call to a string function, or well-named variables). But let's assume for a moment that it's not: using sizeof(char) is a good indicator that you're not meaning to allocate a buffer with a particular number of bytes, but so is sizeof(*s), and neither makes it clear that you're allocating a buffer to hold a cstring rather than an array of 1-byte signed integers.

Humor me for a moment, and show me exactly how you personally would allocate a cstring str to hold length characters?

I tend to stick to C++ these days, but I do a bunch of code reviews on it. This is the pattern that makes reviewing it most clearly:

code:

str = (char*)malloc(sizeof(char) * (length + 1));

The sizeof(char) makes it really clear to me when reviewing that this is a string, so when someone leaves off the +1, it is pretty obvious that there is likely a bug here. I'll also note that I'm a bit less strict when reviewing about the parends around length +1 however.

# ? Feb 15, 2015 05:30

ultramiraculous: Nov 12, 2003; "No..."; Grimey Drawer

Karate Bastard posted:

Any of you whippersnappers actually got to do this?

Yes, and I stressed out so hard about it that I all but forgot how to talk for the soft questions.

# ? Feb 15, 2015 07:40

Soricidus: Oct 21, 2010; freedom-hating statist shill

TheFreshmanWIT posted:

I tend to stick to C++ these days, but I do a bunch of code reviews on it. This is the pattern that makes reviewing it most clearly:
code:
str = (char*)malloc(sizeof(char) * (length + 1));
The sizeof(char) makes it really clear to me when reviewing that this is a string, so when someone leaves off the +1, it is pretty obvious that there is likely a bug here. I'll also note that I'm a bit less strict when reviewing about the parends around length +1 however.

If only programming languages had some way to assign names to patterns like this, so you could have something like str = malloc_str(length) and it would be explicit and simple and you wouldn't have to worry about using redundant multiplications by obfuscated constants to convey your intent

# ? Feb 15, 2015 14:04

Internet Janitor: May 17, 2008; "That isn't the appropriate trash receptacle."

I dunno, Soricidus- that sounds like overhead talk. My PDP-7 only has 4 kilowords of memory to work with and an indulgent 10 character identifier will take quite a toll on the symbol table.

# ? Feb 15, 2015 14:52

Adbot: ADBOT LOVES YOU

# ? Jun 6, 2024 11:02

Bonfire Lit: Jul 9, 2008; If you're one of the sinners who caused this please unfriend me now.

Internet Janitor posted:

I dunno, Soricidus- that sounds like overhead talk. My PDP-7 only has 4 kilowords of memory to work with and an indulgent 10 character identifier will take quite a toll on the symbol table.

don't worry, it'll be truncated to 8 characters anyway so it's all good

# ? Feb 15, 2015 17:09

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›1503 »