Monthly Archives: August 2020

Is code review subjective or objective-quantifiable? My answer, still useful!


“Software Engineering” on Stack Exchange, OP question and all responses here: https://softwareengineering.stackexchange.com/questions/120244/is-a-code-review-subjective-or-objective-quantifiable/120254#120254

My answer, perhaps my most useful, professional, lessons learned. From 2010:

Grading individuals in a review is counter to most successful systems I’ve worked with, maybe all. But the goal I’ve been trying to reach for more than 30 years is fewer bugs and increased productivity per-engineer-hour. If grading individuals is a goal, I suppose reviews could be used. I’ve never seen a situation where it was required, as a worker or as a leader.

Some objective study (Fagan, etc.) and a lot of popular wisdom suggests that peer relationships facilitate code reviews aimed at reducing bugs and increasing productivity. Working managers may participate as workers, but not as managers. Points of discussion are noted, changes to satisfy reviewers are generally a good thing but not required. Hence the peer relationship.

Any automated tools that can be accepted without further analysis or judgment are good – lint in C, C++, Java. Regular compilation. Compilers are REALLY good at findng compiler bugs. Documenting deviations in automated checks sounds like a subtle indictment of the automated checks. Code directives (like Java does) that allow deviations are pretty dangerous, IMHO. Great for debugging, to allow you to get the heart of the matter, quickly. Not so good to find in a poorly documented, 50,000 non-comment-line block of code you’ve become responsible for.

Some rules are stupid but easy to enforce; defaults for every switch statement even when they’re unreachable, for example. Then it’s just a check box, and you don’t have to spend time and money testing with values which don’t match anything. If you have rules, you’ll have foolishness, they are inextricably linked. Any rule’s benefit should be worth the foolishness it costs, and that relationship should be checked at regular intervals.

On the other hand, “It runs” is no virtue before review, or defense in review. If development followed the waterfall model, you’d like to do the review when coding is 85% complete, before complicated errors were found and worked out, because review is a cheaper way to find them. Since real life isn’t the waterfall model, when to review is somewhat of an art and amounts to a social norm. People who will actually read your code and look for problems in it are solid gold. Management that supports this in an on-going way is a pearl above price. Reviews should be like checkins- early and often.

I’ve found these things beneficial:

1) No style wars. Where open curly braces go should only be subject to a consistency check in a given file. All the same. That’s fine then. Ditto indentation depth**s and **tab widths. Most organizations discover they need a common standard for tab, which is used as a large space.

2) `Ragged

looking
text that doesn’t

line up is hard to read
for content
.`

BTW, K&R indented five (FIVE) spaces, so appeals to authority are worthless. Just be consistent.

3) A line-numbered, unchanging, publicly available copy of the file to be reviewed should be pointed to for 72 hours or more before the review.

4) No design-on-the-fly. If there’s a problem, or an issue, note its location, and keep moving.

5) Testing that goes through all paths in the development environment is a very, very, very, good idea. Testing that requires massive external data, hardware resources, use of the customer’s site, etc, etc. is testing that costs a fortune and won’t be thorough.

6) A non-ASCII file format is acceptable if creation, display, edit, etc., tools exist or are created early in development. This is a personal bias of mine, but in a world where the dominant OS can’t get out of its own way with less than 1 gigabyte of RAM, I can’t understand why files less than, say, 10 megabytes should be anything other than ASCII or some other commercially supported format. There are standards for graphics, sound, movies, executable, and tools that go with them. There is no excuse for a file containing a binary representation of some number of objects.

For maintenance, refactoring or development of released code, one group of co-workers I had used review by one other person, sitting at a display and looking at a diff of old and new, as a gateway to branch check-in. I liked it, it was cheap, fast, relatively easy to do. Walk-throughs for people who haven’t read the code in advance can be educational for all but seldom improve the developer’s code.

If you’re geographically distributed, looking at diffs on a screen while talking with someone else looking at the same would be relatively easy. That covers two people looking at changes. For a larger group who have read the code in question, multiple sites isn’t a lot harder than all in one room. Multiple rooms linked by shared computer screens and squak boxes work very well, IMHO. The more sites, the more meeting management is needed. A manager as facilitator can earn their keep here. Remember to keep polling the sites you’re not at.

