Code review requested: Fruit inventory coding challenge

[cartman82]

In this thread:

OP: Hey guys, can someone offer some constructive criticism about my code?

WTDWTF: (crickets)

OP: By the way, I also happened to pick one arbitrary code style over another.

WTDWTF: (50 replies)

[accalia]

@cartman82 said in Code review requested: Fruit inventory coding challenge:

In this thread:

OP: Hey guys, can someone offer some constructive criticism about my code?

WTDWTF: (crickets)

OP: By the way, I also happened to pick one arbitrary code style over another.

WTDWTF: (50 replies)

welcome to WTDWTF.

[accalia]

@masonwheeler said in Code review requested: Fruit inventory coding challenge:

@accalia

that's decidedly unsafe flying. Who's the pilot of that aircraft. it is my civil duty to report them to the FAA as their flying is unsafe, not only for their passengers but to all those people within a ten mile radius of the plane they fly

[Dreikin]

@cartman82 said in Code review requested: Fruit inventory coding challenge:

In this thread:

OP: Hey guys, can someone offer some constructive criticism about my code?

WTDWTF: (crickets)

Speaking of which, here's the current state:

GitHub - mrwirth/CodingChallenge-FruitInventory at code-reviews/TestSource

Presents a coding challenge with several levels of difficulty. - GitHub - mrwirth/CodingChallenge-FruitInventory at code-reviews/TestSource

It's got most-all (I think) of @cartman82's suggested changes (though not in exactly the same manner) and has been linted to the displeasure of several people, but no testing yet.

[Dreikin]

@accalia said in Code review requested: Fruit inventory coding challenge:

@masonwheeler said in Code review requested: Fruit inventory coding challenge:

@accalia

that's decidedly unsafe flying. Who's the pilot of that aircraft. it is my civil duty to report them to the FAA as their flying is unsafe, not only for their passengers but to all those people within a ten mile radius of the plane they fly

No no no, look closer: clearly they're flying in a terrible fog and just dodged colliding with us, the viewers. As a major commercial passenger aircraft, they're almost certainly flying a path given to them by air traffic control. Indeed, their pilot should be commended for their keen observational skills and quick reflexes, saving the passengers of both planes from a fiery, grindy death. The air traffic controller that put those two aircraft on a collision course, however, is clearly not one for keeping their job.

[blakeyrat]

Ok here's a quick .NET code review. Gonna be harsh, so I hope you're ready.

It's sloppy to include usings you're not actually using. This is a pet peeve of mine, since Visual Studio is good enough to JUST FUCKING FIX IT FOR YOU (except the 1 in 100,000,000 times you have a naming collision), but doesn't. Because it's a jerk.

EDIT: By the way, just to hammer-in how stupid this is, the compiler is also smart enough to not bother linking usings that don't get used. So the compiler knows how to deal with them, the IDE knows how to deal with them, but both still make it the user's problem to deal with them. Ugh.

var dataUrl = ConfigurationManager.AppSettings["variedEverythingSource"];

Kudos on putting this in AppSettings (correct), but "variedEverythingSource" is a terrible name for it.

var fruits = from fruit in Fruits.GetFruits(dataUrl).DeduplicateBy(x => x.Name)

• I have a very strong preference for the functional style of writing LINQ, and I think you'll find most C# shops prefer that nowadays.
• DeduplicateBy appears to just reproduce the built-in Distinct LINQ operator. Unless you're building it as a learning exercise, it's far better to rely on built-ins instead of writing your own.

where fruit.Price >= 30

You're comparing a decimal to an int32. This might be my personal pet peeve, but this is one of the places where C# is not strongly-typed enough, and since you can easily define a decimal constant, you should be doing that instead of relying on the compiler to fix it for you. (Put another way: write 30.0m.)

        public string Name { get; }
        public decimal Price { get; }
        public decimal Amount { get; }
        public string AmountUnit { get; }
        public decimal AmountInGrams { get; }

