Register a SA Forums Account here!
JOINING THE SA FORUMS WILL REMOVE THIS BIG AD, THE ANNOYING UNDERLINED ADS, AND STUPID INTERSTITIAL ADS!!!

You can: log in, read the tech support FAQ, or request your lost password. This dumb message (and those ads) will appear on every screen until you register! Get rid of this crap by registering your own SA Forums Account and joining roughly 150,000 Goons, for the one-time price of $9.95! We charge money because it costs us money per month for bills, and since we don't believe in showing ads to our users, we try to make the money back through forum registrations.
 
  • Post
  • Reply
New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

Outlaw Programmer posted:

The other developer will get offended and say that code reviews are only for finding bugs, not for criticizing "style."

That's a total bullshit attitude. We treat code reviews as an overall sanity check, which includes calling people out on bad/questionable stylistic choices. Luckily, I work on a small team and the boss is a hardass who will outright tell you to stop being a pussy if you complain about someone being "mean" in a code review.

I've gotten my rear end handed to me in code reviews, and I like to think that I came away having learned something as a result. I mean, some stuff is personal preference... I don't like the ternary operator, but I'm not going to insist that someone not use it unless it makes the code an unreadable shitpile.

Adbot
ADBOT LOVES YOU

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug
Today's coding horror is courtesy of an applicant for a job at my company. The job's standard C#, and we start off with a phone interview where we make sure they at least know what an abstract class is and the like. We had someone come in today, and one of the questions we always ask them to code up in front of us is FizzBuzz. It's simple and any good developer will knock it out in under 5 minutes.

Today's applicant had to use MSDN to look up the syntax for a "for" loop, and then didn't know the modulus operator, so the solution looked (roughly) like this:

code:
for (int i=1;i<=100;i++) 
{ 
      decimal dec = i;
      if (dec/3 == i/3) 
      {
          ...
      }
}
That hurt my brain. It's actually pretty clever (and the solution ultimately worked!), but it's the bad kind of clever. The interview was more or less over right after needing to google the syntax for "for", anyway.

At the very least, being present for these interviews is making me feel like a superstar programmer.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug
Best of all: the "low-priority" bug/feature request that is ignored for 9 months, then suddenly becomes the most important thing in the universe and the entire team is chastisied for not having addressed it. By the same people responsible for it not getting time allocated to it, of course. Then they insist that it be fixed in the current iteration, causing things to be shuffled off to the next iteration. Then the team gets poo poo for not having done the things that were shuffled off to make room for the most important thing in the universe.

Repeat a month later with different backlog items.

This post belongs in the "management horrors" thread, though. It seems like some folks in management positions are unable to understand the this simple concept: "our team can get X things done in a 2-week release. If you want to add something of size Y, something equally sized won't be done."

New Yorp New Yorp fucked around with this message at 03:41 on May 10, 2011

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

Fiend posted:

Find/Replace 'printDay()' with 'DateTime.Now.ToString("dd.MMM.yyyy").ToUpper()'

Well, "lines of code written per day" obviously isn't a performance metric where you work! That other guy is like 40x more productive than you.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug
The solution to every problem is to over-engineer the gently caress out of it! Why do it in 9 lines of clear, readable code if you can abstract it into an unreadable monstrosity?

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

Milotic posted:

Look at those C/C++ guys frontin' with their pointers and templates. Time to step to and show them how we do this stuff in the .NET world.

That's beautifully horrible. Kudos.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

Milotic posted:

Thank you for all the kind words. I found it surprisingly fun. There's a couple of problems with that solution, one of which being it is far too quick - you don't really get the excitement of waiting for the solution. We could throw in Thread.Sleeps, but that's far too predictable. We want people on the edge of their seats.

So I present: GeneticFizzBuzz, with a mutation chance of around 1 in 1 million.

(Why yes, I am on holiday)

You are a god of overcomplication. I'm stunned.

[edit] Although, I noticed you're not really taking advantage of the parallel task library on this one. I took the liberty of altering your code thusly:

code:
        public static FizzBuzzCandidateSolution Mate(FizzBuzzCandidateSolution mother, FizzBuzzCandidateSolution father)
        {
            var child = new FizzBuzzCandidateSolution();
            
            Parallel.ForEach(mother.Map.Keys, key =>
                                                  {
                                                      lock(child.Map)
                                                      {
                                                          child.Map.Add(key, MateValue(key, mother.Map[key], father.Map[key]));   
                                                      }
                                                  });

            return child;
        }