At one point, the same organization had automated unit testing which was used as regression testing. That was really nice. Of course we then changed platforms and automated test got left behind. Review is better, as the Agile Manifesto notes, relationships are more important than process or tools. But once you’ve got review, automated unit tests/regression tests are the next most important help in creating good software.

If you can base the tests on requirements, well, like the lady says in “When Harry Met Sally”, I’ll have what she’s having!

All reviews need to have a parking lot to capture requirements and design issues at the level above coding. Once something is recognized as belonging in the parking lot, discussion should stop in the review.

Sometimes I think code review should be like schematic reviews in hardware design- completely public, thorough, tutorial, the end of a process, a gateway after which it gets built and tested. But schematic reviews are heavyweight because changing physical objects is expensive. Architecture, interface and documentation reviews for software should probably be heavyweight. Code is more fluid. Code review should be lighter weight.

In a lot of ways, I think technology is as much about culture and expectation as it is about a specific tool. Think of all the “Swiss Family Robinson”/Flintstones/McGyver improvisations that delight the heart and challenge the mind. We want our stuff to work. There isn’t a single path to that, any more than there was “intelligence” which could somehow be abstracted and automated by 1960s AI programs.

share edit delete flag
edited Dec 12 ’13 at 21:51
community wiki
3 revs, 2 users 79%
Bill IV

Blog Post of “List and example of Ultimate Car Page’s Porsche 911 (993) GT1 photos”


Ultimate Car Page: https://www.ultimatecarpage.com/

https://www.ultimatecarpage.com/car/3174/Porsche-911-GT1.html

That top-level page includes a link to 88 GT1 images,

Selected pictures I find useful are included below. Pictures I find less useful are just listed by number. See the note on accessing the pictures at the bottom of this page.

124269 FTN 00 car in foggy paddock

124279 FTN 00 car and Factory / Mobil 1 car from right, rear, quarter

124482

124483

124484

124485

124486

124487

124488

124489

124490

124491

124492

124493

1997 Evo / Customer cars:

https://www.ultimatecarpage.com/car/3175/Porsche-911-GT1-Evolution.html

Note about access by image number:

Photos at Ultimate Car Page can’t be searched by number *in the page*. But can be searched by number from a search engine (DuckDuckGo, Google, Bing, etc.) Each image is in a file with its 6 digit ID where this example shows ######: https://www.ultimatecarpage.com/images/car/3174/Porsche-911-GT1-######.jpg, for example, https://www.ultimatecarpage.com/images/car/3174/Porsche-911-GT1-124490.jpg

or

https://www.ultimatecarpage.com/images/gallery/mm75/Porsche-911-GT1-124279.jpg

Notice the bold text for the literal image file shows internal details of how Ultimate Car Page is organized internally.

The images are embedded in a web page named https://www.ultimatecarpage.com/img/Porsche-911-GT1-######.html
ie:
https://www.ultimatecarpage.com/img/Porsche-911-GT1-124490.html

or

https://www.ultimatecarpage.com/img/Porsche-911-GT1-124279.html

You can click through those links to see the picture but I don’t know html well enough to get the picture to display in this page AND have it be live for click-to-open, from the .html URL. More to learn. Another opportunity to excell!

Tamiya Porsche 911 GT1 kit: imperfections and repairs -Still Under Development!


Lets review: Tamiya’s 1996 Porsche 911 GT1 kit is mostly accurate, and concisely engineered. The 2 sides of the transmission each represent 6 or 7 pieces from the actual car, and the top and final drive piece also includes the rear suspension bulkhead, six space-frame tubes, rear suspension rocker bases and two adjustment knobs for the external shock absorber reservoirs.

But a few of the things provided aren’t quite what you see in pictures of the real thing. And a number of things in the pictures aren’t in the kit. In describing the kit I touched on The cooling air ducts for the rear brakes, and the breather catch tank.