Doesn't really matter in your case, but I think the standard here would be to do:

public string Name { get; protected set; }

in case you ever subclass.

EDIT: BTW if your intention is that this class can never be subclassed for whatever reason, you can use the sealed keyword to instruct the compiler to enforce that. I wouldn't recommend that, though. Better to make the class subclass-able and leave it unsealed.

I've been bitten in the ass before by framework classes that were sealed for no good reason (looking at you, WebAPI2!) when I really needed to subclass them-- the workaround resulted in tons of nasty duplicated code.

private static decimal NormalizeWeight(decimal amount, string amountUnit)

This probably should protected, again in case you ever subclass.

            if (amountUnit == "g") { return amount; };
            if (amountUnit == "kg") { return amount * 1000; };

I really don't like amountUnit being a string here. Consider making it an enum, with "g" and "kg" being two of the enum values. Parse from a string into the enum when you load the value, keep it in an enum from then on.

throw new ArgumentOutOfRangeException("amountUnit", amountUnit, "Only g and kg are currently supported.");

This is good, a lot of people would miss this.

    public static class Fruits

Two classes in one file will be flagged by most code reviewers. I'm kind of ok with it, but in this specific case I still think you'd be better off either:

1. Adding those static methods to the existing Fruit class, or
2. Splitting the Fruits class into its own file.

        public static IEnumerable<Fruit> GetFruits(string url)
        {
            return FetchData(url).SplitData().ParseData();
        }

        private static string FetchData(string url)
        {
            using (var client = new System.Net.WebClient())
            {
                return client.DownloadString(url);
            }
        }

For a small project like this, eh. But... generally speaking, I don't like my Fruits class having to know about System.Net.WebClient() to do its job. Seems like it has too many concerns here-- why not have the static class parse the string regardless of where the string originally comes from?

        private static (string, IEnumerable<string>) SplitData(this string data)
        {
            var rows = data.Split('\n');
            var headerRow = rows[0];
            var dataRows = rows.Skip(2);
            return (headerRow, dataRows);
        }

Hard-coding a 2-line header makes this parser more fragile than it needs to be. Also there's no error-checking if the input has fewer than 2 lines.

            var names = FindIndexes("Product name", data.header);
            var prices = FindIndexes("Unit price", data.header);
            var amounts = FindIndexes("Amount", data.header);

These are not good names. The variable names does not contain names, but numbers.

            var numberPattern = new Regex(@"[0-9.]+");
            var unitPattern = new Regex(@"[A-Za-z]+");

Fuck RegEx. char.IsDigit() and related could help here.

var amount = decimal.Parse(numberPattern.Match(amountText).Value);

decimal.Parse() can throw. You're not really handling that potential error in a very organized way. (For a throwaway project like this, probably not a big deal.)

You're not providing a culture to decimal.Parse(), you probably should be. Not everybody uses the same punctuation to separate digits. (Again: not a big deal for a throwaway project, but.)

[Weng]

@blakeyrat That said, removing the generic collections and tasks usings is outright evil.

I tend not to remove any of the default usings because nobody ever thinks about them to remember off the top of their head what namespace those features are in.

[blakeyrat]

@weng Collections and tasks are no big dealios-- just right-click and say "add this reference" or whatever it's named now.

I have been bit in the ass by having the Linq using removed, then wondering why the Linq functions don't show up in my autocomplete.

But still, I'd say 90% of C# shops will ding you in a code review if you have usings that aren't used. Even if they're System.Linq. And of course use your brain, because obviously a file that's just a POCO isn't going to need System.Linq ever.

EDIT: I also noticed that he's splitting on \n... that should probably be Environment.NewLine although I could see arguments either way, frankly... since the environment the file's coming from is not the environment the program's running in. Just, if you're in an interview situation, be able to explain that things like "what starts a new line on this computer?" are part of the Environment in C#, and ignoring that and hard-coding values could break your program for no good reason on some future OS.