It seems to be working, although I'm not 100% sure it'll return the correct answer, since Dictionary isn't thread-safe as far as I know. It's certainly keeping my 8 CPU cores occupied

New Yorp New Yorp fucked around with this message at 00:33 on May 24, 2011

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

BP posted:

We use SVN where I work. Depending on your team, it's actually mandated that you comment out code you wish to remove instead of just removing it. You will be called out in code review for not doing so.

That's horrible. We actually will call people out in code reviews for leaving in chunks of commented out code. I very proudly removed several thousand lines of commented out code when I was recently making some tweaks to an older application. The culprit for all of those commented out lines? Me. I felt a great weight lift off my shoulders.

The thing I wish more than anything else in the world is that people would be more verbose in their check-in comments. I hate it when I get a user saying "hey, process A used to do X, Y, and Z. But I just noticed it stopped doing them at some point."

I check source control, and sure enough, a 2 year old checkin from a guy who left long ago. The comment on the checkin says: "Changed process to not do X, Y, Z"

No explanation for why the change was made, so the best I can say to the user is "Yep, you're right, it did used to do that stuff. Timmy removed it on 3/28/2009. I have no clue why. Sorry."

New Yorp New Yorp fucked around with this message at 00:35 on Jun 9, 2011

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

DIW posted:

Neat, never seen LINQ used for XML before :)

Every time I see LINQ to XML, it makes me want to go back and rewrite a specific XML-parsing application we have. It uses DataTables. It's horrible.

I just put the whole thing under test, too... maybe it's time to do some stealth refactoring.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug
Here's everyone's warm fuzzy feeling for the day: We're not insane, other people see these horrible things and agree with us.

It helps to remember that, especially when dealing with things that seem stupid but are seen as acceptable by co-workers. Like having merging turned off in TFS, and when an hour-and-a-half lunch locked me out of a file I needed to work on, being told "you should've told me before I went to lunch, LOL" :what:

The explanation is "we're a small group, so we can talk to each other!" My response was "I can't talk to you when you're missing for an hour and a half"

Response: "..."

My favorite moment so far was when I was giving a short talk on unit testing methodologies. One guy said "well, i don't care if it works properly as long as it gives a descriptive error message when it fails," and my response was "If you test properly, you should rarely have failure cases"

That shut him up. He hates me now. I hope he gets fired due to a huge, breaking bug he introduces that is reported in a very clear, concise manner so that his replacement can fix it.

Changing jobs recently wasn't a mistake (more money, better commute, saner management), but I'm used to being in an environment where my co-workers are badasses who raise the bar constantly. Now I'm the "badass," and I'm not learning dick at work (luckily, I take my career seriously and study on my own time), and the other guys are 10+ years older than me and don't want to learn new poo poo. It's an uphill battle, but at least the CTO is on my side.

New Yorp New Yorp fucked around with this message at 07:53 on Aug 6, 2011

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

TasteMyHouse posted:

It's cool when people declare virtual methods in abstract base classes that need to be implemented in subclasses as:
code:
protected virtual void cool_method() {}
instead of
code:
protected void abstract cool_method();
of course if I actually had documentation for this poo poo it wouldn't've been as big of a deal, but as is I had no idea why my subclass wasn't working.

Maybe they didn't know that abstract methods exist.

I like it when people make every method in a class virtual, because "I might need to override it some day!" It doesn't hurt anything other than readability, of course.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

Plorkyeran posted:

and performance

I'd never thought of that, actually. I always assumed that was something the compiler figured out, and at runtime it was already squared away.

Turns out I'm wrong! http://stackoverflow.com/questions/530799/what-are-the-performance-implications-of-marking-methods-properties-as-virtual

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

Eggnogium posted:

Here's a horror: Scrolling past your avatar in IE9 slows the browser down to a crawl.

I thought I was insane. Thank you for confirming that I'm not. Scrolling past tef's avatar spikes my CPU like crazy and slows down IE9. Sometimes the browser even becomes non-responsive! It's the animated GIF; opening it in IE9 alone sends my CPU usage up to a constant 12% and memory usage keeps going up. Someone ought to send that to Microsoft and mention that so they can fix whatever the gently caress is happening.

