How To Kill a Code Review

Today's guest post is from Gary Myers from Igor's Oracle Lab. I was first introduced to Gary via comments left here. I can't find the first one of course...but he has left plenty of them. All well thought out and informative. Most recently he introduced me to the ability to do a bulk bind using %ROWTYPE here.

This is a topic that is all too often ignored, as you all know.

Steven Feuerstein states here that "Everyone knows that code review is a good idea"

The problem is what happens after the review.

Once upon a time there was a piece of code. That code had been in production for a long time, ran pretty slow but not slow enough that it had reached the top of the pile of stuff people complained about.

A change was done to that code for a new enhancement, and in one of those bursts of enthusiasm that sometimes hits a development team, it got subjected to a code review.

The whole structure of that code was ugly, with unnecessary nested cursor loops. The big kicker for performance was that at the end of one of these inner loops was a TRUNCATE TABLE. Because when you are deleting all the rows from a table, every-one *KNOWS* that a truncate is fastest, right ? Of course, as the TRUNCATE is DDL, it meant that all the SQL using that table inside that loop was getting re-parsed each and every time through the loop.

There were other problems in the code too. I believe one was with variables not being re-initialized at various points in the loop, so there was a risk of incorrect results in some unlikely cases.

The verdict of the review was that the code needed a re-write. The problem was, since it was already in production, no-one wanted to admit that there were bugs in it (and the users hadn't spotted any incorrect results). The new code would go into a future patch, but that wouldn't go live for months. However it had been promised for delivery to a test environment. A rewrite would mean missing the drop deadline.

A quick-fix could be done to improve performance. A rewrite could be done and the deadline missed. The quick-fix could be done and still meet the deadline, then a later rewrite to fix the underlying problems (but those problems probably would have been blamed on the quick fix).

A compromise was reached. Since the change to the code didn't actually introduce any new bugs, it would be allowed to go through to test with no changes from the review. And there was a promise to actually rewrite the code.

Of course, once the delivery was done, lots of other priorities came ahead. I don't know whether the code ever got the rewrite, but I suspect not. Definitely, for at least six months, there was a batch job taking hours when a five minute code change could have cut it to minutes.

At least the developers who participated in the review learnt that a TRUNCATE has drawbacks. The code reviews pretty much never happened again though.

Mentoring

At my previous job, I was very fortunate to be surrounding by some incredibly smart people. Start-ups tend to attract those types on both the business and IT side. Now those of you who work for Oracle or who work for some of the larger companies out there may be surrounded by these types.

I've typically worked at small companies and I think that makes it harder. My first boss I believe would fall into the "incredibly smart" category. It should go without saying that the other people are not smart as well, I'm talking the best of the best. People you could truly learn from. There's always a catch though...what if they don't like to teach or mentor?

My first boss hired me as a reports developer and allowed me a fair amount of room to grow. I felt like it was too slow so I moved on. Looking back on it, I think I made the right decision but I probably didn't go about it the right way.

One of the big reasons I chose to go to Revolution Money was the opportunity to do and learn cutting edge stuff. The goal (of the company) was to match Visa in transaction per second (if I remember correctly that was set around 5 or 6K). They had people who had built high-transaction/high-volume applications all over the place. "All over the place" may be an exaggeration, IT was only 20 or 25 strong at the time. "Relatively speaking," how about that? There was a high percentage of these people.

John Davies

Previously global head of architecture at BNP Paribas and JP Morgan Chase, CTO and co-founder of C24 recently sold to IONA Technologies (Nasdaq: IONA). Author of several Java books published by Wrox, veteran speaker at technical and banking conferences world-wide, expert in high performance/low latency enterprise and global architectures

Edward Katzin

He was the CTO for Madison Tyler LLC a proprietary trading company that is making the most of the capital markets shift towards electronic trading platforms in the United States and abroad.

Edward was Vice President – Technology Strategy with Visa before joining Madison Tyler and was responsible for leading the development of Visa’s technology strategy to address ever evolving challenges specific to ensuring the reliability, security, and cost effective operation of Visa’s systems, network, and application infrastructure.

As a consultant for DiamondCulster (now Diamond Management & Technology Consultants Inc.), Edward was a Principal and Senior Technical Architect responsible for designing and deploying large scale information systems solutions that align the deployment of technology solutions with business strategy and maximize the return on investments in technology.

Miladin Modrakovic [ blog ]

Again, nothing good in his LinkedIn profile so I get to make it all up. I've talked about Miladin before here and here. He's the kind of DBA who you actually believe practices the dark arts. I'm sure I could do what he does given an almost infinite amount of time. He was never afraid to show me his work, i.e. how he arrived at an answer. That's a great trait to have.

Bob Kerner

Bob doesn't have a nice summary of his skills on LinkedIn so I'm going to have to make stuff up. As far as street cred goes, he was the Chief Architect at AOL from 1999 to 2005. Despite what you may think of the company, it is still a force out there. He's also done stuff which I probably should not or can't mention else he'll have to kill me. I have a great amount of respect for this guy. Not only does he know what he's talking about he'll show you exactly how he came to the conclusion. He held a few lunch and learns from XML (schemas) to cryptography (how and why). It's not been often in my career that I've come across someone with so much experience that is as willing as he is to teach. My daily interactions were a constant opportunity to learn something new. Cyclomatic Complexity was from him. He also spoke of Information Theory and Marshalling.

There's my ode to mentors.

I try and do the same for people when the opportunity presents itself. For some reason I tend to focus on those on the business side who show a serious aptitude for data related activities.

So if you have someone in your organization who shows a higher than normal interest, take them under your wing. If you are a newbie, throw yourself at their feet and ask them to help you.

Financial Services: Learning the Business

I'm on a quest to find all the information I can about the Financial Service industry...specifically how it works.

I started with CardReport, which besides being pretty ugly, has some fairly useful information.

Of course it led me to much more reliable info sources.

First up, the Federal Reserve. That took me to the Government Printing Office (GPO) where I can then read in excruciating (and exciting!) detail all about Title 12, Banks and Banking.

That takes me to Regulations.gov, which I can then search through thousands of documents online (very nice). Sadly the material isn't all that exciting.

Finally, I found NACHA, The Electronic Payments Association. Here you can learn all about ACH (Automated Clearing House) payments. Joy.

I'm sure glad I've got this on my reading list. If I weren't a software developer, I'd probably go mad.

Lessons Learned

I was let go yesterday. Many thought that it was another one of my pranks...which was funny. Following are my lessons learned from my time there:

1. Get express written permission about what you can (if anything) and cannot blog about.
2. Not everyone is your friend.
3. Not everyone will like you...just because.
4. Some people will not hesitate to throw you under the bus.
5. Big companies might not be the place for me.
6. Big companies are full of many motivated and talented people.

I learned a great deal in the year and a half. My family and I moved from Gainesville to Tampa and it was the best decision we've made so far.

I was exposed to data warehousing which was a challenge to say the least. To spend a week trying to figure out the best way to move data was an interesting exercise.

I was exposed to big company politics which was fun.

Overall my experience there was positive. I learned a great deal both personally and professionally. I met and hope to retain many good friends and I will miss all of them dearly.

It's a Matter of Time

I've been thinking a lot lately about my recent failed deployments.

How did I get so sloppy?

I'm not one to make excuses, but I would say there are some mitigating circumstances at least. Time being one of them.

So I got out my trusty calculator (SQL*Plus), and ran the numbers.

From August 26, 2007 through March 30, 2008, I've worked 1802 hours. Of that, 118 were PTO or holiday, which brings the total down to 1784. For perspective, a work year of 8 hours per day comes out to 2080 hours a year.

VAR HOURS NUMBER;
EXEC :hours := 1784

  1  SELECT
  2    ROUND( ( :HOURS / 2080 ) * 100, 1 ) per_of_tot_year_hours
  3* FROM DUAL
ETL_WRK@ORA10GR2>/

PER_OF_TOT_YEAR_HOURS
---------------------
                 85.8

Cool! Only 86% of my hours in a little over 6 months!

Obviously, this is not a good thing.

Further breaking the numbers down:

VAR C VARCHAR2(10);
EXEC :C := '26-AUG-07';

SELECT
  start_day,
  end_day,
  days_between db,
  SUM( business_days ) bd,
  ROUND( ( :hours / days_between ), 1 ) hpd,
  ROUND( ( :hours / ( days_between / 7 ) ), 1 ) hpdw,
  ROUND( ( :hours / SUM( business_days ) ), 1 ) hpwd,
  ROUND( ( :hours / SUM( business_days / 5 ) ), 1 ) hpww
FROM
  (
    SELECT
      start_day,
      end_day, 
      TRUNC( end_day - start_day ) days_between,
      start_day + rownum dayof,
      ( CASE
          WHEN TO_CHAR( start_day + rownum, 'D' ) IN ( 2, 3, 4, 5, 6 ) THEN
            1
        END ) business_days
    FROM 
      dual a,
      (
        SELECT
          TO_DATE( :c, 'DD-MON-YY' ) start_day,
          TO_DATE( '30-MAR-08', 'DD-MON-YY' ) end_day
        FROM dual
      ) b
    CONNECT BY LEVEL <= TRUNC( end_day - start_day )
  )
GROUP BY 
  start_day,
  end_day,
  days_between
/

START_DAY  END_DAY        DB     BD    HPD   HPDW   HPWD   HPWW
---------- ---------- ------ ------ ------ ------ ------ ------
08/26/2007 03/30/2008    217    155    8.2   57.5   11.5   57.5

DB = Days Between
BD = Business Days
HPD = Hours Per Day
HPDW = Hours Per Day/Week
HPWD = Hours Per Work Day
HPWW = Hours Per Work Week

The scary part is that I am not very diligent about entering my time. It's probably short anywhere between 5 and 10%.

I'm not the only one working these kinds of hours either. I know for a fact there are others.

Do you think this plays a role in my failed deployments?

WellCare Data Breach...

Great...

• Georgia WellCare Data Exposed
  


• Insurance records of 71000 Ga. families made public


• Georgia: 71,000 private records publicized


• Georgia Patients’ Records Exposed on Web for Weeks


• WellCare Flubs Data Privacy for 10,000 Georgians

I haven't spoken with anyone at work about this but I was aware of the event before going on vacation. I did not know any of the particulars, only that it involved Georgia in some way.

I believe everyone works hard to safeguard patient data. The short term drawback is that it will make everyone's life that much more difficult, the long term good will be a better process (I hope).

Code Style: Tables

Tables are easy.

CREATE TABLE t
(
  col1 NUMBER(10,0)
    CONSTRAINT  pk_col1 PRIMARY KEY,
  col2 VARCHAR2(32)
    CONSTRAINT nn_col2_t NOT NULL
    CONSTRAINT uq_col2_t UNIQUE,
  col3 VARCHAR2(400),
  col4 VARCHAR2(1) DEFAULT 'N'
    CONSTRAINT ck_yorn_col4_t CHECK ( col4 IN ( 'Y', 'N' ) )
    CONSTRAINT nn_col4_t NOT NULL
);

Remember to always name your constraints. While I am at, use constraints as much as humanly possible, at least in your OLTP systems. You'll be able to reduce the amount of code you need to write and actually let the database do it's job. I'd much rather let the database do it than rely on code to maintain my data integrity.

For the datawarehouse, you'll need to think about constraints a bit more as it may slow down load times. I'm still all for constraints, but I would never say always use them.

For child tables:

CREATE TABLE s
(
  col5 NUMBER(10,0)
    CONSTRAINT pk_col5 PRIMARY KEY,
  col1
    CONSTRAINT fk_col1_s REFERENCES t( col1 )
    CONSTRAINT nn_col1_s NOT NULL,
  col6 VARCHAR2(30)
);

For Foreign Key constraints, you do not have to declare the type as it will be inherited from the parent table.

This would be helpful if someone up and decided to change the NUMBER(10,0) to a VARCHAR2(10) or something (please don't ever do that!).

As for STORAGE or other table options, I typically leave that up to the DBA or work with them to add them. They may have a particular setup for certain tables that you can't possibly know (if you don't talk to them).

To recap:

• Use constraints as much as possible


• Always name your constraints


• Work with your DBA for table options


• Always name your constraints

Code Style: General Principles

I have always wanted to put together my personal code style guide. Now that I have a blog, I can do so rather easily.

This will be the first among many that details my approach to coding style. Yes, it is very particular to me. No, I don't expect anyone in their right mind to follow it. Though perhaps someone can use it as a general guideline.

I am fairly fanatical about style.

Parenthesis go on separate lines, always (I'll eventually contradict myself I'm sure). None of this my line starts with a comma crap. My only concession is that it makes commenting out lines easier. Otherwise, I can't stand it. It's ugly to me and I don't like ugly.

Tabs = 2 spaces

CREATE OR REPLACE is the top line followed by the particular object on the second line (I don't know why, it's my style though).

CREATE OR REPLACE
PACKAGE p_package
IS
...

Always put "show errors" at the bottom of procedure/function/package scripts.

Always end your procedure or function with the name after END.

CREATE OR REPLACE
PACKAGE p_package
IS
...
END p_package;
/
show errors

Align SELECT statements left, not right:

Evil

SELECT blah, t.id, s.x
  FROM t, s
 WHERE t.id = s.id
   AND t.x = s.x; 

Correct

SELECT 
  blah,
  t.id,
  s.x
FROM 
  t, 
  s
WHERE t.id = s.id
  AND t.x = s.x; 

I believe but can't confirm that Toad Formatter is the main perpetrator of this travesty.

Instrumentation. Use lots of it. Home grown or Mr. Kyte's debug routine. You can also use the Oracle supplied package DBMS_APPLICATION_INFO. I have gotten into the habit of using set_session_longops and it helps tremendously in monitoring those pesky long running datawarehouse programs.

Be liberal in your use of spaces: ADD_MONTHS( TRUNC( SYSDATE, 'MONTH' ), 2 )
Capitalize keywords like SELECT, INSERT, UPDATE and DELETE.
Standard functions should also be capitalized.

I'm sure I've got more and I'll add them here as time goes on.

Zero Day is NOT Upon Me

I got a brief reprieve today, I guess.

Neither of my two deployments are going through tomorrow, so I have two more weeks to go until another opportunity to screw something up. ;)