[Weng]

@blakeyrat said in Code review requested: Fruit inventory coding challenge:

Collections and tasks are no big dealios-- just right-click and say "add this reference" or whatever it's named now

Oh yeah forgot they added that.

[ben_lubar]

@jaloopa said in Code review requested: Fruit inventory coding challenge:

@weng said in Code review requested: Fruit inventory coding challenge:

I can see the JS community adopting this enthusiastically.

Probably not. This is a classic example of why ASI is bad.

It would be translated into something like

var x =
"my return value";

return;
x;

which also goes to show why it's a bad idea to have such a loose language as Javascript, as in C#, the second one wouldn't compile due to the different return type and the x doing nothing

Go also inserts semicolons, but that code gives 3 errors. The correctly formatted version of the code gives 0 errors.

[ben_lubar]

@blakeyrat said in Code review requested: Fruit inventory coding challenge:

just right-click and say "add this reference" or whatever it's named now

That feature is great, but my Go IDE automatically adds and removes imports to make the code compile when I save a file. I wish Visual Studio did it automatically instead of making me tell it to do its job every time.

[djls45]

@ben_lubar said in Code review requested: Fruit inventory coding challenge:

automatically adds and removes imports to make the code compile when I save a file

Does it give you a choice in the case of multiple possibilities?
It can be bad if it picks the wrong thing to import. I've had trouble with Eclipse picking the wrong Java class from a completely different package before, making me have to manually delete that import and choose the correct one.

[CarrieVS]

@blakeyrat said in Code review requested: Fruit inventory coding challenge:

The stuff that clogs up my dryer is a physical substance. My code is not at all physical.

It's what we call a metaphor.

[asdf]

@blakeyrat said in Code review requested: Fruit inventory coding challenge:

And the command to rename a file is "mv" which stands for move.

Toby Faire, mv actually moves files as well. The only problem is that it doesn't have an alias named "rename" by default.

[asdf]

@cartman82 said in Code review requested: Fruit inventory coding challenge:

In this thread:

OP: Hey guys, can someone offer some constructive criticism about my code?

WTDWTF: (crickets)

OP: By the way, I also happened to pick one arbitrary code style over another.

WTDWTF: (50 replies)

This probably applies to every programming-related forum ever. Programmers love to argue about irrelevant shit.

Although, admittedly, it's not completely irrelevant in this case.

[Zecc]

@blakeyrat said in Code review requested: Fruit inventory coding challenge:

@masonwheeler And the command to rename a file is "mv" which stands for move.

Which is a great metaphor, because if something gets renamed in real life (for example, Istanbul to Constantinople), it also changes location. Everybody knows that.

I want to agree with you, but "repath" would be a stupid name. "mv" is.. tolerable.

[Jaloopa]

@zecc said in Code review requested: Fruit inventory coding challenge:

"mv" is.. tolerable.

maybe if you're familiar enough with it to know that it means move, be aware of the related but conceptually different additional use to rename, and forget that it didn't make much sense when you first saw it

[sloosecannon]

@djls45 said in Code review requested: Fruit inventory coding challenge:

@ben_lubar said in Code review requested: Fruit inventory coding challenge:

automatically adds and removes imports to make the code compile when I save a file

Does it give you a choice in the case of multiple possibilities?
It can be bad if it picks the wrong thing to import. I've had trouble with Eclipse picking the wrong Java class from a completely different package before, making me have to manually delete that import and choose the correct one.

I know IntelliJ on "automatically insert references" mode will insert unambiguous references, and give a syntax error when it's ambiguous (giving the option to fix the issue with alt+enter (ctrl+, for VS types)

[Dreikin]

@sloosecannon said in Code review requested: Fruit inventory coding challenge:

@djls45 said in Code review requested: Fruit inventory coding challenge:

@ben_lubar said in Code review requested: Fruit inventory coding challenge:

automatically adds and removes imports to make the code compile when I save a file