Rear Brakes Cooling Air Scoops and Ducts:

The back of the scoop and ducts need to be made or bought. They should look like

naked .jpg URL: Displays image in moderate size, click takes one through to it. Maybe add height and width, or just one as part of this?

Link to a jpg URL: Displays the URL, not the thing the URL points to. Not worth it.
https://www.ultimatecarpage.com/images/car/3174/Porsche-911-GT1-48904.jpg

Image of a jpg URL: Displays a really large image, click does NOT link through. Big, but not worth it.
48904.jpg

naked .HTML URL: Doesn’t display image. Can be clicked through.
https://www.ultimatecarpage.com/img/Porsche-911-GT1-48904.html

https://www.ultimatecarpage.com/img/Porsche-911-GT1-124487.html

https://www.ultimatecarpage.com/img/Porsche-911-GT1-124486.html124486

Catch Tank:

The catch tank can be re-worked to fix the shape problem. So lets start with what can be fixed or re-worked. Then the missing bits that need to made or found.

The catch tank should look like this: Rectangular. Top 2 pictures.

detail 2

Porsche Rohr 1996 911 GT1 
DSC_0133//embedr.flickr.com/assets/client-code.js

If you haven’t seen Renaissance Model’s reference pages you’re in for a treat:

RM galerie de details:
https://www.renaissance-models.com/galerie_de_details.htm

RM Front Page:
https://www.renaissance-models.com/francais.htm

The front page is French/English, deeper in, French only.

Brake Disks
The carbon brake disks (380 mm diameter 37mm thick) are ventilated, by
round holes on straight radial lines. Rotating cutter? Formed by tooling? They follow a two-holes, space, one hole, space, two-holes pattern. My guess, 10 sets of 3.

In a picture showing < 180 degrees I count 14 holes. 2 X 14 = 28, but 28/3 is 9 1/3, not an even pattern. Symmetry around the disk seems likely to be important. The smallest number greater than 28 that is evenly divisible by 3 is 30.

007_00_GT1_left_rear_corner_D13a_john_sinkgraven_crop_holeCount//embedr.flickr.com/assets/client-code.js

Think of the 1 hole-space-2 holes-space pattern as 5 points, 3 of which are holes, 2 of which are not. 0-00- so the disc goes: 0-00-0-00-0-00-…If 10 sets of 3 holes, is right, 360 degrees/50 points = 7.2 degrees/point. 5 points = 36 degrees, 360/10.

Brembo’s web site:

https://www.brembo.com/en/company/news/formula-1-vs-lemans-2018

https://www.brembo.com/en/company/news/formula-1-vs-lemans-2018

lists 36 as the minimum number of cooling holes for GT1 brakes, in 2018.

Robert Muschitz’s marvelous photo:

Robert M again:

Porsche GT1//embedr.flickr.com/assets/client-code.js

Brake disk calipers.
Brake disk calipers should have open backs, through which the pads should be seen. The kit parts have closed backs, but can be filed down to shape to expose the disk, and the pads, same radius. Mind you file for a curve.

A cardboard template from the rest of the disc would help. A brass-colored brace connects the two halves, at the back. Something to make, but not much. Stretched parts-tree polystyrene, metal wire, or mechanical pencil “lead” will suffice. Two “U” shaped wire bails cross the gap behind the pads and across the disk. Small steel wire, mini staples?

The wheels cover this area, but if you want to back off a wheel (or more) for an informal pose, some time on the backs of the calipers will be rewarded.

This should be a link:
<a href="http://Frame Grab from Callas RennSport Rohr Yellow Car front brake_caliper//embedr.flickr.com/assets/client-code.js” target=”_blank”>Frame Grab from Callas RennSport Rohr yellow car front brake caliper” https://www.flickr.com/photos/wbaiv/50231600796/

This should be an image but not a link

This should also be an image but not a link
The real thing:
fd7cc8...