Speaking of which...
I watch SpongeBob Squarepants quite a lot with a 5 year old boy in the house. I was watching this episode the other day where Patrick (the starfish and best buddy to SpongeBob) found out his parents were coming over to visit him.

He was not very happy as his parents treated him like he was stupid. SpongeBob told him that he would help out by acting even dumber than Patrick thus making Patrick actually look smart. The act worked, but too well. Patrick, even out of earshot of his parents, kept cracking stupid jokes at the expense of SpongeBob. This resulted in SpongeBob getting mad and reminding Patrick that he was only acting stupid to help him out.

Since I put my sign up (not counting my counter up top, no one here gives me a hard time) everyone has consistently beat me up over it. Today was it for me. Yes, I brought it on myself with the sign and all that, but I'm through with it. My boss mentioned that if I had to reset my sign that I wouldn't really be there to do so as I would have been escorted out the door (he didn't say this in a mean-spirited way...he might be right next to me) and unable to reset.

I started practicing how I would answer questions about my firing in my next interview.

Interviewer: "So, why did you get fired?"
Me: "Well, I screwed up production a couple of times."
Interviewer: "How did you do that?"
Me: "The first time I deployed code from development to production."
Interviewer: "Really?! Why did you have access to production?"
Me: "..."
Me: "On the second occurence, I deployed a bug into production; I changed a non-requirement driven piece of code"
Interviewer: "What about QA? Didn't they do regression testing?"
Me: "..."

I felt a little better.

I've have never been happy about my mistakes, I tried to "man-up" and own them; perspective and context are good things though. I should learn from my mistakes and correct the behavior that led to them, that's all I can really do.

The Chicken Almost Came Home to Roost

We have a daily meeting with the Business concerning the current, long running project. This is the one I have screwed up a couple of times.

I have a scheduled deployment next week, mainly just performance improvements. I've managed to get this down from 10-12 hours to 7-8.

At the end of the meeting, the Business questions the need for the performance improvements - we're re-architecting my solution in parallel because mine was just a conversion from SAS (yuk!) - as we'll be live in just 2 months with the new one.

Obviously, I know why they're thinking that.

IT, me, can't be trusted to do it right. Since I've, umm...screwed up a couple of times. Why do this if it's not broken?

I just hung my head low, I knew, and I couldn't really argue with them. I had no ground to stand on. Our PM said that he'd take it back to our manager and let them know.

I went to lunch and thought about this blog entry I would be writing.

I got back from lunch and my PM informed me that this would be going into production...the Business' big boss, Miss VP, said so.

Woohoo! Someone has some semblance of confidence in IT (me)!

I've done everything I could to make sure that I didn't do something silly. We had a peer code review to compare the most recent build against that which was in production. My unit tests were much more thorough. I worked with QA to get them to look at specific points. Let's just hope all goes well.

This is a big test for me. Either I pass and gain some credibility back or fail and lose my job. Wish me luck!

The Good Manager

I read Lewis Cunningham's article today What Keeps you at your Job?

This is something I think about quite often. I am in a very chaotic, immature organization currently. The process to deploy code changes about every other day and of course non of it is documented. Then there's the fact that I had complete control at my previous job, I was the DBA, Architect, Web Developer and Designer (suck at that), and most importantly Database Developer. I had a very good manager who just literally let me run wild (within reason of course).

For me, that was a perfect situation. I felt I was under-utilized at my previous job and that was the perfect opportunity to flex my muscles. I learned a great deal there and I am forever thankful for that.

One of the big reasons I took my current job was because of the chaos and the immaturity of the IT organization. There are countless opportunities to help shape the future, to build the foundation. I'd also get to experience life in the for-profit corporate world where performance is rewarded financially. There's also significant room to advance relatively quickly compared to more established environments.

I have learned things on the technical side, but far and away my biggest gain in knowledge is in how to do software development in a team environment and the peculiar politics of a company.

I have my manager to thank for that. He is a former military officer who attended one of the military academies. He has worked in our industry for a number of years and is our subject matter expert on the financial side of things.

• He gives us (developers) the opportunity to voice our opinions.
• He gives us a view into the politics.
• He gives us the big picture view.
• He is fair.
• He does not do things just because that's the way they're done. He fights those battles so that we don't have to do it the wrong way.
• He backs us up.
• Shit doesn't roll downhill with him.

To me, those are all terrific qualities. When I've screwed up, he tells me; usually though, he asks me questions so that I will come to the realization. He's been an outstanding leader and most importantly (to me anyway), a teacher.

If he ever decided to leave, I might just have to follow him.

The Countdown Timer

So I found a handy little countdown timer from this site so that I can replicate my hand made sign in my cube. This all stems from a previous incident.

Now I'll have a reminder at work and on the blog not to touch that which is out of scope.

Humor in the Workplace

In an effort to deal with my recent screwup (one of two recent ones), I decided to try and laugh at it a bit.

Our organization is very young and since the on-boarding of our new CIO, our directive has been to stabilize current processes. In that regard, the VP who heads up the Infrastructure team has placed placards on his office window signifying the number of days one or another system has been up with no interruption in service.

I ran into him in the hallway and suggested he put one up for me too, "Days since Chet 'messed' up Production code." He got a hearty laugh out of that.

I've always been able to laugh at myself, if not immediately, then soon after. So I put up a hand made sign in my cube that read the same thing; I'm at 21 days. Hopefully it will remind me as I'm pouring through code not to touch that which is out of scope with the current requirements. That was my mistake on both occurences.

It's definitely a talking piece and hopefully people can laugh at it (as I can...sortof), but it will be a constant reminder to me as well.

Good Day to Worse Day

Today I got the opportunity to have lunch with my CIO.

A few weeks ago I sent him a manifesto, which I would now classify as more of a philosophy. He kept promising me he would articulate a response via email. Being the CIO of a Fortune 400 company, I figured he had better things to do than to write out an email to me. So I offered him up a trade, lunch in exchange for the email. Surprisingly, he agreed.

Today was the day but I fully expected him to cancel; surely something else would come up. Nothing did, but he did move it back by 15 minutes.

Down to the cafeteria we went (I was really hoping to go out to lunch, just to get a ride in his Porsche Gemballa). We sat in a booth and started talking. We discussed everything from my group (excellent group of people, talented and fun), using MySQL databases for one-off projects (I was for putting them in a single Oracle database), to the state of our current OLTP application (crud).

He mentioned user-defined fields (OLTP) and I told him about one instance where someone in our company created those in an internal application. I didn't say anything at the time because it was not in my group and I didn't want to call someone out for something I thought was wrong. He told me I should have, that it would be in the best interests of the company. Then he said something that I have heard him say in our All-Hands meeting, "Let me be the one that makes the wrong decision."

That sealed the deal for me. I have liked him and what he has done since he got here, but that one comment told me that he took his job as leader seriously. I was thoroughly impressed.

So that was the good (great) part of the day. I felt great because our CIO listened to me prattle on for an hour and listened to me. He even used one of my analogies (well, not mine really) in his management meeting a short time later.

Now on to the worse part.

The application I have been working on for the last few months required an Emergency Fix (EFIX) because they had detected a bug. I realized quickly that I was the culprit. Something that had worked previously was changed by me in an effort to re-factor the code. It wasn't broken. There was no specific business requirement to re-factor it, I just did it...and screwed it up. The ironic part is I had just sent out an article to my co-workers about discipline making good developers.

I told this to the Business folks, what I had done; and apparently I hadn't earned any brownie points with them because they escalated it to my boss and VP. Which of course had to go to the CIO as well...

I definitely screwed up. There's no way around that. I know better than that. Oh well. It was certainly a lesson in humility. I'm just thankful I still have a job...