Does it give you a choice in the case of multiple possibilities?
It can be bad if it picks the wrong thing to import. I've had trouble with Eclipse picking the wrong Java class from a completely different package before, making me have to manually delete that import and choose the correct one.

I know IntelliJ on "automatically insert references" mode will insert unambiguous references, and give a syntax error when it's ambiguous (giving the option to fix the issue with alt+enter (ctrl+, for VS types)

Why does it change the shortcut if it's a type Visual Studio would reference?

[sloosecannon]

@dreikin said in Code review requested: Fruit inventory coding challenge:

@sloosecannon said in Code review requested: Fruit inventory coding challenge:

@djls45 said in Code review requested: Fruit inventory coding challenge:

@ben_lubar said in Code review requested: Fruit inventory coding challenge:

automatically adds and removes imports to make the code compile when I save a file

Does it give you a choice in the case of multiple possibilities?
It can be bad if it picks the wrong thing to import. I've had trouble with Eclipse picking the wrong Java class from a completely different package before, making me have to manually delete that import and choose the correct one.

I know IntelliJ on "automatically insert references" mode will insert unambiguous references, and give a syntax error when it's ambiguous (giving the option to fix the issue with alt+enter (ctrl+, for VS types)

Why does it change the shortcut if it's a type Visual Studio would reference?

VS types is a property of Person not Code :P

[blakeyrat]

@asdf said in Code review requested: Fruit inventory coding challenge:

Toby Faire, mv actually moves files as well. The only problem is that it doesn't have an alias named "rename" by default.

Moving and renaming are two entirely different operations. Moving a file shouldn't rename it. "My Ford Fusion is now called a Mord Musion because I moved it into Idaho." And renaming shouldn't move, dumb analogy already given.

The problem is Unix was designed stupidly by morons and since then generations of morons haven't fixed any of its problems, even the most basic ones.

[Yamikuronue]

@blakeyrat said in Code review requested: Fruit inventory coding challenge:

Moving a file shouldn't rename it

Giving a file a different fully-qualified name has nothing to do with moving it; my server hardware doesn't get put in a truck and shipped to another state

[blakeyrat]

@yamikuronue They chose the file metaphor, not me.

[ObjectMike]

@blakeyrat said in Code review requested: Fruit inventory coding challenge:

@asdf said in Code review requested: Fruit inventory coding challenge:

Toby Faire, mv actually moves files as well. The only problem is that it doesn't have an alias named "rename" by default.

Moving and renaming are two entirely different operations. Moving a file shouldn't rename it. "My Ford Fusion is now called a Mord Musion because I moved it into Idaho." And renaming shouldn't move, dumb analogy already given.

The problem is Unix was designed stupidly by morons and since then generations of morons haven't fixed any of its problems, even the most basic ones.

It works if you think of its current directory as a street and renaming as changing the house number. Or, putting the house on a trailer and moving it to another property.

[blakeyrat]

@mikehurley Right and the word lint works as long as you think of minor style errors as pieces of your old trousers clogging up the air filter in your gas dryer.

The problem is: nobody thinks that way.

Also: they specifically used the term "file" which guarantees nobody thinks that way.

[ObjectMike]

@blakeyrat said in Code review requested: Fruit inventory coding challenge:

@mikehurley Right and the word lint works as long as you think of minor style errors as pieces of your old trousers clogging up the air filter in your gas dryer.

The problem is: nobody thinks that way.

Also: they specifically used the term "file" which guarantees nobody thinks that way.

You do realize that you tend to take things way too literally, right? I say that as somebody who tends to take things way too literally. Plus I don't even feel like I'm calling the kettle black.

We work with abstract constructs. Humans naturally want metaphors from the real world. Metaphors are not meant to be perfect analogies. Hell, analogies are not even meant to be perfect. These things are meant to help. If they don't help you, fine. That doesn't detract from them since apparently they help most people.

[blakeyrat]

@mikehurley said in Code review requested: Fruit inventory coding challenge:

We work with abstract constructs. Humans naturally want metaphors from the real world. Metaphors are not meant to be perfect analogies. Hell, analogies are not even meant to be perfect.

RIght; but the Unix people aren't even fucking trying.

@mikehurley said in Code review requested: Fruit inventory coding challenge:

That doesn't detract from them since apparently they help most people.

"Most people" would immediately know what a "linter" is based on the lint-in-dryer metaphor? Citation needed.

[ben_lubar]

@djls45 said in Code review requested: Fruit inventory coding challenge:

Does it give you a choice in the case of multiple possibilities?

I think it favors the shortest package import path if multiple packages have the same name and all the functions and types needed to make the code compile. That's usually what you want, since zip.OpenReader("foo.zip") generally means you want the archive/zip package from the standard library.

[boomzilla]

@blakeyrat said in Code review requested: Fruit inventory coding challenge:

The stuff that clogs up my dryer is a physical substance. My code is not at all physical.

I'm reminded of PCU:

Droz: All right, what's your major?
Phys. Ed. Major: Phys. Ed.
Droz: Phys. Ed? All right, stud, you're out of my room. Seriously, get out.

Seriously, you're worse than a physical education major with that dumb line.

[Jaloopa]

@boomzilla said in Code review requested: Fruit inventory coding challenge:

PCU

Police Credit Union?

[cartman82]

@blakeyrat said in Code review requested: Fruit inventory coding challenge:

I have a very strong preference for the functional style of writing LINQ, and I think you'll find most C# shops prefer that nowadays.

The special sql-like keywords was IMO the worst design decision they made about C# after 1.0.

For a small project like this, eh. But... generally speaking, I don't like my Fruits class having to know about System.Net.WebClient() to do its job. Seems like it has too many concerns here-- why not have the static class parse the string regardless of where the string originally comes from?

Yes, business logic should definitely be separate from IO. That goes for any language.

Hard-coding a 2-line header makes this parser more fragile than it needs to be. Also there's no error-checking if the input has fewer than 2 lines.

Having a 2 line header is a reasonable assumption to make about the data format.

Since he is detecting columns based on headers, it's not like he'd even be able to parse the data if the header changed format.

I wouldn't ding him for this even a little bit.

Fuck RegEx. char.IsDigit() and related could help here.

I would use one regex for the amount column, then extract both amount and unit at once. You can do it character by character, but that's adding needless micro-optimization to the determent of code readability.

I would parse the price using substring.

decimal.Parse() can throw. You're not really handling that potential error in a very organized way. (For a throwaway project like this, probably not a big deal.)

Good point. Probably best to catch errors per row while parsing and skip invalid rows, with some kind of error reporting.

[masonwheeler]

@cartman82 said in Code review requested: Fruit inventory coding challenge:

The special sql-like keywords was IMO the worst design decision they made about C# after 1.0.

Why?

[Jaloopa]

@cartman82 said in Code review requested: Fruit inventory coding challenge:

The special sql-like keywords was IMO the worst design decision they made about C# after 1.0.

I found it a good way to get into LINQ when I knew SQL but not functional programming idioms. I wouldn't use it now, but it has its place

[cartman82]

@masonwheeler said in Code review requested: Fruit inventory coding challenge:

Why?

They severely polluted the global namespace, and in the end, everyone ended up using extension method syntax anyway. Terrible.

[cartman82]

@jaloopa said in Code review requested: Fruit inventory coding challenge:

I found it a good way to get into LINQ when I knew SQL but not functional programming idioms. I wouldn't use it now, but it has its place

That sounds like a very obscure use case. Certainly not worth adding like 15 new keywords.

[masonwheeler]

@cartman82 said in Code review requested: Fruit inventory coding challenge:

polluted the global namespace

Meaningless buzzword alert! Why does anyone care? I've never seen any use case for someone wanting to use one of those words unqualified. No one wants a new while() or namespace from...

[cartman82]