New Yorp New Yorp fucked around with this message at 05:21 on Sep 27, 2011

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug
I work for an ecommerce site, and I was digging through our caching code today because I was working on something that would benefit from being cached.

The reason the site's performance is generally terrible became pretty quicky apparent: The "cache" is just an incorrectly-implemented Singleton that reads a List<T> out of the ASP .NET cache. The singleton looks exactly like the first version of the Singleton that Jon Skeet wrote about here.

When something needs to be pulled out of the cache, it loops over every row in the List to try to find a match. Luckily, the cache expires every 5 minutes, so the List isn't getting so big that it's performing worse than just pulling the data straight from the loving database on every page request.

I just can't loving fathom why someone thought that this was a good implementation.

Monday morning's task: See if I can reimplement the cache to be sane.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

TRex EaterofCars posted:

If there is ever a PE exam for software engineers, I will demand that one of the questions be, "should you roll your own cache implementation?"

If you answer "yes" then you not only immediately fail the exam, but the proctor will saw your fingers off to prevent you from programming ever again.

It's just a terrible wrapper around the existing ASP .NET cache. I'm glad they didn't try to make their own.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug
This one made me weep today:

code:
public bool HasErrors() 
{
     return errorList.GetEnumerator().MoveNext();
}
I replaced it with

code:
public bool HasErrors() 
{
     return errorList.Count > 0;
}
Of course, I had to explain to someone why doing it the original way was staggeringly inefficient, but it was worth it.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

Sedro posted:

The horror is not disposing that enumerator.

Trust me, all of my disposables are in using blocks.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

Jabor posted:

It's certainly better than foo.Count(), but the general contract of a property is that it functions like a field with a bit of extra functionality hooked up to it.

If something provides a Count property of its own accord, you can take it as given that that is efficient to access. If it implements an interface that requires it to implement that property, then you can generally assume that it's doing the necessary caching/maintaining that value behind the scenes.

If something is expensive to determine and the object doesn't cache that value, then it should be a retrieval method, not a property.

In that particular case, errorList had a Count property. There was another instance that did the same thing but was IEnumerable, which I replaced with .Any().

ninjeff posted:

I agree, but the code posted was a method.

edit: maybe that's the real horror

Most of this software is a horror. :cry:

I'm improving what I can, when I can, without freaking the Crusty Old Dudes out. Sadly, during the interview process, I took the presence of Crusty Old Dudes to be a good sign.

I hope that when I'm 60, some guy half my age isn't posting on SomethingTwitterBlogIpad.com making fun of me. :(

New Yorp New Yorp fucked around with this message at 09:17 on Oct 8, 2011

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

OriginalPseudonym posted:

I would think that should be reversed- if you're bad and you know it, you'll want to share your code so you can get better.

The problem with most bad developers is that they don't know they're bad. It's usually the opposite; they think they're awesome.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

Internet Janitor posted:

ToxicFrog: I think you nailed it hard.

'I find this really difficult' + 'I got it to work' = 'I am a goddamn genius'.

It's actually pretty impressive how well that generalizes to other situations.

Absolutely. Good developers will take "I found that really difficult" to mean "I don't know enough about how to do this correctly" and study the poo poo out of it. And that applies at all levels, up from basic string manipulation to architecting major applications with a ton of moving parts.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

baquerd posted:

That honestly sounds to me like you've gone from too far on one side to too far on the modular side. How many functions do you call more than once? If unit testing is your group's thing, it will certainly make easier, but it's a lot of jumping around and likely often passing a lot of parameters. I'd much rather have a 100 line function that does one thing than 5 20 line functions that do one thing.

You have to strike a balance. Some people say "if your method is more than X lines long, it's doing too much", where X is 5, or 10, or 20. I don't necessarily agree, but big methods are generally doing too much and are a nightmare to unit test and maintain.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

baquerd posted:

My group doesn't do unit testing as the development time cost is considered too high to achieve good coverage and black box testing combined with good general practices with exception handling serves us well.

That sounds insane. You're testing your code at some point (even if it's by writing status text to the console or stepping through with a debugger), so why not capture those tests in a repeatable fashion? I'd rather spend an hour or two writing test cases now and save myself from pain 6 months down the line when I have to refactor to add a new feature.

The team I recently joined has a similar "boo hoo it takes extra time to write tests" attitude, and I'm trying really hard to change it. I'm leading by example, adding tests whenever I can, even if it's only for a couple of simple methods. I've found and corrected a few ticking timebombs by doing so, too. Someone even brought up the "exception handling" point, which just seems like a crutch to me. You shouldn't have exceptions in the first place. That means your code is hosed up, which is why you should unit test.