Cold air to turbo inlet pipes.
Tamiya provide part A32, connected to the roof air scoop, with left and right takeoffs going to the turbo-compressors and the center going down through the air to air inter-cooler. Unfortunately, the two turbo-bound pipes on Tamiya’s part have a flat bottom, simplifying the mold, but changing the cross section of the pipe. Straight lines that should be curves are eye-catching.

Tamiya Porsche 911 GT1 filing & cleanup. Front of engine, serpentine belt, turbos, waste gates and air ducts,//embedr.flickr.com/assets/client-code.js

These photos show how it should look.

https://www.ultimatecarpage.com/img/Porsche-911-GT1-48904.html

48904

https://www.ultimatecarpage.com/img/Porsche-911-GT1-48905.html

48905

https://www.ultimatecarpage.com/img/Porsche-911-GT1-48906.html

48906

https://www.ultimatecarpage.com/img/Porsche-911-GT1-604.html

48904

https://www.ultimatecarpage.com/img/Porsche-911-GT1-124486.html

48904

https://www.ultimatecarpage.com/img/Porsche-911-GT1-14936.html

12436

flickr pix

https://i0.wp.com/farm6.staticflickr.com/5178/5418668794_fd7cc82416_b.jpg

Emphasize Division Between The Left and Right Inter-cooler Outputs.
Tamiya supply a single piece for both intercoolers, the ducts that take their outputs to the sides and the plenum “log”s that feed the throttle and injector for each cylinder. The division between the ducts on the two sides aren’t as clear as photos suggest:

https://www.ultimatecarpage.com/img/Porsche-911-GT1-603.html

603

https://www.ultimatecarpage.com/img/Porsche-911-GT1-14936.html

603

Remove One or Both Pairs Of Shock Reservoirs.
Tamiya provides a pair of shock reservoirs attached to the sides of the final drive. And a second pair attached atop the rear anti-roll bar housing. One place or the other was common for different cars at different times. Some shock reservoirs are seen attached to the diagonal tubes from the top of the transmission to the rear suspension bulkhead.

Other cars, other times, reservoirs are on top of the final drive housing, or on either side of the back end of the transmission. Or asymmetrically disposed somewhere across two of the places listed. There is clearly the potential for a table showing where the reservoirs were and whether gray metallic painted reservoirs and shock bodies, or red metallic paint painted sets were used. And what color for the springs. Indexed by chassis number and markings. More opportunities to excel!

GT1 109, gray metallic, reservoirs on diagonal tubes. Metallic / stainless braid hoses and banjo fittings. Glossy black springs. Glossy black three-bolt top of the spring. Black-red-black lower end stop.

https://www.ultimatecarpage.com/img/Porsche-911-GT1-14936.html

14936

GT1 104 Gray metallic, reservoirs on either side of the final drive. Aluminum colored cloth heat reflective wrap on hoses. Glossy, slightly darker red springs. Blue anodize banjo fittings. Black + silver / gold metallic three-bolt top of the spring. Red, Red, Black lower end stop.

https://www.ultimatecarpage.com/img/Porsche-911-GT1-124483.html

14936

https://www.ultimatecarpage.com/img/Porsche-911-GT1-124484.html

14936

GT1 101 Red metallic, reservoirs on carrier at back edge of final drive. Metallic braided hose or braid-like cloth.Semi-gloss black spring. White (silver) metallic banjo fittings. Dark Gray / Gray Blue on cone-shaped top of spring .

https://www.ultimatecarpage.com/img/Porsche-911-GT1-48904.html<img

14936

GT1 117 Red metallic, reservoirs at back edge transmission. Black rubber/vinyl hoses. Glossy dark red spring. White (silver) metallic banjo fittings. Dark Gray / Gray Blue anodize on cone-shaped top of spring. Black-black-black lower end stop.

2349/1997-Porsche-911-GT1-2.jpg

web post: Tamiya’s 1/24 Porsche 911 GT1 (993, 1996) kit described. #24186


This is the same text as a page I just constructed, but will be more easily found this way. Still figuring out how to format long form information.

