Defence Against the Dark Arts

Transcript

1. Defence Against the Dark Arts
2. About Softwire
3. Where are we?
4. About Me
5. What are the “Dark Arts”?
6. Programming Is Magic
7. Good
8. Evil
9. Malicious
10. Incompetent
11. Virtues
12. Honesty The truth, the whole truth and nothing but the truth.
13. Naming
14. Bad Names doStuff validate getInfo AbstractMagicController MagicTable temp
15. Good Names bookHotelRoom validateUser getRestaurantMenu TimetableViewController TimetableView temp
16. Lies of Omission Side effects are bad
17. public function __construct($charge_id) { $this->_chargeId = $charge_id; $this->populate($charge_id); }
18. Comments
19. They should be informative... /** * Constructor */
20. They should not state the obvious... /** * Performs initialisation of controller * * @return void */ public function init() { // Initialise... }
21. They should be brief... // This method takes two integer values and adds them together via the built-in // .NET functionality. It would be possible to code the arithmetic function // by hand, but since .NET provides it, that would be a waste of time private int Add(int i, int j) // i is the first value, j is the second value { // add the numbers together using the .NET "+" operator int z = i + j; // return the value to the calling function // return z; // this code was updated to simplify the return statement, eliminating the need // for a separate variable. // this statement performs the add functionality using the + operator on the two // parameter values, and then returns the result to the calling function return i + j; }
22. They should provide useful extra information... /** * Set booking type * * @param bookingType a value from the Booker.BookingType enumeration. * * @see my.example.Booker.BookingType */ public void setBookingType(Booker.BookingType bookingType) { var log = Log.beginFunction("setBookingType"); log.normal("Set BookingType " + bookingType); BookingType = bookingType; log.endFunction(); }
23. They should be spelled correctly... Propertiary
24. Honesty • Tell the truth → name things accurately • The whole truth → side effects are bad • Nothing but the truth → your comments will rot
25. Brevity Writing is 1 percent inspiration and 99 percent elimination.
26. Example: Brevity public function sessionbookingsAction() { $params = $this->getRequest()->getParams(); $course_id = (int) $this->getRequest()->getUserParam('course_id', 0); $this->view->headLink()->appendStylesheet('/css/sessionbookings.v06112012.css', 'screen,print'); $this->view->headLink()->appendStylesheet('/public_shared/css/coursemigration.css', 'screen,print'); $this->view->headLink()->appendStylesheet('/css/commonbookingforms.css', 'screen,print'); $this->view->inlineScript()->appendFile('/js/booking/sessionbookings.v06112012.js'); // list.js used for nice JS sorting $this->view->inlineScript()->appendFile('/public_shared/js/list.min.js'); $this->view->inlineScript()->appendFile('/public_shared/js/spin.min.js'); $player_id = (int) $this->getRequest()->getUserParam('player_id', 0); if ($player_id > 0) { $this->view->player_id = $player_id; } $errors = array(); $fatalErrors = array(); $bookedMembersViewModel = array(); $bookedNonMembers = array(); $fatal = false; // if an error message param is passed show it if (isset($params['errormsg'])) { switch($params['errormsg']){ case 'missparams': $fatalErrors[] = 'Missing required parameters'; $fatal = true; break; case 'badcourse'; $replace = array( 'type' => 'Course or class', );
27. private string GetRandomString(int numberOfChars) { string sRandomString = ""; Random oRand = null; char[] arrCharArray = new char[36] {'a','b','c','d','e','f','g','h','i','j','k','l','m','n','o','p','q','r','s','t','u','v','w','x','y','z','0','1','2','3','4','5','6','7','8','9'}; try { if (numberOfChars <= 1) { sRandomString = string.Empty; } oRand = new Random(); for (int x = 0; x <= numberOfChars; x++) { sRandomString = sRandomString + arrCharArray[oRand.Next(0, 34)]; } if (Directory.Exists(sRandomString)) { sRandomString = GetRandomString(numberOfChars); } else { sRandomString = string.Empty; } } catch (Exception ex) { throw new Exception(string.Concat(MethodBase.GetCurrentMethod().Name, ex.Message)); } finally { sRandomString = null; oRand = null; arrCharArray = null; } return sRandomString; }
28. Long Methods Hides Bugs
29. function validatePassword($email, $password) { $user = getUser($email); if ($password == $user->password || $user->isEmployee) { // Perform login } }
30. Keep Things Short
31. public function cancelClass() { $this->_cancelCourts(); $this->_cancelCoaches(); $this->_setStatusToCancelled(); $this->_cancelMemberBookings(); if ($this->isOpenToNonMembers()){ $this->cancelAllNonMemberBookings(); } }
32. Simplicity If you can’t explain it to a six year old, you don’t understand it yourself.
33. We Know You’re Clever
34. Database Calculator
35. Don’t Make Me Think
36. if (!m_dontSendEmail) { ... } vs if (sendEmail) { … }
37. Don’t Reinvent the Wheel
38. True, False, FileNotFound enum Bool { True, False, FileNotFound };
39. Complex inheritance Premature optimisation Over generalising
40. YAGNI
41. Bonus Awful
42. foreach ($links as $link) { ${$link}->delete(array('instance_id = ?' => $instance['instance_id'])); foreach ($instance[$link . '_id'] as $item) { ${$link}->insert(array( 'instance_id' => $instance['instance_id'], $link . '_id' => $item )); } } $links = array('coach', 'court'); $coach = new CoachTable(); $court = new CourtTable(); // Several lines of unrelated code...
43. public function cancelBooking($bookingRef, $notes = '', $description = '', $cancellationReason = null, $resourcesRequired = null) { $validate = array( self::BOOKING_REF => $bookingRef, self::BOOKING_INFO_REQ__BOOKING => array( self::BOOKING_NOTES => $notes, self::BOOKING_DESCRIPTION => $description ), self::BOOKING_INFO_REQ__CANCELLATION_REASON => $cancellationReason, self::BOOKING_INFO__RESOURCES_REQUIRED => $resourcesRequired, ); $this->validate($validate); return $this->makeBooking(self::BOOKING_STATUS__CANCELLED); }
44. public function validate($requestParams) { Service_Profiler::profileStart(__METHOD__); $this->validated = false; // testing & debugging $this->bookingInfo = null; // avoid residual information $this->_bookedPlayersCheckedForAvailability = false; // allow players to be checked once per validate! $assembly = new Service_Booking_ResourceAssembly($requestParams, $this->_db); $this->bookingInfo = $assembly->getBookingInfo(); $rights = new Service_Booking_BookingRights($this->_db); $this->bookingRights = $rights->getBookingRights(); $rules = new Service_Booking_BookingRules($requestParams, $this->_db); $this->bookingInfo = $rules->getBookingInfo(); // Validate resources - are we able to 'book' these? if (isset($this->bookingInfo[self::BOOKING_INFO__RESOURCES][self::BOOKING_INFO_RES__BOOKED]) || isset($this->bookingInfo[self::BOOKING_INFO__RESOURCES][self::BOOKING_INFO_RES__NEW]) || empty($requestParams[self::BOOKING_INFO__RESOURCES]) ) { $this->validated = true; } Service_Profiler::profileStop(__METHOD__); }
46. Q: How do people get into this mess?
47. A: Not intentionally
48. Eternal Vigilance Refactor early, refactor often
49. Caveat Rules are there to be broken.
50. /** * Given two intervals, we check if they overlap for any period of time * See http://stackoverflow.com/questions/3269434 for the logic behind this * * @return bool */ public function checkDatePeriodsOverlap($period1Start, $period1End, $period2Start, $period2End) { return $period1Start->compare($period2End) < 0 && $period2Start->compare($period1End) < 0; }
51. Honesty Brevity Simplicity Eternal Vigilance
52. Further Reading
53. Clean Code Robert C. Martin
54. Daily “Worse Than Failure” www.thedailywtf.com
55. Softwire softwire.com | @SoftwireUK