@masonwheeler said in Code review requested: Fruit inventory coding challenge:

Meaningless buzzword alert! Why does anyone care? I've never seen any use case for someone wanting to use one of those words unqualified. No one wants a new while() or namespace from...

var from = "cartman@example.com";
var group = Group.Administrators;
var join = true;
//...

[blakeyrat]

@cartman82 said in Code review requested: Fruit inventory coding challenge:

The special sql-like keywords was IMO the worst design decision they made about C# after 1.0.

Possibly, but you can see the rationale when one of the major original selling points of the technology was Linq-To-SQL. The fact that Linq-To-SQL turned out to be kind of a turd wasn't apparent until years later.

@cartman82 said in Code review requested: Fruit inventory coding challenge:

I would use one regex for the amount column, then extract both amount and unit at once. You can do it character by character, but that's adding needless micro-optimization to the determent of code readability.

We'll have to agree to disagree on this one. I believe regex puts your code readability in the goddamned toilet instantly.

[masonwheeler]

@cartman82

using System;
					
public class Program
{
	public static void Main()
	{
		var from = "cartman@example.com";
		var group = "Group.Administrators";
		var join = true;
		Console.WriteLine(from);
		Console.WriteLine(group);
		Console.WriteLine(join);
	}
}

Just tried it over at https://dotnetfiddle.net/ and it compiles and works fine.

[blakeyrat]

@masonwheeler said in Code review requested: Fruit inventory coding challenge:

Just tried it over at https://dotnetfiddle.net/ and it compiles and works fine.

C#'s smart enough to know whether you're using that keyword as a variable name, or as part of a LINQ statement.

[cartman82]

@blakeyrat said in Code review requested: Fruit inventory coding challenge:

Just tried it over at https://dotnetfiddle.net/ and it compiles and works fine.

C#'s smart enough to know whether you're using that keyword as a variable name, or as part of a LINQ statement.

TIL.

It'd still be better if these weren't keywords.

[blakeyrat]

@cartman82 said in Code review requested: Fruit inventory coding challenge:

It'd still be better if these weren't keywords.

Archiveddocs

C# Keywords

C# has two types of keywords, "reserved" (stuff like for) and "contextual" (stuff like from).

The latter are fine to use as variable names. And of course you can even use the reserved keywords if you prefix them with an @.

[accalia]

@cartman82 said in Code review requested: Fruit inventory coding challenge:

var from = "cartman@example.com";
var group = Group.Administrators;
var join = true;
//...

that's perfectly valid C#, even with LINQ enabled.

so they're not keywords. ebcause otherwise i couldn't use them as variable.

it's actually important they aren't keywords, because if they were it would have been a breaking change for old code.

:-P

contextual keyword <> keyword

[Dreikin]

@blakeyrat said in Code review requested: Fruit inventory coding challenge:

For a small project like this, eh. But... generally speaking, I don't like my Fruits class having to know about System.Net.WebClient() to do its job. Seems like it has too many concerns here-- why not have the static class parse the string regardless of where the string originally comes from?

One of the aims of this library is to be able to handle large inputs as well as small in-memory ones. In particular, if the client downloads to a file (temp or otherwise) and passes it into the library for parsing. I've already moved the FetchData stuff out into the console app, but what's the suggested approach in this instance? Pass in a TextReader? A filename or file handle of some sort?

ETA:
The reason I'm doing that at all is because of one of the follow-up questions @cartman82 mentioned here about what changes would be needed if the table were really large. Comments on whether I'm looking at the wrong approach to that in the first place are also welcome.

[blakeyrat]

@dreikin said in Code review requested: Fruit inventory coding challenge:

In particular, if the client downloads to a file (temp or otherwise) and passes it into the library for parsing. I've already moved the FetchData stuff out into the console app, but what's the suggested approach in this instance? Pass in a TextReader? A filename or file handle of some sort?

Have it operate on a Stream. Yield completed rows as they come.