Tamiya’s Porsche 911 GT1 represents the first year, 1996, factory racing cars that finished 1st and 2nd in the GT1 class, at Le Mans. This excellent debut was slightly bittersweet, the overall winner was a Porsche, but the previous generation, the 1995 World Sportscar Championship Prototype that Porsche had developed with Tom Walkinshaw Racing. but hadn’t raced. Long term Porsche customer Joest Racing borrowed the Prototype from the Porsche Museum, used the 962 engines they were familiar with, and beat the factory team! They would repeat this performance in 1997 as well.

Still, the 1996 GT1s did finish better than the McLaren F1s and every other manufacturer, as they were designed to do. Toyota, Nissan, Lister and Chrysler/Dodge GT1 efforts all came to naught, Mercedes Benz followed Porsche’s lead and produced purpose-built racing cars justified by a single streetable example. Porsche responded in kind, the factory team winning Le Mans overall in 1998 with their GT1 motor in a carbon-fiber Mk II chassis, while Mercedes Benz won most of the races other than Le Mans. The GT1 class was discontinued.

23 street versions of the 911 GT1 were built and sold to civilians, and apparently two of the customer racing cars were made street legal during recent restorations. So the idea of a road car that could compete at Le Mans wasn’t completely wrong.

Tamiya’s kit is molded in white, black and “chromed”, with an extensive decal sheet for the exuberant Mobil 1 sponsorship markings. Besides the 1996 factory Le Mans cars 002 and 003, and the bare carbon-fiber test mule 001, it can be marked as all but one of the customer racing cars, which were delivered with the first year bodywork. Only the last customer car was built with the 1997 “Evo” body, though most (but not all) were later converted for better aerodynamics.  “Evo”s can be recognized by their 911 (996) / Boxter common form headlight and turn signal assemblies. The Rohr team were notably successful with their round-headlight GT1 in yellow, as were the G-Force / Blue Coral team with their dark blue with black and yellow trim on chassis 101. Chassis 101 was originally in dayglo red and white, for cigarette sponsorship. Chassis 104 enjoys Larbre Compétition‘s busy FATurbo markings with round headlamps.

The kit has 21 construction steps, the first 9 for the engine, transmission and rear suspension, with 45 parts and 2 decals. 5 more steps (11-14 and 16), cover all 22 parts of the interior / driver’s compartment, except the 6 of the roof duct, body shell and clear parts.  Step 10 creates the front suspension, mounted on the front carbon fiber undertray. Along with the interior, step 11 crosses the length of the underside of the car, includes fixing the undertray and front suspension to the lower front chassis, adding radiator, nose air intake, and adding pivots for the engine cover.  Step 12 adds powertrain and rear suspension to the chassis, behind the firewall.  Step 15 is a pure play in paint and decals for the wheels, and slipping the tires on them. The last 5 steps, 17-21, build the body shell. A separate sheet shows how the Mobil 1 decals cover the body, most to be applied between steps 19 and 20. Other decals are applied in 9 other steps.

The decal sheet has 60 numbered images, and the kit has 132 parts, including a stretched part-tree radio antenna, 4 soft tires (with maker’s markings) and 4 soft-plastic “Poly caps” that pins molded inside the wheels plug into. Masks are provided to simplify painting the edges of the windshield and door windows semi-gloss black.

A clear body version was released later, but only front and rear body shells are clear, the aft edges of the front fenders, lower aft of the rear fenders, and rear fender air scoops are all white plastic. The roof scoop underside and single horizontal splitter are black plastic.

The crankcase, cylinders, and lower head, (intake and exhaust ports) are 2 pieces, split top and bottom, with reasonable molded-on top-of-engine coolant plumbing.  The transmission case is split left and right, with a third piece for the top of the case,  and final drive. Each bank of 3 cylinders has a separate cam box, intake and exhaust manifolds, exhaust and compressor turbines with restrictor air box, waste gate and bypass, exhaust tailpipe and tip, and compressed air pipe to the intercoolers. The two intercoolers and intake “logs” are a single piece shared by both sides, as is the air intake for the inter-coolers and turbo-compressors, and the engine front serpentine belt pulleys with oil and water header tank. The alternator, 3 piece oil tank and a mysterious small radiator behind the driver’s seat, complete the purely “engine” parts.

