Meteor

@Maddus Mattus: Could you tell us why it wouldn't pass?

Herbie

@Dr Herbie: don't know about Maddus, but in my company that combination would be enough for being flogged in public on the following counts:

- use of hardcoded constants, aggravated by the fact that they are completely arbitrary and not self-evident;

- returning strings directly (everybody speaks English, right?);

- strong evidence that you are building your message on the fly (again: not every language structures sentences the same way);

- (minor) assuming that it's ok to mix 0 and false (or null). Mangling intent just to save a few keystrokes is just sloppy.

@Blue Ink: I hadn't noticed the !likes to be honest (which shows it is indeed bad practice), and I'm so used to writing bespoke software that I've completely lost the habit of language agnosticism.

In this case I think the arbitrary constants are fine given the context; 'some' and 'a lot' are arbitrary amounts and I immediately understood what the code was doing. 

Herbie

@Dr Herbie: I agree that the function is readable, my problem with arbitrary constants is that - being arbitrary - they are just "semi-constants" that will change at the whim of upper management. And you'll find yourself sifting through a large solution you don't know much about, searching for all those 20's that need to become 25's. Been there, it's not pretty.

Just noticed that negative values will map to "a lot". I wish this code were from some home-banking system.

Personally, I don't like using multiple returns within a function. It seems like a poor way to implement flow-control.

-Josh

That code snippet is common practice with a personal (non-corporate) website. While it does look ugly, I don't see the point in critiquing it so much as it probably isn't even intended to scale and most likely isn't meant to be shown to more than just a few fellow coders. Honestly, except for the !this.likes, I couldn't tell what was wrong with it until you guys mentioned code reviews and upper management.

@Blue Ink: I think you're over-engineering with the introduction of constants -- there is only 1 instance of each constant here because the procedure is small and very well defined. And searching for a single instance of '20' is just as easy and trying to figure out what the constant will be called.

Herbie

Just curius, would this pass a code review (it's c):

loop:
    switch(ctab[c]) {

    case 125:    /* newline */
        line++;

    case 126:    /* white space */
        c = getchar();
        goto loop;

    case 0:        /* EOF */
        eof++;
        return(0);

    case 40:    /* + */
        return(subseq(c,40,30));

    case 41:    /* - */
        return(subseq(c,subseq('>',41,50),31));

    case 80:    /* = */
        if (subseq(' ',0,1)) return(80);
        c = symbol();
        if (c>=40 & c<=49)
            return(c+30);
        if (c==80)
            return(60);
        peeksym = c;

[It's from here: http://minnie.tuhs.org/cgi-bin/utree.pl?file=V3/c/c00.c]

@fanbaby: Gotos, relying on falling through cases, numbers scattered throughout the source. It's not exactly the most readable code ever written. Not unsuprising given where it's from, but I think it could definitely be written better.

@AndyC: that made me smile

@fanbaby: After 10 seconds, I still couldn't figure out exactly what it was supposed to be doing, so it fails code review at the first hurdle.

Herbie

1 hour ago, fanbaby wrote

@AndyC: that made me smile

 

that code is 30 years old, things have definitely improved, so not really worth spending too much time on apart from historic curiosity.

Warren looks an awful lot like SpectateSwamp. Too bad the sample wasn't written in VB6.

-Josh

3 hours ago, blowdart wrote

Actually I don't mind multiple return points in code, safer in a lot of cases as you don't have to keep checking some bool to see if you need to continue, or ending up with deep nested conditions.

I do the same thing. I know it's technically more correct to have a single point of return in a method, but I find that my code becomes much more readable if I just have multiple returns instead of a bunch of nested conditions and checking variables. I prefer readability over.. beauty, or whatever you'd call it, so multiple returns it is.

If you're checking method arguments...I'd rather just have a chunk of checks at the beginning returning false or throwing errors instead of setting a variable/nesting if statements.

7 hours ago, Bas wrote

*snip*

I do the same thing. I know it's technically more correct to have a single point of return in a method, but I find that my code becomes much more readable if I just have multiple returns instead of a bunch of nested conditions and checking variables. I prefer readability over.. beauty, or whatever you'd call it, so multiple returns it is.

Well, I don't think it's beauty, since beauty is what Richie or Thomson or Rob Pike write, and that is the same as you write.