BAD ANALOGY TIME: It's like not bothering to check if you have faulty wiring because you have a really good sprinkler system and plenty of fire extinguishers. It's good to have, but you should still fix the wires before you need the fire extinguishers.

New Yorp New Yorp fucked around with this message at 04:15 on Oct 19, 2011

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug
Saw something along these lines today:

code:
string s = string.Empty;

if (somebool)
{
    s = "foo";
}

{
    s = "bar";
}
I checked source control and it's been like that since 2008. I'm not putting the missing "else" in, that's for sure.

I'm not even going to bother mentioning it to anyone else on the team, or that we could catch poo poo like that really easily by unit testing. They've broken my spirit. I will be the sole person on the team who cares about writing testable, tested code. Then I will find a new job in 6 months.

New Yorp New Yorp fucked around with this message at 02:14 on Oct 22, 2011

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

Captain Cappy posted:

Seems like you caught that poo poo pretty easily by code reviewing. Why add the extra step of unit testing?

Because if the original developer had written unit tests, he would've known immediately that there was a problem and it wouldn't have required a second set of eyes to catch.

My viewpoint is that code reviews should be for sanity-checking business rules and architectural decisions, not finding bugs.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

Captain Cappy posted:

Why? It seemed to work pretty quickly to find that bug you just posted. Also are you sure he would have known the error immediately, considering he's the one who made it in the first place?

I'm not saying you won't find bugs in a code review, but I never sit down to do a code review with the intent of finding bugs. I've found bad architectures, missing requirements, completely mis-implemented requirements, solutions that can't possibly scale, correct solutions implemented in mind-bogglingly stupid ways, solutions containing unnecessarily copy-and-pasted code, but very rarely any "i forgot an else statement :downs:" bugs.

A good unit test suite would catch something like that because there would be an explicit test case for both scenarios (e.g. "When_Somebool_Is_True_Then_S_Is_Foo" and "When_Somebool_Is_False_Then_S_Is_Bar" or "When_Somebool_Is_False_Then_S_Is_Not_Foo"). Of course, that's assuming the test is important enough to be written in the first place. In this case, it doesn't look like it was important.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

PrBacterio posted:

That's what we in the industry like to call an "interface," and it's what the derived classes are "copying" (actually *implementing*, since, as the empty methods from the base class don't actually *do* anything there isn't any code being duplicated from there). The horror here being that all of these empty virtual methods from the base class should have been declared as abstract virtual methods (ie. virtual foo() = 0;) instead.

Not according to what he said -- they're duplicating functionality. The common functionality needs to be implemented in the base class in a non-virtual method, which possibly calls some virtual/abstract methods that contain differing, implementation-specific code.

Of course, I'm not sure what you're calling an "interface" there, since there are no interfaces anywhere in the example, and an interface isn't appropriate for what he was talking about anyway.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug
code:
if (someObject != null)
{    
     if (someObject != null) 
     {
       //code
     }
}
Someone wanted to be really really really really sure that it wasn't null.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

wwb posted:

That would make sense if there was a lock statement in there between the ifs. Guess that was too cool for school.

There was absolutely no threading involved, nor the possibility of concurrent access to the object. Just lovely, careless developers.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

mobby_6kl posted:

Did I mention that the current development code is the only version of the source that he has? Yeah. :suicide:

Oh god. Let's talk source control horrors! My soon-to-be-former job has a beautiful TFS installation that they're totally mismanaging.

Branching? gently caress branching, everyone just checks directly into the trunk. Don't worry, they label for dev deployments and release deployments!

Continuous integration? Nope! Do a "get latest" against source control and you're probably going to get something that doesn't build. Have fun getting applications working again after someone makes a breaking change to a shared component! A 10 minute change now requires a day and a half of refactoring, retesting, and praying.

I laid out a sane system with development branches, integration branches, and release branches. They don't like it, it's "more work" than having a bunch of broken applications that don't build and every release requiring figuring out what changesets are part of it and what changesets aren't.

Did I mention that my new job is at least partly going to entail teaching people how to properly do continuous integration and manage source control via TFS? Yeah. Even knowing that, they don't care to listen to me.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

baquerd posted:

Suppose we want to read a table and display it to the page. Why ORM?

It saves time and effort. How many times have you written code that:

1) Sets up a SQL command
2) Executes the command and gets back a dataset (with proper exception handling, etc)
3) Parses the dataset into an object model of some kind

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

AlsoD posted:

How do things like this come to be? How does this ever seem like the right thing to do? Was this originally a _returnBool that, over the course of things, only ever returned true and so it got optimised and renamed? Perhaps the fear of hard-coding values extended to boolean literals? Is this along the lines of #DEFINE TRUE true?

We may never know.

Maybe it did something at one time, but it was deprecated. Instead of doing the sane thing and refactoring it and any dead code out entirely, they left the method, renamed it, and made it always return the desired value.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug
Jon Skeet has an entertaining section on handling dates and times in software in this talk: http://vimeo.com/7403673 The UTC stuff starts at 16:50 or so.

I highly recommend it!

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

Poop Delicatessen posted:

Here's a lightning talk about why JavaScript sucks (and a few things about Ruby, too) that sheds more light on why JavaScript is an awful, awful language.

JavaScript has some rough parts, and that's why I'm becoming a huge fan of CoffeeScript. Plus, I just really like CoffeeScript's syntax over JavaScript.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug
At least with jQuery, it's pretty likely that anyone new on your team will already know it. If I was interviewing at a place and they said "We don't use jQuery, we wrote our own jQuery analog," I'd be very wary about taking the position. Reinventing the wheel is rarely beneficial.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

BP posted:

This is despite realistic performance tests with our application that show around a ~0.2% performance degradation from turning on TLS.

Of course, if it's already performing so poorly that 0.2% is the difference between "barely usable" and "unusable", then that makes sense. :v:

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

The Gripper posted:

:words:

That's awesome. My best story doesn't even hold a candle to that one; management was reporting that a feature was "too slow" (without any metrics defining what constituted acceptable performance, of course). We requested metrics and received some that actually were reasonable.

We tested the feature, and it performed well within the metrics provided. When told that, management insisted it was unacceptably slow and that we "do something".

Ultimately, we just said "okay, we made some tweaks that may have improved performance" (without any numbers, of course), and suddenly it was performing great! We were all commended for doing such good work. Yay!

That kind of bullshit is one of a million reasons I'm glad I don't work there anymore.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

Ephphatha posted:

How much did you charge them for your time?

Nothing, since we were salaried employees. They just didn't get another feature/bug fix in that sprint because they allocated time to a nonexistent problem.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

Aleksei Vasiliev posted:

that's why you never use Integers or the other boxing classes if you can't help it
and also never use things written by idiots who return null Integers
Eclipse warns me :colbert:

C# has nullable value types, but you have to explicitly specify that they're nullable. It's useful for, say, databases, where you could end up with a NULL integer coming back.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

wwb posted:

2 interviews this week for a mid-level position. Neither candidate could effectively answer the first bits of the "can you actually code" phone screen questions. These questions are:


I'm getting a bit worried.

For 2b, would you accept the use of
code:
CultureInfo.CurrentCulture.TextInfo.ToTitleCase()
? That's how I'd do it, although I had to look up where "ToTitleCase" lived.

I prefer to save the "can you actually code" questions until they come in for the in-person, and then have them do it with an IDE. Coding over the phone sucks. Your first question, although you'd expect that anyone with a reasonable level of proficiency would be able to answer, it doesn't show anything other than that they've read some code before. Your second question is better because it makes them solve a simple problem.

FizzBuzz really is one of the best benchmarks I've seen, though. It covers looping and conditionals and doesn't require any trivia to implement correctly, although a surprising number of people get hung up on the modulus operator.

#1 becomes trickier if you ask "What's the best way to determine if an IEnumerable<T> contains any elements?", because IEnumerable doesn't contain a Count property, it has a LINQ Count() extension method. The best way for an IEnumerable would be to use Any(), since Count() requires iterating over the entire collection, and Any() will immediately return true on the first element it hits.

[edit]
Hey, this isn't the interview thread! :downs:

New Yorp New Yorp fucked around with this message at 21:44 on Mar 3, 2012

Adbot
ADBOT LOVES YOU

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

Atimo posted:

You'd want to use .Any(), Count() will enumerate the collection.
iList has a Count property.

  • 1
  • 2
  • 3
  • 4
  • 5
  • Post
  • Reply