The talented part A34, the transmission top plate, concisely incorporates four space-frame “tubes” along a horizontal line, that link to horizontal “tubes” in the frame molded in relief on the back of the firewall.  They are really solid rods, but representing tubes on the real car. Indulge me. Another 2 tubes molded in the same part, carry the space frame back to the top of the transmission, and support the rear suspension spring and shock rockers. The broad, vertical, “bulkhead” that carries suspension loads directly to the space frame, first seen in the 956, are also included in part A34. Two additional “V” shaped tube pairs, A25 and A26, join vertical tubes in the frame on the back of the fireall, complete the 3d linkage of firewall tube frame and the rear suspension bulkhead.

Its probably a kindness that all this complexity is molded into one plastic part. Consider a loose bulkhead linked to the firewall by 4 loose V shapes. Not unlike building biplanes with wings and struts, and no 3D structures. Possible, but difficult. Nicer if the struts are long enough and strong enough to hold the wings in a fixed relationship. And absolutely depending on the geometry of the holes in the wing. Count on using the chassis bottom as tooling to hold the firewall and transmission in perfect alignment while the space frame to firewall joints are drying. Put plastic wrap or wax paper on the tray under the engine, and where the firewall would glue to the driver’s floor re-enforcement.

The rest of the rear suspension is equally concise, one piece for lower “A” arms. one piece for the upper parallel links, with a diagonal brace. The rear suspension push rod, rocker and spring/shock absorber are one piece for each side. Two sets of external shock absorber reservoirs are provided, one pair molded on the anti-roll bar cover and a second pair molded at the top front of the final drive, with ajustment knobs as part of A34.  The drive shafts are integral with half the rear hub. A separate “other half” makes a full hub, which locks between the lower and upper suspension arms and plus into the transmission final drive.

Structure is similarly concise: The tube frame that braces the exhaust pipes, contains the breather catch tank, holds both the air-jack connection point, supports the access for the externally adjustable anti-roll bar and serves as the lifting eye for the back of the car is 2 pieces, and a little bas-relief on the catch-tank, which is integral with the 2 sides of the transmission.

The radiator is supplied, the guide vanes for hot air that exits through the trunk lid, and the air intake in the nose, with lifting eye. But nothing else from inside the front body shell is provided. No radiator exhaust duct, front brake cooling ducts, stock 993 front chassis sheet metal, fuel cell and filler/vent hardware, or any of the fiddly suspension or fuel system bits.

The “chromed” parts should mostly be painted bare metal- aluminum for the air intakes, stainless steel for the exhaust. No clues are provided for the extensive hoses and cables that should dress the engine and transmission, the instruments, console controls and passenger seat area. A few details are wrong or misunderstood, but most detail is correct. There’s just a lot isn’t present.

The ugliest problem built into the kit as the inside of the rear brake cooling air scoops. It should be a large, rectangular cross section air duct on each side.  Instead, there’s a bizarre and inaccurate insert that makes a 90 degree turn in the airflow, then very odd “duct work” that assembles to join the 90 degree bend with a hard, white, plastic piece representing the “soft duct” that connects to the cooling air director on the rear hub. The soft pieces can be used The ducts and scoop fittings should not be used.

The next inaccurate detail is the catch tank. Its provided as a parallelogram, fore and aft edges parallel, top and bottom faces parallel, corners between them more or less than 90 degrees. The actual part is a rectangle, I’ve photographed it, its a translucent blow-molding, with a 930- (911 Turbo) part number and can be seen easily in engine compartments from 917s to 962s.

After that, the sins are mostly omission. Few of the cables and pipes/hoses in the engine compartment are provided or suggested, except for a bas relief drain tube on the catch tank. and an oil-return among the top-of-engine coolant plumbing. None of the cables in the passenger side of the cockpit.are provided or suggested. No engine management box, no Bowden cable from accelerator to throttle butterfly valves. No computer or power connector.  One fuel filter is on one intake manifold, but not the other, and the suggested injector rail and hoses are very poor and small.