Refactoring

Martin Fowler, Kent Beck

Mentioned 148

Users can dramatically improve the design, performance, and manageability of object-oriented code without altering its interfaces or behavior. "Refactoring" shows users exactly how to spot the best opportunities for refactoring and exactly how to do it, step by step.

More on Amazon.com

Mentioned in questions and answers.

If you could go back in time and tell yourself to read a specific book at the beginning of your career as a developer, which book would it be?

I expect this list to be varied and to cover a wide range of things.

To search: Use the search box in the upper-right corner. To search the answers of the current question, use inquestion:this. For example:

inquestion:this "Code Complete"

Applying UML and Patterns by Craig Larman.

The title of the book is slightly misleading; it does deal with UML and patterns, but it covers so much more. The subtitle of the book tells you a bit more: An Introduction to Object-Oriented Analysis and Design and Iterative Development.

Masters of doom. As far as motivation and love for your profession go: it won't get any better than what's been described in this book, truthfully inspiring story!

Beginning C# 3.0: An Introduction to Object Oriented Programming

This is the book for those who want to understand the whys and hows of OOP using C# 3.0. You don't want to miss it.

alt text

Mastery: The Keys to Success and Long-Term Fulfillment, by George Leonard

It's about about what mindsets are required to reach mastery in any skill, and why. It's just awesome, and an easy read too.

Pro Spring is a superb introduction to the world of Inversion of Control and Dependency Injection. If you're not aware of these practices and their implications - the balance of topics and technical detail in Pro Spring is excellent. It builds a great case and consequent personal foundation.

Another book I'd suggest would be Robert Martin's Agile Software Development (ASD). Code smells, agile techniques, test driven dev, principles ... a well-written balance of many different programming facets.

More traditional classics would include the infamous GoF Design Patterns, Bertrand Meyer's Object Oriented Software Construction, Booch's Object Oriented Analysis and Design, Scott Meyer's "Effective C++'" series and a lesser known book I enjoyed by Gunderloy, Coder to Developer.

And while books are nice ... don't forget radio!

... let me add one more thing. If you haven't already discovered safari - take a look. It is more addictive than stack overflow :-) I've found that with my google type habits - I need the more expensive subscription so I can look at any book at any time - but I'd recommend the trial to anyone even remotely interested.

(ah yes, a little obj-C today, cocoa tomorrow, patterns? soa? what was that example in that cookbook? What did Steve say in the second edition? Should I buy this book? ... a subscription like this is great if you'd like some continuity and context to what you're googling ...)

Database System Concepts is one of the best books you can read on understanding good database design principles.

alt text

Algorithms in C++ was invaluable to me in learning Big O notation and the ins and outs of the various sort algorithms. This was published before Sedgewick decided he could make more money by dividing it into 5 different books.

C++ FAQs is an amazing book that really shows you what you should and shouldn't be doing in C++. The backward compatibility of C++ leaves a lot of landmines about and this book helps one carefully avoid them while at the same time being a good introduction into OO design and intent.

Here are two I haven't seen mentioned:
I wish I had read "Ruminations on C++" by Koenig and Moo much sooner. That was the book that made OO concepts really click for me.
And I recommend Michael Abrash's "Zen of Code Optimization" for anyone else planning on starting a programming career in the mid 90s.

Perfect Software: And Other Illusions about Testing

TITLE Cover

Perfect Software: And Other Illusions about Testing by Gerald M. Weinberg

ISBN-10: 0932633692

ISBN-13: 978-0932633699

Rapid Development by McConnell

The most influential programming book for me was Enough Rope to Shoot Yourself in the Foot by Allen Holub.

Cover of the book

O, well, how long ago it was.

I have a few good books that strongly influenced me that I've not seen on this list so far:

The Psychology of Everyday Things by Donald Norman. The general principles of design for other people. This may seem to be mostly good for UI but if you think about it, it has applications almost anywhere there is an interface that someone besides the original developer has to work with; e. g. an API and designing the interface in such a way that other developers form the correct mental model and get appropriate feedback from the API itself.

The Art of Software Testing by Glen Myers. A good, general introduction to testing software; good for programmers to read to help them think like a tester i. e. think of what may go wrong and prepare for it.

By the way, I realize the question was the "Single Most Influential Book" but the discussion seems to have changed to listing good books for developers to read so I hope I can be forgiven for listing two good books rather than just one.

alt text

C++ How to Program It is good for beginner.This is excellent book that full complete with 1500 pages.

Effective C++ and More Effective C++ by Scott Myers.

Inside the C++ object model by Stanley Lippman

I bough this when I was a complete newbie and took me from only knowing that Java existed to a reliable team member in a short time

Not a programming book, but still a very important book every programmer should read:

Orbiting the Giant Hairball by Gordon MacKenzie

The Pragmatic programmer was pretty good. However one that really made an impact when I was starting out was :

Windows 95 System Programming Secrets"

I know - it sounds and looks a bit cheesy on the outside and has probably dated a bit - but this was an awesome explanation of the internals of Win95 based on the Authors (Matt Pietrek) investigations using his own own tools - the code for which came with the book. Bear in mind this was before the whole open source thing and Microsoft was still pretty cagey about releasing documentation of internals - let alone source. There was some quote in there like "If you are working through some problem and hit some sticking point then you need to stop and really look deeply into that piece and really understand how it works". I've found this to be pretty good advice - particularly these days when you often have the source for a library and can go take a look. Its also inspired me to enjoy diving into the internals of how systems work, something that has proven invaluable over the course of my career.

Oh and I'd also throw in effective .net - great internals explanation of .Net from Don Box.

I recently read Dreaming in Code and found it to be an interesting read. Perhaps more so since the day I started reading it Chandler 1.0 was released. Reading about the growing pains and mistakes of a project team of talented people trying to "change the world" gives you a lot to learn from. Also Scott brings up a lot of programmer lore and wisdom in between that's just an entertaining read.

Beautiful Code had one or two things that made me think differently, particularly the chapter on top down operator precedence.

K&R

@Juan: I know Juan, I know - but there are some things that can only be learned by actually getting down to the task at hand. Speaking in abstract ideals all day simply makes you into an academic. It's in the application of the abstract that we truly grok the reason for their existence. :P

@Keith: Great mention of "The Inmates are Running the Asylum" by Alan Cooper - an eye opener for certain, any developer that has worked with me since I read that book has heard me mention the ideas it espouses. +1

I found the The Algorithm Design Manual to be a very beneficial read. I also highly recommend Programming Pearls.

This one isnt really a book for the beginning programmer, but if you're looking for SOA design books, then SOA in Practice: The Art of Distributed System Design is for you.

For me it was Design Patterns Explained it provided an 'Oh that's how it works' moment for me in regards to design patterns and has been very useful when teaching design patterns to others.

Code Craft by Pete Goodliffe is a good read!

Code Craft

The first book that made a real impact on me was Mastering Turbo Assembler by Tom Swan.

Other books that have had an impact was Just For Fun by Linus Torvalds and David Diamond and of course The Pragmatic Programmer by Andrew Hunt and David Thomas.

In addition to other people's suggestions, I'd recommend either acquiring a copy of SICP, or reading it online. It's one of the few books that I've read that I feel greatly increased my skill in designing software, particularly in creating good abstraction layers.

A book that is not directly related to programming, but is also a good read for programmers (IMO) is Concrete Mathematics. Most, if not all of the topics in it are useful for programmers to know about, and it does a better job of explaining things than any other math book I've read to date.

For me "Memory as a programming concept in C and C++" really opened my eyes to how memory management really works. If you're a C or C++ developer I consider it a must read. You will defiantly learn something or remember things you might have forgotten along the way.

http://www.amazon.com/Memory-Programming-Concept-C/dp/0521520436

Agile Software Development with Scrum by Ken Schwaber and Mike Beedle.

I used this book as the starting point to understanding Agile development.

Systemantics: How Systems Work and Especially How They Fail. Get it used cheap. But you might not get the humor until you've worked on a few failed projects.

The beauty of the book is the copyright year.

Probably the most profound takeaway "law" presented in the book:

The Fundamental Failure-Mode Theorem (F.F.T.): Complex systems usually operate in failure mode.

The idea being that there are failing parts in any given piece of software that are masked by failures in other parts or by validations in other parts. See a real-world example at the Therac-25 radiation machine, whose software flaws were masked by hardware failsafes. When the hardware failsafes were removed, the software race condition that had gone undetected all those years resulted in the machine killing 3 people.

It seems most people have already touched on the some very good books. One which really helped me out was Effective C#: 50 Ways to Improve your C#. I'd be remiss if I didn't mention The Tao of Pooh. Philosophy books can be good for the soul, and the code.

Discrete Mathematics For Computer Scientists

Discrete Mathematics For Computer Scientists by J.K. Truss.

While this doesn't teach you programming, it teaches you fundamental mathematics that every programmer should know. You may remember this stuff from university, but really, doing predicate logic will improve you programming skills, you need to learn Set Theory if you want to program using collections.

There really is a lot of interesting information in here that can get you thinking about problems in different ways. It's handy to have, just to pick up once in a while to learn something new.

I saw a review of Software Factories: Assembling Applications with Patterns, Models, Frameworks, and Tools on a blog talking also about XI-Factory, I read it and I must say this book is a must read. Altough not specifically targetted to programmers, it explains very clearly what is happening in the programming world right now with Model-Driven Architecture and so on..

Solid Code Optimizing the Software Development Life Cycle

Although the book is only 300 pages and favors Microsoft technologies it still offers some good language agnostic tidbits.

Managing Gigabytes is an instant classic for thinking about the heavy lifting of information.

My vote is "How to Think Like a Computer Scientist: Learning With Python" It's available both as a book and as a free e-book.

It really helped me to understand the basics of not just Python but programming in general. Although it uses Python to demonstrate concepts, they apply to most, if not all, programming languages. Also: IT'S FREE!

Object-Oriented Programming in Turbo C++. Not super popular, but it was the one that got me started, and was the first book that really helped me grok what an object was. Read this one waaaay back in high school. It sort of brings a tear to my eye...

My high school math teacher lent me a copy of Are Your Lights Figure Problem that I have re-read many times. It has been invaluable, as a developer, and in life generally.

I'm reading now Agile Software Development, Principles, Patterns and Practices. For those interested in XP and Object-Oriented Design, this is a classic reading.

alt text

Kernighan & Plauger's Elements of Programming Style. It illustrates the difference between gimmicky-clever and elegant-clever.

to get advanced in prolog i like these two books:

The Art of Prolog

The Craft of Prolog

really opens the mind for logic programming and recursion schemes.

Here's an excellent book that is not as widely applauded, but is full of deep insight: Agile Software Development: The Cooperative Game, by Alistair Cockburn.

What's so special about it? Well, clearly everyone has heard the term "Agile", and it seems most are believers these days. Whether you believe or not, though, there are some deep principles behind why the Agile movement exists. This book uncovers and articulates these principles in a precise, scientific way. Some of the principles are (btw, these are my words, not Alistair's):

  1. The hardest thing about team software development is getting everyone's brains to have the same understanding. We are building huge, elaborate, complex systems which are invisible in the tangible world. The better you are at getting more peoples' brains to share deeper understanding, the more effective your team will be at software development. This is the underlying reason that pair programming makes sense. Most people dismiss it (and I did too initially), but with this principle in mind I highly recommend that you give it another shot. You wind up with TWO people who deeply understand the subsystem you just built ... there aren't many other ways to get such a deep information transfer so quickly. It is like a Vulcan mind meld.
  2. You don't always need words to communicate deep understanding quickly. And a corollary: too many words, and you exceed the listener/reader's capacity, meaning the understanding transfer you're attempting does not happen. Consider that children learn how to speak language by being "immersed" and "absorbing". Not just language either ... he gives the example of some kids playing with trains on the floor. Along comes another kid who has never even SEEN a train before ... but by watching the other kids, he picks up the gist of the game and plays right along. This happens all the time between humans. This along with the corollary about too many words helps you see how misguided it was in the old "waterfall" days to try to write 700 page detailed requirements specifications.

There is so much more in there too. I'll shut up now, but I HIGHLY recommend this book!

alt text

The Back of the Napkin, by Dan Roam.

The Back of the Napkin

A great book about visual thinking techniques. There is also an expanded edition now. I can't speak to that version, as I do not own it; yet.

Agile Software Development by Alistair Cockburn

Do users ever touch your code? If you're not doing solely back-end work, I recommend About Face: The Essentials of User Interface Design — now in its third edition (linked). I used to think my users were stupid because they didn't "get" my interfaces. I was, of course, wrong. About Face turned me around.

"Writing Solid Code: Microsoft's Techniques for Developing Bug-Free C Programs (Microsoft Programming Series)" by Steve MacGuire.

Interesting what a large proportion the books mentioned here are C/C++ books.

While not strictly a software development book, I would highly recommend that Don't Make me Think! be considered in this list.

As so many people have listed Head First Design Patterns, which I agree is a very good book, I would like to see if so many people aware of a title called Design Patterns Explained: A New Perspective on Object-Oriented Design.

This title deals with design patterns excellently. The first half of the book is very accessible and the remaining chapters require only a firm grasp of the content already covered The reason I feel the second half of the book is less accessible is that it covers patterns that I, as a young developer admittedly lacking in experience, have not used much.

This title also introduces the concept behind design patterns, covering Christopher Alexander's initial work in architecture to the GoF first implementing documenting patterns in SmallTalk.

I think that anyone who enjoyed Head First Design Patterns but still finds the GoF very dry, should look into Design Patterns Explained as a much more readable (although not quite as comprehensive) alternative.

Even though i've never programmed a game this book helped me understand a lot of things in a fun way.

How influential a book is often depends on the reader and where they were in their career when they read the book. I have to give a shout-out to Head First Design Patterns. Great book and the very creative way it's written should be used as an example for other tech book writers. I.e. it's written in order to facilitate learning and internalizing the concepts.

Head First Design Patterns

97 Things Every Programmer Should Know

alt text

This book pools together the collective experiences of some of the world's best programmers. It is a must read.

Extreme Programming Explained: Embrace Change by Kent Beck. While I don't advocate a hardcore XP-or-the-highway take on software development, I wish I had been introduced to the principles in this book much earlier in my career. Unit testing, refactoring, simplicity, continuous integration, cost/time/quality/scope - these changed the way I looked at development. Before Agile, it was all about the debugger and fear of change requests. After Agile, those demons did not loom as large.

One of my personal favorites is Hacker's Delight, because it was as much fun to read as it was educational.

I hope the second edition will be released soon!

You.Next(): Move Your Software Development Career to the Leadership Track ~ Michael C. Finley (Author), Honza Fedák (Author) link text

I've been arounda while, so most books that I have found influential don't necessarily apply today. I do believe it is universally important to understand the platform that you are developing for (both hardware and OS). I also think it's important to learn from other peoples mistakes. So two books I would recommend are:

Computing Calamities and In Search of Stupidity: Over Twenty Years of High Tech Marketing Disasters

Working Effectively with Legacy Code is a really amazing book that goes into great detail about how to properly unit test your code and what the true benefit of it is. It really opened my eyes.

Are there good reasons why it's a better practice to have only one return statement in a function?

Or is it okay to return from a function as soon as it is logically correct to do so, meaning there may be many return statements in the function?

Structured programming says you should only ever have one return statement per function. This is to limit the complexity. Many people such as Martin Fowler argue that it is simpler to write functions with multiple return statements. He presents this argument in the classic refactoring book he wrote. This works well if you follow his other advice and write small functions. I agree with this point of view and only strict structured programming purists adhere to single return statements per function.

It wasn't that long ago that I was a beginning coder, trying to find good books/tutorials on languages I wanted to learn. Even still, there are times I need to pick up a language relatively quickly for a new project I am working on. The point of this post is to document some of the best tutorials and books for these languages. I will start the list with the best I can find, but hope you guys out there can help with better suggestions/new languages. Here is what I found:

Since this is now wiki editable, I am giving control up to the community. If you have a suggestion, please put it in this section. I decided to also add a section for general be a better programmer books and online references as well. Once again, all recommendations are welcome.

General Programming

Online Tutorials
Foundations of Programming By Karl Seguin - From Codebetter, its C# based but the ideas ring true across the board, can't believe no-one's posted this yet actually.
How to Write Unmaintainable Code - An anti manual that teaches you how to write code in the most unmaintable way possible. It would be funny if a lot of these suggestions didn't ring so true.
The Programming Section of Wiki Books - suggested by Jim Robert as having a large amount of books/tutorials on multiple languages in various stages of completion
Just the Basics To get a feel for a language.

Books
Code Complete - This book goes without saying, it is truely brilliant in too many ways to mention.
The Pragmatic Programmer - The next best thing to working with a master coder, teaching you everything they know.
Mastering Regular Expressions - Regular Expressions are an essential tool in every programmer's toolbox. This book, recommended by Patrick Lozzi is a great way to learn what they are capable of.
Algorithms in C, C++, and Java - A great way to learn all the classic algorithms if you find Knuth's books a bit too in depth.

C

Online Tutorials
This tutorial seems to pretty consise and thourough, looked over the material and seems to be pretty good. Not sure how friendly it would be to new programmers though.
Books
K&R C - a classic for sure. It might be argued that all programmers should read it.
C Primer Plus - Suggested by Imran as being the ultimate C book for beginning programmers.
C: A Reference Manual - A great reference recommended by Patrick Lozzi.

C++

Online Tutorials
The tutorial on cplusplus.com seems to be the most complete. I found another tutorial here but it doesn't include topics like polymorphism, which I believe is essential. If you are coming from C, this tutorial might be the best for you.

Another useful tutorial, C++ Annotation. In Ubuntu family you can get the ebook on multiple format(pdf, txt, Postscript, and LaTex) by installing c++-annotation package from Synaptic(installed package can be found in /usr/share/doc/c++-annotation/.

Books
The C++ Programming Language - crucial for any C++ programmer.
C++ Primer Plus - Orginally added as a typo, but the amazon reviews are so good, I am going to keep it here until someone says it is a dud.
Effective C++ - Ways to improve your C++ programs.
More Effective C++ - Continuation of Effective C++.
Effective STL - Ways to improve your use of the STL.
Thinking in C++ - Great book, both volumes. Written by Bruce Eckel and Chuck Ellison.
Programming: Principles and Practice Using C++ - Stroustrup's introduction to C++.
Accelerated C++ - Andy Koenig and Barbara Moo - An excellent introduction to C++ that doesn't treat C++ as "C with extra bits bolted on", in fact you dive straight in and start using STL early on.

Forth

Books
FORTH, a text and reference. Mahlon G. Kelly and Nicholas Spies. ISBN 0-13-326349-5 / ISBN 0-13-326331-2. 1986 Prentice-Hall. Leo Brodie's books are good but this book is even better. For instance it covers defining words and the interpreter in depth.

Java

Online Tutorials
Sun's Java Tutorials - An official tutorial that seems thourough, but I am not a java expert. You guys know of any better ones?
Books
Head First Java - Recommended as a great introductory text by Patrick Lozzi.
Effective Java - Recommended by pek as a great intermediate text.
Core Java Volume 1 and Core Java Volume 2 - Suggested by FreeMemory as some of the best java references available.
Java Concurrency in Practice - Recommended by MDC as great resource for concurrent programming in Java.

The Java Programing Language

Python

Online Tutorials
Python.org - The online documentation for this language is pretty good. If you know of any better let me know.
Dive Into Python - Suggested by Nickola. Seems to be a python book online.

Perl

Online Tutorials
perldoc perl - This is how I personally got started with the language, and I don't think you will be able to beat it.
Books
Learning Perl - a great way to introduce yourself to the language.
Programming Perl - greatly referred to as the Perl Bible. Essential reference for any serious perl programmer.
Perl Cookbook - A great book that has solutions to many common problems.
Modern Perl Programming - newly released, contains the latest wisdom on modern techniques and tools, including Moose and DBIx::Class.

Ruby

Online Tutorials
Adam Mika suggested Why's (Poignant) Guide to Ruby but after taking a look at it, I don't know if it is for everyone. Found this site which seems to offer several tutorials for Ruby on Rails.
Books
Programming Ruby - suggested as a great reference for all things ruby.

Visual Basic

Online Tutorials
Found this site which seems to devote itself to visual basic tutorials. Not sure how good they are though.

PHP

Online Tutorials
The main PHP site - A simple tutorial that allows user comments for each page, which I really like. PHPFreaks Tutorials - Various tutorials of different difficulty lengths.
Quakenet/PHP tutorials - PHP tutorial that will guide you from ground up.

JavaScript

Online Tutorials
Found a decent tutorial here geared toward non-programmers. Found another more advanced one here. Nickolay suggested A reintroduction to javascript as a good read here.

Books
Head first JavaScript
JavaScript: The Good Parts (with a Google Tech Talk video by the author)

C#

Online Tutorials
C# Station Tutorial - Seems to be a decent tutorial that I dug up, but I am not a C# guy.
C# Language Specification - Suggested by tamberg. Not really a tutorial, but a great reference on all the elements of C#
Books
C# to the point - suggested by tamberg as a short text that explains the language in amazing depth

ocaml

Books
nlucaroni suggested the following:
OCaml for Scientists Introduction to ocaml
Using Understand and unraveling ocaml: practice to theory and vice versa
Developing Applications using Ocaml - O'Reilly
The Objective Caml System - Official Manua

Haskell

Online Tutorials
nlucaroni suggested the following:
Explore functional programming with Haskell
Books
Real World Haskell
Total Functional Programming

LISP/Scheme

Books
wfarr suggested the following:
The Little Schemer - Introduction to Scheme and functional programming in general
The Seasoned Schemer - Followup to Little Schemer.
Structure and Interpretation of Computer Programs - The definitive book on Lisp (also available online).
Practical Common Lisp - A good introduction to Lisp with several examples of practical use.
On Lisp - Advanced Topics in Lisp
How to Design Programs - An Introduction to Computing and Programming
Paradigms of Artificial Intelligence Programming: Case Studies in Common Lisp - an approach to high quality Lisp programming

What about you guys? Am I totally off on some of there? Did I leave out your favorite language? I will take the best comments and modify the question with the suggestions.

Java: SCJP for Java 6. I still use it as a reference.

Haskell:

O'Reilly Book:

  1. Real World Haskell, a great tutorial-oriented book on Haskell, available online and in print.

My favorite general, less academic online tutorials:

  1. The Haskell wikibook which contains all of the excellent Yet Another Haskell Tutorial. (This tutorial helps with specifics of setting up a Haskell distro and running example programs, for example.)
  2. Learn you a Haskell for Great Good, in the spirit of Why's Poignant Guide to Ruby but more to the point.
  3. Write yourself a Scheme in 48 hours. Get your hands dirty learning Haskell with a real project.

Books on Functional Programming with Haskell:

  1. Lambda calculus, combinators, more theoretical, but in a very down to earth manner: Davie's Introduction to Functional Programming Systems Using Haskell
  2. Laziness and program correctness, thinking functionally: Bird's Introduction to Functional Programming Using Haskell

Some books on Java I'd recommend:

For Beginners: Head First Java is an excellent introduction to the language. And I must also mention Head First Design Patterns which is a great resource for learners to grasp what can be quite challenging concepts. The easy-going fun style of these books are ideal for ppl new to programming.

A really thorough, comprehensive book on Java SE is Bruce Eckel's Thinking In Java v4. (At just under 1500 pages it's good for weight-training as well!) For those of us not on fat bank-bonuses there are older versions available for free download.

Of course, as many ppl have already mentioned, Josh Bloch's Effective Java v2 is an essential part of any Java developer's library.

Let's not forget Head First Java, which could be considered the essential first step in this language or maybe the step after the online tutorials by Sun. It's great for the purpose of grasping the language concisely, while adding a bit of fun, serving as a stepping stone for the more in-depth books already mentioned.

Sedgewick offers great series on Algorithms which are a must-have if you find Knuth's books to be too in-depth. Knuth aside, Sedgewick brings a solid approach to the field and he offers his books in C, C++ and Java. The C++ books could be used backwardly on C since he doesn't make a very large distinction between the two languages in his presentation.

Whenever I'm working on C, C:A Reference Manual, by Harbison and Steele, goes with me everywhere. It's concise and efficient while being extremely thorough making it priceless(to me anyways).

Languages aside, and if this thread is to become a go-to for references in which I think it's heading that way due to the number of solid contributions, please include Mastering Regular Expressions, for reasons I think most of us are aware of... some would also say that regex can be considered a language in its own right. Further, its usefulness in a wide array of languages makes it invaluable.

C: “Programming in C”, Stephen G. Kochan, Developer's Library.

Organized, clear, elaborate, beautiful.

C++

The first one is good for beginners and the second one requires more advanced level in C++.

I know this is a cross post from here... but, I think one of the best Java books is Java Concurrency in Practice by Brian Goetz. A rather advanced book - but, it will wear well on your concurrent code and Java development in general.

C#

C# to the Point by Hanspeter Mössenböck. On a mere 200 pages he explains C# in astonishing depth, focusing on underlying concepts and concise examples rather than hand waving and Visual Studio screenshots.

For additional information on specific language features, check the C# language specification ECMA-334.

Framework Design Guidelines, a book by Krzysztof Cwalina and Brad Abrams from Microsoft, provides further insight into the main design decisions behind the .NET library.

For Lisp and Scheme (hell, functional programming in general), there are few things that provide a more solid foundation than The Little Schemer and The Seasoned Schemer. Both provide a very simple and intuitive introduction to both Scheme and functional programming that proves far simpler for new students or hobbyists than any of the typical volumes that rub off like a nonfiction rendition of War & Peace.

Once they've moved beyond the Schemer series, SICP and On Lisp are both fantastic choices.

For C++ I am a big fan of C++ Common Knowledge: Essential Intermediate Programming, I like that it is organized into small sections (usually less than 5 pages per topic) So it is easy for me to grab it and read up on concepts that I need to review.

It is a must read for me the night before and on the plane to a job interview.

C Primer Plus, 5th Edition - The C book to get if you're learning C without any prior programming experience. It's a personal favorite of mine as I learned to program from this book. It has all the qualities a beginner friendly book should have:

  • Doesn't assume any prior exposure to programming
  • Enjoyable to read (without becoming annoying like For Dummies /
  • Doesn't oversimplify

For Javascript:

For PHP:

For OO design & programming, patterns:

For Refactoring:

For SQL/MySQL:

  • C - The C Programming Language - Obviously I had to reference K&R, one of the best programming books out there full stop.
  • C++ - Accelerated C++ - This clear, well written introduction to C++ goes straight to using the STL and gives nice, clear, practical examples. Lives up to its name.
  • C# - Pro C# 2008 and the .NET 3.5 Platform - Bit of a mouthful but wonderfully written and huge depth.
  • F# - Expert F# - Designed to take experienced programmers from zero to expert in F#. Very well written, one of the author's invented F# so you can't go far wrong!
  • Scheme - The Little Schemer - Really unique approach to teaching a programming language done really well.
  • Ruby - Programming Ruby - Affectionately known as the 'pick axe' book, this is THE defacto introduction to Ruby. Very well written, clear and detailed.

So we have this huge (is 11000 lines huge?) mainmodule.cpp source file in our project and every time I have to touch it I cringe.

As this file is so central and large, it keeps accumulating more and more code and I can't think of a good way to make it actually start to shrink.

The file is used and actively changed in several (> 10) maintenance versions of our product and so it is really hard to refactor it. If I were to "simply" split it up, say for a start, into 3 files, then merging back changes from maintenance versions will become a nightmare. And also if you split up a file with such a long and rich history, tracking and checking old changes in the SCC history suddenly becomes a lot harder.

The file basically contains the "main class" (main internal work dispatching and coordination) of our program, so every time a feature is added, it also affects this file and every time it grows. :-(

What would you do in this situation? Any ideas on how to move new features to a separate source file without messing up the SCC workflow?

(Note on the tools: We use C++ with Visual Studio; We use AccuRev as SCC but I think the type of SCC doesn't really matter here; We use Araxis Merge to do actual comparison and merging of files)

Another book you may find interesting/helpful is Refactoring.

My sympathies - in my previous job I encountered a similar situation with a file that was several times larger than the one you have to deal with. Solution was:

  1. Write code to exhaustively test the function in the program in question. Sounds like you won't already have this in hand...
  2. Identify some code that can be abstracted out into a helper/utilities class. Need not be big, just something that is not truly part of your 'main' class.
  3. Refactor the code identified in 2. into a separate class.
  4. Rerun your tests to ensure nothing got broken.
  5. When you have time, goto 2. and repeat as required to make the code manageable.

The classes you build in step 3. iterations will likely grow to absorb more code that is appropriate to their newly-clear function.

I could also add:

0: buy Michael Feathers' book on working with legacy code

Unfortunately this type of work is all too common, but my experience is that there is great value in being able to make working but horrid code incrementally less horrid while keeping it working.

Exactly this problem is handled in one of the chapters of the book "Working Effectively with Legacy Code" (http://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052).

What are the ways to eliminate the use of switch in code?

See the Switch Statements Smell:

Typically, similar switch statements are scattered throughout a program. If you add or remove a clause in one switch, you often have to find and repair the others too.

Both Refactoring and Refactoring to Patterns have approaches to resolve this.

If your (pseudo) code looks like:

class RequestHandler {

    public void handleRequest(int action) {
        switch(action) {
            case LOGIN:
                doLogin();
                break;
            case LOGOUT:
                doLogout();
                break;
            case QUERY:
               doQuery();
               break;
        }
    }
}

This code violates the Open Closed Principle and is fragile to every new type of action code that comes along. To remedy this you could introduce a 'Command' object:

interface Command {
    public void execute();
}

class LoginCommand implements Command {
    public void execute() {
        // do what doLogin() used to do
    }
}

class RequestHandler {
    private Map<Integer, Command> commandMap; // injected in, or obtained from a factory
    public void handleRequest(int action) {
        Command command = commandMap.get(action);
        command.execute();
    }
}

If your (pseudo) code looks like:

class House {
    private int state;

    public void enter() {
        switch (state) {
            case INSIDE:
                throw new Exception("Cannot enter. Already inside");
            case OUTSIDE:
                 state = INSIDE;
                 ...
                 break;
         }
    }
    public void exit() {
        switch (state) {
            case INSIDE:
                state = OUTSIDE;
                ...
                break;
            case OUTSIDE:
                throw new Exception("Cannot leave. Already outside");
        }
    }

Then you could introduce a 'State' object.

// Throw exceptions unless the behavior is overriden by subclasses
abstract class HouseState {
    public HouseState enter() {
        throw new Exception("Cannot enter");
    }
    public HouseState leave() {
        throw new Exception("Cannot leave");
    }
}

class Inside extends HouseState {
    public HouseState leave() {
        return new Outside();
    }
}

class Outside extends HouseState {
    public HouseState enter() {
        return new Inside();
    }
}

class House {
    private HouseState state;
    public void enter() {
        this.state = this.state.enter();
    }
    public void leave() {
        this.state = this.state.leave();
    }
}

Hope this helps.

Alright it can be a lame question, but everybody uses these things differently. What's some of the best time savers out there for this IDE.

Tom

Alt-Shift-R stands for rename, not refactor. Refactoring is a more general term (as defined by the book).

Nevertheless, it is one of my favorite refactorings. Others include:

Extract Local Variable is especially useful when I don't remember (or bother to type) the result type of a method. Assuming you have a method JdbcTemplate createJdbcTemplate() in your class, write some code such as this:

void someQuery() {
    createJdbcTemplate()
}

Select the expression createJdbcTemplate(), click Alt-Shift-L, type the name of variable and press enter.

void someQuery() {
    JdbcTemplate myTemplate = createJdbcTemplate();
}

Are there any tools which support refactoring PHP code (renaming variables, extracting methods, finding method references, ...)? Thank you.

https://github.com/QafooLabs/php-refactoring-browser

PHP Refactoring Browser is a more recent take on the Refactoring Browser described in the good book. It could use some help (please contribute), but it has support for several common refactorings.

As of writing, the browser supports:

  • Extract Method
  • Rename Local Variable
  • Convert Local to Instance Variable
  • Rename Class and Namespaces
  • Optimize Use Statements (PHP-specific)

There is a SublimeText plugin which actually works. When I'm looking at new code I'll apply some of these simple refactorings to help me get a handle on the codebase.

I moved one years ago from classic OO languages such like Java to JavaScript. The following code is definitely not recommended (or even not correct) in Java:

if(dayNumber = getClickedDayNumber(dayInfo))
{
    alert("day number found : " + dayNumber);
}
function getClickedDayNumber(dayInfo)
{
    dayNumber = dayInfo.indexOf("fc-day");
    if(dayNumber != -1) //substring found
    {
        //normally any calendar month consists of "40" days, so this will definitely pick up its day number.
        return parseInt(dayInfo.substring(dayNumber+6, dayNumber+8));
    }
    else return false;
}

Basically I just found out that I can assign a variable to a value in an if condition statement, and immediately check the assigned value as if it is boolean.

For a safer bet, I usually separate that into two lines of code, assign first then check the variable, but now that I found this, I am just wondering whether is it good practice or not in the eyes of experienced JavaScript developers?

If you were to refer to Martin Fowlers book Refactoring improving the design of existing code ! Then there are several cases where it would be good practice eg. long complex conditionals to use a function or method call to assert your case:

"Motivation

One of the most common areas of complexity in a program lies in complex conditional logic. As you write code to test conditions and to do various things depending on various conditions, you quickly end up with a pretty long method. Length of a method is in itself a factor that makes it harder to read, but conditions increase the difficulty. The problem usually lies in the fact that the code, both in the condition checks and in the actions, tells you what happens but can easily obscure why it happens.

As with any large block of code, you can make your intention clearer by decomposing it and replacing chunks of code with a method call named after the intention of that block of code. > With conditions you can receive further benefit by doing this for the conditional part and each of the alternatives. This way you highlight the condition and make it clearly what you > are branching on. You also highlight the reason for the branching."

And yes his answer is also valid for Java implementations. It does not assign the conditional function to a variable though in the examples.

Anyone can read the GoF book to learn what design patterns are and how to use them, but what is the process for figuring out when a design pattern solves a problem? Does the knowledge of the pattern drive the design, or is there a way to figure out how a pattern can be used to change a design?

In other words, are there patterns for Patterns?

Design Patterns? You're soaking in them!

There's nothing special about design patterns, they are merely patterns of design. All development uses design patterns. There are a certain set of design patterns in object oriented programming which are considered generally desirable and have become the canonical Design Patterns. But there are also many undesirable or otherwise indifferent design patterns (such as design anti-patterns) as well as undiscovered and/or undocumented patterns.

You can't avoid using patterns when programming. But you can become more aware of the patterns you are using and of when certain patterns are useful and when they are not. Studying the canonical Design Patterns from the GoF book will help, as will learning about code smells and refactoring. There's no one right answer for when a particular design or design pattern should be used, you need to build up experience in using and implementing them in order to know when and where to use which pattern.

I've had a colleague that told me he once worked for a company that had as a policy to never have conditionals ("if" and "switch" statements) in the code and that they let all the decisions in the code be done using polymorphism and (I'm guessing) some other OO principles.

I sort of understand the reasoning behind this, of having code that is more DRY and easier to update, but I'm looking for a more in-depth explanation of this concept. Or maybe it's part of a more general design approach.

If anyone has any resources for this or would be willing to explain or even have some more terms related to this I can use to find more answers I'd be much obliged.

I found one question on SO that was kind of related but I'm unfamiliar with C++ so I don't understand too much of the answers there.

(I'm no OO guru btw but I can manage)

I'm most proficient in PHP, and after that Python so I'd prefer info that uses those languages.

Update: I'll ask my colleague for more info on what he meant exactly.

Update 2015: after some more years of experience in programming I see now that the aim of this policy was probably to prevent programmers from adding functionality in a haphazard way by just adding conditionals (if statements) in certain places. A better way to extend software is to use the "Open/Closed principle" where software is extended by using inheritance and polymorphism. I strongly doubt whether the policy was super strict on all conditionals as it's kinda hard to go completely without them.

There are some resources on the Anti-IF Campaign site, such as this article.

I believe it's a matter of degree. Conditionals aren't always bad, but they can be (and frequently are) abused.

Additional thoughts (one day later)

Refactoring: Improving the Design of Existing Code is a good reference on this subject (and many others). It covers Replace Conditional with Polymorphism. There's also a new one, Replace Conditional with Visitor, on the web site.

I value simplicity and single responsibility over removing all if statements. These three goals often coincide. Static analysis tools that support the cyclomatic complexity metric can quickly point out code with nested or serial conditionals. The if statements may remain post-refactoring, but could be broken out into smaller methods and/or multiple classes.

Update: Michael Feathers wrote an article on Unconditional Programming.

This is a popular topic: Phil Haack on Death to the IF statement!

What are good arguments to convince others to comment their code?

I notice many programmers favor the perceived speed of writing code without comments over leaving some documentation for themselves and others. When I try to convince them I get to hear half baked stuff like "the method/class name should say what it does" etc. What would you say to them to change their minds?

If you are against commenting, you please just leave comments. This should be a resource for people trying to convince people to comment the code, not otherwise. :-)

Other related questions are: Commenting code, Do you comment your code and How would you like your comments.

Comments should be thorough, written at the level of intent (why not how), and rare.

When writing code I tend to comment reasonably heavily as a matter of course. Then, I go back through and try to delete as many comments as possible, without decreasing the understandability of the code. >80% of the time this is as easy as extracting a well named method, this usually results in a comment which merely duplicates the information in the code itself. Beyond that, if there's a section of code that "needs" a comment I look for ways to simplify it or make it clearer.

Code should be self-documenting, and with the right techniques you can get 95% of the way there pretty easily. Generally I consider it to be a failure if there are any comments remaining on code that I check in.

I have been working on some 10 year old C code at my job this week, and after implementing a few changes, I went to the boss and asked if he needed anything else done. That's when he dropped the bomb. My next task was to go through the 7000 or so lines and understand more of the code, and to modularize the code somewhat. I asked him how he would like the source code modularized, and he said to start putting the old C code into C++ classes.

Being a good worker, I nodded my head yes, and went back to my desk, where I sit now, wondering how in the world to take this code, and "modularize" it. It's already in 20 source files, each with its own purpose and function. In addition, there are three "main" structs. each of these structures has 30 plus fields, many of them being other, smaller structs. It's a complete mess to try to understand, but almost every single function in the program is passed a pointer to one of the structs and uses the struct heavily.

Is there any clean way for me to shoehorn this into classes? I am resolved to do it if it can be done, I just have no idea how to begin.

First, tell your boss you're not continuing until you have:

http://www.amazon.com/Refactoring-Improving-Design-Existing-Code/dp/0201485672

and to a lesser extent:

http://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052

Secondly, there is no way of modularising code by shoe-horning it into C++ class. This is a huge task and you need to communicate the complexity of refactoring highly proceedural code to your boss.

It boils down to making a small change (extract method, move method to class etc...) and then testing - there is no short cuts with this.

I do feel your pain though...

In a recent code review I spotted a few lines of duplicated logic in a class (less than 15 lines). When I suggested that the author refactor the code, he argued that the code is simpler to understand that way. After reading the code again, I have to agree extracting the duplicated logic would hurt readability a little.

I know DRY is guideline, not an absolute rule. But in general, are you willing to hurt readability in the name of DRY?

Refactoring: Improving the Design of Existing Code

The Rule of Three

The first time you do something, you just do it. The second time you do
something similar, you wince at the duplication, but you do the duplicate
thing anyway. The third time you do something similar, you refactor.

Three strikes and you refactor.


Coders at Work

Seibel: So for each of these XII calls you're writing an implementation.
Did you ever find that you were accumulating lots of bits of very similar code?

Zawinski: Oh, yeah, definitely. Usually by the second or third time you've cut and pasted
that piece of code it's like, alright, time to stop cutting and pasting and put it in a subroutine.

I have a big mess of code. Admittedly, I wrote it myself - a year ago. It's not well commented but it's not very complicated either, so I can understand it -- just not well enough to know where to start as far as refactoring it.

I violated every rule that I have read about over the past year. There are classes with multiple responsibilities, there are indirect accesses (I forget the term - something like foo.bar.doSomething()), and like I said it is not well commented. On top of that, it's the beginnings of a game, so the graphics is coupled with the data, or the places where I tried to decouple graphics and data, I made the data public in order for the graphics to be able to access the data it needs...

It's a huge mess! Where do I start? How would you start on something like this?

My current approach is to take variables and switch them to private and then refactor the pieces that break, but that doesn't seem to be enough. Please suggest other strategies for wading through this mess and turning it into something clean so that I can continue where I left off!


Update two days later: I have been drawing out UML-like diagrams of my classes, and catching some of the "Low Hanging Fruit" along the way. I've even found some bits of code that were the beginnings of new features, but as I'm trying to slim everything down, I've been able to delete those bits and make the project feel cleaner. I'm probably going to refactor as much as possible before rigging my test cases (but only the things that are 100% certain not to impact the functionality, of course!), so that I won't have to refactor test cases as I change functionality. (do you think I'm doing it right or would it, in your opinion, be easier for me to suck it up and write the tests first?)

Please vote for the best answer so that I can mark it fairly! Feel free to add your own answer to the bunch as well, there's still room for you! I'll give it another day or so and then probably mark the highest-voted answer as accepted.

Thanks to everyone who has responded so far!


June 25, 2010: I discovered a blog post which directly answers this question from someone who seems to have a pretty good grasp of programming: (or maybe not, if you read his article :) )

To that end, I do four things when I need to refactor code:

  1. Determine what the purpose of the code was
  2. Draw UML and action diagrams of the classes involved
  3. Shop around for the right design patterns
  4. Determine clearer names for the current classes and methods

Pick yourself up a copy of Martin Fowler's Refactoring. It has some good advice on ways to break down your refactoring problem. About 75% of the book is little cookbook-style refactoring steps you can do. It also advocates automated unit tests that you can run after each step to prove your code still works.

As for a place to start, I would sit down and draw out a high-level architecture of your program. You don't have to get fancy with detailed UML models, but some basic UML is not a bad idea. You need a big picture idea of how the major pieces fit together so you can visually see where your decoupling is going to happen. Just a page or two of some basic block diagrams will help with the overwhelming feeling you have right now.

Without some sort of high level spec or design, you just risk getting lost again and ending up with another unmaintainable mess.

If you need to start from scratch, remember that you never truly start from scratch. You have some code and the knowledge you gained from your first time. But sometimes it does help to start with a blank project and pull things in as you go, rather than put out fires in a messy code base. Just remember not to completely throw out the old, use it for its good parts and pull them in as you go.

I'll second everyone's recommendations for Fowler's Refactoring, but in your specific case you may want to look at Michael Feathers' Working Effectively with Legacy Code, which is really perfect for your situation.

Feathers talks about Characterization Tests, which are unit tests not to assert known behaviour of the system but to explore and define the existing (unclear) behaviour -- in the case where you've written your own legacy code, and fixing it yourself, this may not be so important, but if your design is sloppy then it's quite possible there are parts of the code that work by 'magic' and their behaviour isn't clear, even to you -- in that case, characterization tests will help.

One great part of the book is the discussion about finding (or creating) seams in your codebase -- seams are natural 'fault lines', if you like, where you can break into the existing system to start testing it, and pulling it towards a better design. Hard to explain but well worth a read.

There's a brief paper where Feathers fleshes out some of the concepts from the book, but it really is well worth hunting down the whole thing. It's one of my favourites.

Which is more important: The design of the database? Or the design of the application code?

There is a lot of information out there about reusable code (from Carl Franklin at dnrtv.com, CSLA.net, et. al.), but I don't see too much information about Database Design and its impact on the life of an application (particularly how bad design decisions early on affect the application later in its 'life'.

The domain model should be independent of persistence implementation details (although the technology does place some constraints on the model) — http://www.infoq.com/articles/ddd-in-practice

You should focus on the domain model. With great ORM technology like Hibernate/NHibernate the database will only be an implementation detail.

Books you should read if you are doing .NET web development:

  1. ASP.NET MVC Framework Preview (only 100 pages, and will get you started)
  2. NHibernate in Action
  3. Domain Driven Design and Development In Practice (not read it, but I will, and it's free)
  4. Refactoring: Improving the Design of Existing Code

I know that refactoring is "changing the structure of a program so that the functionality is not changed". I was talking with some of the guys I'm working with on my final year project at university and I was surprised that they have a much more expansive (for want of a better word) view of refactoring. I consider refactoring to be things like extracting methods and renaming classes. They also suggested things like changing data structures (like a Java LinkedList to an ArrayList), changing algorithms (using merge sort instead of bubble sort), and even rewriting large chunks of code as refactoring. I was quite sure that they were wrong, but I wasn't able to give a good reason why because what they were suggesting did change the program (and presumably make it better) without changing its behaviour. Am I right, and more importantly, why?

Martin Fowler's "Refactoring: Improving the Design of Existing Code" is perhaps THE reference:

Refactoring is a controlled technique for improving the design of an existing code base. Its essence is applying a series of small behavior-preserving transformations, each of which "too small to be worth doing". However the cumulative effect of each of these transformations is quite significant. By doing them in small steps you reduce the risk of introducing errors. You also avoid having the system broken while you are carrying out the restructuring - which allows you to gradually refactor a system over an extended period of time.

Refactoring goes hand-in-hand with unit testing. Write tests before you refactor and then you have a confidence level in the refactoring (proportional to the coverage of the tests).

A good reference is: Information about Refactoring

One of the most unpleasant (and unfortuantely most frequent) situations I am confronted with in my all day life as a developer is that I have to fix bugs or add features into code that is badly designed. Now as a good craftsman I would like to leave the code in a better state than I found it. Often new features can not be implemented if I do not refactor the design. Well - they could, but that would make the code even worse.

Unfortunately this is exactly what I tend to have a hard time with. I feel that if there is one thing that is hard, it is to refactor bad code, especially when you have deadlines. Touching bad and complex code that more or less works is scary. As a result I introduce even more clutter when I hack a new feature into the code without modifiying existing code.

Now my question is How can I learn to cope with bad code? How can I learn to understand huge codebases and then refactor parts of it without breaking stuff that already worked and without exceeding the deadline? Is there any literature you can recommend? Do you have any general tips for me?

Michael Feathers wrote a good book about this subject exactly.

Working Effectively with Legacy Code.

Another great book is by Martin Fowler, Kent Beck and others:

Refactoring: Improving the Design of Existing Code.

I've heard that projects developed using TDD are easier to refactor because the practice yields a comprehensive set of unit tests, which will (hopefully) fail if any change has broken the code. All of the examples I've seen of this, however, deal with refactoring implementation - changing an algorithm with a more efficient one, for example.

I find that refactoring architecture is a lot more common in the early stages where the design is still being worked out. Interfaces change, new classes are added & deleted, even the behavior of a function could change slightly (I thought I needed it to do this, but it actually needs to do that), etc... But if each test case is tightly coupled to these unstable classes, wouldn't you have to be constantly rewriting your test cases each time you change a design?

Under what situations in TDD is it okay to alter and delete test cases? How can you be sure that altering the test cases don't break them? Plus it seems that having to synchronize a comprehensive test suite with constantly changing code would be a pain. I understand that the unit test suite could help tremendously during maintenance, once the software is built, stable, and functioning, but that's late in the game wheras TDD is supposed to help early on as well.

Lastly, would a good book on TDD and/or refactoring address these sort of issues? If so, which would you recommend?

I would recommended (as others have):

Ever since I started using .NET, I've just been creating Helper classes or Partial classes to keep code located and contained in their own little containers, etc.

What I'm looking to know is the best practices for making ones code as clean and polished as it possibly could be.

Obviously clean code is subjective, but I'm talking about when to use things (not how to use them) such as polymorphism, inheritance, interfaces, classes and how to design classes more appropriately (to make them more useful, not just say 'DatabaseHelper', as some considered this bad practice in the code smells wiki).

Are there any resources out there that could possibly help with this kind of decision making?

Bare in mind that I haven't even started a CS or software engineering course, and that a teaching resource is fairly limited in real-life.

Working Effectively with Legacy Code is one of the best books I have seen on this subject.

Don't be put off the title of the book - Rather than treating Refactoring as a formal concept (which has its place), this book has lots and lots of simple "why didn't I think of that" tips. Things like "go through a class and remove any methods not directly realted to that class and put them in a different one".

e.g. You have a grid and some code to persist the layout of that grid to file. You can probably safely move the layout persisting code out to a different class.

A real eye-opener to me was Refactoring: Improving the Design of Existing Code:

With proper training a skilled system designer can take a bad design and rework it into well-designed, robust code. In this book, Martin Fowler shows you where opportunities for refactoring typically can be found, and how to go about reworking a bad design into a good one.

Refactoring

It helped me to efficiently and systematically refactor code. Also it helped me a lot in discussions with other developers, when their holy code has to be changed ...

I'd recommend Domain Driven Design. I think both YAGNI and AlwaysRefactor principles are two simplistic. The age old question on the issue is do i refactor "if(someArgument == someValue)" into a function or leave it inline?

There is no yes or no answer. DDD advises to refactor it if the test represents a buisiness rule. The refactoring is not (only) about reuse but about making the intentions clear.

Check out Martin Fowler's comments and book on Refactoring

Is it better to write many small methods (or functions), or to simply write the logic/code of those small processes right into the place where you would have called the small method? What about breaking off code into a small function even if for the time being it is only called from one spot?

If one's choice depends on some criteria, what are they; how should a programmer make a good judgement call?

I'm hoping the answer can be applied generally across many languages, but if necessary, answers given can be specific to a language or languages. In particular, I'm thinking of SQL (functions, rules and stored procedures), Perl, PHP, Javascript and Ruby.

I always break long methods up into logical chunks and try to make smaller methods out of them. I don't normally turn a few lines into a separate method until I need it in two different places, but sometimes I do just to help readability, or if I want to test it in isolation.

Fowler's Refactoring is all about this topic, and I highly recommend it.

Here's a handy rule of thumb that I use from Refactoring. If a section of code has a comment that I could re-word into a method name, pull it out and make it a method.

After years of coding Delphi programs as untestable code in forms and datamodules, including global variables, and the only classes are the forms themselves, containing all the code I need for the form UI itself.

How would I convert the code to a set of classes that do the actual work? would I need to stop using the datasources/datasets and do everything in classes? do I need an ORM?

There's usually zero need for reuse of the code in the forms, so does it make sense to convert the logic to classes?

Another book I can highly, highly recommend - in my personal opinion even better suited than the "generic" refactoring book by Fowler - is "Working Effectively with Legacy Code" by Michael Feathers. It truly showcases the major bumps you will hit while doing that kind of work. Oh, and: Refactoring legacy code can be quite hard on your psyche. I hope you can handle frustration... I like this quote (don't remember where I got it from): "God was able to create the world in 6 days, just because there wasn't any legacy code". Good luck. ;)

To start with I can highly recommend reading the book Refactoring by Martin Fowler.

This will give you a real understanding about how best to sensibly approach introducing changes to the existing (non OO) code to improve maintainability.

I would not look at an ORM until you have a clear understanding about what benefits (if any) one would bring to your application.

I've been programming c++ for four years and java for two, I feel that I have a strong grasp of the languages them selves, although I'm no wizard in either. It seems that once I hit the 75% completion mark my code seems to be a god awful ball of spaghetti mess, sometimes to the point of unreadability. What have you done to produce good readable code?

The algo I followed is pretty simple:

  1. Study the code you consider to be good
  2. Decide what aspects of the code you study are better then in your code
  3. Improve your code untill you are happy with the way it reads

Later I've read THE book Refactoring: Improving the Design of Existing Code (Amason link) . It contains concrete advices of do-s and dont-s for clean and pretty code with Java & Python examples.

I've seen a trend to move business logic out of the data access layer (stored procedures, LINQ, etc.) and into a business logic component layer (like C# objects).

Is this considered the "right" way to do things these days? If so, does this mean that some database developer positions may be eliminated in favor of more middle-tier coding positions? (i.e. more c# code rather than more long stored procedures.)

If the applications is small with a short lifetime, then it's not worth putting time into abstracting the concerns in layers. In larger, long lived applications your logic/business rules should not be coupled to the data access. It creates a maintenance nightmare as the application grows.

Moving concerns to a common layer or also known as Separation of concerns, has been around for a while:

Wikipedia

The term separation of concerns was probably coined by Edsger W. Dijkstra in his 1974 paper "On the role of scientific thought"1.

For Application Architecture a great book to start with is Domain Driven Design. Eric Evans breaks down the different layers of the application in detail. He also discusses the database impedance and what he calls a "Bounded Context"

Bounded Context

A blog is a system that displays posts from newest to oldest so that people can comment on. Some would view this as one system, or one "Bounded Context." If you subscribe to DDD, one would say there are two systems or two "Bounded Contexts" in a blog: A commenting system and a publication system. DDD argues that each system is independent (of course there will be interaction between the two) and should be modeled as such. DDD gives concrete guidance on how to separate the concerns into the appropriate layers.

Other resources that might interest you:

Until I had a chance to experience The Big Ball of Mud or Spaghetti Code I had a hard time understanding why Application Architecture was so important...

The right way to do things will always to be dependent on the size, availability requirements and lifespan of your application. To use stored procs or not to use stored procs... Tools such as nHibrnate and Linq to SQL are great for small to mid-size projects. To make myself clear, I've never used nHibranate or Linq To Sql on a large application, but my gut feeling is an application will reach a size where optimizations will need to be done on the database server via views, Stored Procedures.. etc to keep the application performant. To do this work Developers with both Development and Database skills will be needed.

According to anti-if campaign it is a best practice not to use ifs in our code. Can anyone tell me if it possible to get rid of the if in this piece of code ? (switch is also not an option, The point is to remove the conditional logic, not replace ifs with similar language constructs)

if(s == "foo")
{
    Writeln("some logic here");
}
else if(s == "bar")
{
    Writeln("something else here");
}
else if(s == "raboof")
{
    Writeln("of course I need more than just Writeln");
}

(language: Java or C#)

The example you have given I would not change (though I guess you realise it wouldn't need changing)- I'm guessing you are using it as a representational example.

In Fowler's Refactoring book, he discusses the Replace Conditional with Polymorphism. That's what I see as a good use to replace if/switch statements (where appropriate).

I have been coding for a while now and am confortable with a few programming languages but now feel that I need to improve my problem solving or program design technique.

What skills do I need have to be able to design a program as a solution to a problem?

More: The question is about how to design a solution rather than how to code better.

For problem solving technique, look at how other people have solved problems. I recommend The Algorithm Design Manual and Algorithms in a Nutshell. Both of these books take you from a problem statement through several iterations of solutions to show you the thought process, rather than going directly to the final solution.

Talking about books, I suggest this great one, by Martin Fowler

Refactoring: Improving the Design of Existing Code

Is it better(what is the best practice) to create methods with a long list of parameters or wrap the parameters into an object?

I mean lets say i have a Client data type with a long list of properties and i want to update all the properties at once. is it better to do something like

public int Update(int id, string name, string surname, string streetAddress, string streetAddress2, string postcode, string town, string city, string nationality, string age, string gender,string job)
{  }

or wrap all the properties in a object and do something like

public int Update(Client client)
{  }

thanks

In his book Refactoring, Martin Fowler explicitly calls out long parameter lists as a code smell and suggest refactoring such methods to use a Parameter Object.

A variation is to identify how those many parameters group themselves, and create more than one Parameter Object that represent each such group.

The advantage of a Parameter Object is that the code becomes more readable when you can give the Parameter Object a communicative name. It may turn out that the Parameter Object represents a real Domain Concept, and the next thing you can do is to start moving behavior into it.

I have a dejavu feeling when reading [What to do about a 11000 lines C++ source file?] post, but I don't think I can start taking action on my own since I do not have the authority to take action. So I think the first step is to convince people in the organization that big lump of code is bad.

I have a similar situation where there is a single class with 11975 lines of code and every time there are new features, there is a high probability that this class will get bigger and bigger.

If this class consists of many smaller methods AND these methods actually belong to the class i.e. they are cohesive to each other and the class, then this is not necessarily bad.

The mere fact that a class is large does not make it automatically bad.

What is more important is avoiding a so called "god class" that contains many methods that have no conceptual relation to each other or the class itself.

Keeping the size of the individual methods down is also worth focusing on because most developers will need to "load" a whole method into their brains at one time, not the entire class. So the more concise the method the easier it is for developers to maintain.

You should gather metrics about the number of support issues that can be traced back to code in this class. Also, how long to developers spend making changes to this class compared to other smaller classes? Once you have these metrics you are in a better position to explain to management why such a large class is bad.

In the situation of a large class with a small number of large methods I would use refactoring techniques described in Martin Fowlers book, along with unit tests to ensure no new bugs are introduced.

Once you've refactored a huge method down to a number of simpler understandable methods and have successful unit tests, simply demonstrate this to your colleagues.

At this point, if your colleagues disagree or cannot see a general improvement in the understandability and maintainability of the refactored code, you are dealing with a political/personality problem. In that case it's up to you if you want to continue working in such an environment.

I recently read an interesting comment on an OOP related question in which one user objected to creating a "Manager" class:

Please remove the word manager from your vocabulary when talking about class names. The name of the class should be descriptive of its' purpose. Manager is just another word for dumping ground. Any functionality will fit there. The word has been the cause of many extremely bad designs

This comment embodies my struggle to become a good object-oriented developer. I have been doing procedural code for a long time at an organization with only procedural coders. It seems like the main strategy behind the relatively little OO code we produce is to break the problem down into classes that are easily identifiable as discrete units and then put the left over/generalized bits in a "Manager" class.

How can I break my procedural habits (like the Manager class)? Most OO articles/books, etc. use examples of problems that are inherently easy to transform into object groups (e.g., Vehicle -> Car) and thus do not provide much guidance for breaking down more complex systems.

Becoming good at OO takes years of practice and study of good OO code, ideally with a mentor. Remember that OO is just one means to an end. That being said, here are some general guidelines that work for me:

  • Favor composition over inheritance. Read and re-read the first chapter of the GoF book.
  • Obey the Law of Demeter ("tell, don't ask")
  • Try to use inheritance only to achieve polymorphism. When you extend one class from another, do so with the idea that you'll be invoking the behavior of that class through a reference to the base class. ALL the public methods of the base class should make sense for the subclass.
  • Don't get hung up on modeling. Build a working prototype to inform your design.
  • Embrace refactoring. Read the first few chapters of Fowler's book.

Reading and then practicing OO principles is what works for me. Head First Object-Oriented Analysis & Design works you through examples to make a solution that is OO and then ways to make the solution better.

My eureka moment for understanding object-oriented design was when I read Eric Evans' book "Domain-Driven Design: Tackling Complexity in the Heart of Software". Or the "Domain Driven Design Quickly" mini-book (which is available online as a free PDF) if you are cheap or impatient. :)

Any time you have a "Manager" class or any static singleton instances, you are probably building a procedural design.

I was wondering how other developers begin refactoring. What is your first step? How this process (refactoring) differ if you refactor code which is not yours? Do you write tests while refactoring?

Read Martin Fowler's book "Refactoring"

BTW - that's Martin Fowler's own Amazon exec link, if you're wondering :)

I apologize for the subjectiveness of this question, but I am a little stuck and I would appreciate some guidance and advice from anyone who's had to deal with this issue before:

I have (what's become) a very large RESTful API project written in C# 2.0 and some of my classes have become monstrous. My main API class is an example of this -- with several dozen members and methods (probably approaching hundreds). As you can imagine, it's becoming a small nightmare, not only to maintain this code but even just navigating the code has become a chore.

I am reasonably new to the SOLID principles, and I am massive fan of design patterns (but I am still at that stage where I can implement them, but not quite enough to know when to use them - in situations where its not so obvious).

I need to break my classes down in size, but I am at a loss of how best to go about doing it. Can my fellow StackOverflow'ers please suggest ways that they have taken existing code monoliths and cut them down to size?

Single Responsibility Principle - A class should have only one reason to change. If you have a monolithic class, then it probably has more than one reason to change. Simply define your one reason to change, and be as granular as reasonable. I would suggest to start "large". Refactor one third of the code out into another class. Once you have that, then start over with your new class. Going straight from one class to 20 is too daunting.

Open/Closed Principle - A class should be open for extension, but closed for change. Where reasonable, mark your members and methods as virtual or abstract. Each item should be relatively small in nature, and give you some base functionality or definition of behavior. However, if you need to change the functionality later, you will be able to add code, rather than change code to introduce new/different functionality.

Liskov Substitution Principle - A class should be substitutable for its base class. The key here, in my opinion, is do to inheritance correctly. If you have a huge case statement, or two pages of if statements that check the derived type of the object, then your violating this principle and need to rethink your approach.

Interface Segregation Principle - In my mind, this principle closely resembles the Single Responsibility principle. It just applies specifically to a high level (or mature) class/interface. One way to use this principle in a large class is to make your class implement an empty interface. Next, change all of the types that use your class to be the type of the interface. This will break your code. However, it will point out exactly how you are consuming your class. If you have three instances that each use their own subset of methods and properties, then you now know that you need three different interfaces. Each interface represents a collective set of functionality, and one reason to change.

Dependency Inversion Principle - The parent / child allegory made me understand this. Think of a parent class. It defines behavior, but isn't concerned with the dirty details. It's dependable. A child class, however, is all about the details, and can't be depended upon because it changes often. You always want to depend upon the parent, responsible classes, and never the other way around. If you have a parent class depending upon a child class, you'll get unexpected behavior when you change something. In my mind, this is the same mindset of SOA. A service contract defines inputs, outputs, and behavior, with no details.

Of course, my opinions and understandings may be incomplete or wrong. I would suggest learning from people who have mastered these principles, like Uncle Bob. A good starting point for me was his book, Agile Principles, Patterns, and Practices in C#. Another good resource was Uncle Bob on Hanselminutes.

Of course, as Joel and Jeff pointed out, these are principles, not rules. They are to be tools to help guide you, not the law of the land.

EDIT:

I just found these SOLID screencasts which look really interesting. Each one is approximately 10-15 minutes long.

There's a classic book by Martin Fowler - Refactoring: Improving the Design of Existing Code.

There he provides a set of design techniques and example of decisions to make your existing codebase more manageable and maintainable (and that what SOLID principals are all about). Even though there are some standard routines in refactoring it is a very custom process and one solution couldn't be applied to all project.

Unit testing is one of the corner pillars for this process to succeed. You do need to cover your existing codebase with enough code coverage so that you'd be sure you don't break stuff while changing it. Actually using modern unit testing framework with mocking support will lead encourage you to better design.

There are tools like ReSharper (my favorite) and CodeRush to assist with tedious code changes. But those are usually trivial mechanical stuff, making design decisions is much more complex process and there's no so much tool support. Using class diagrams and UML helps. That what I would start from, actually. Try to make sense of what is already there and bring some structure to it. Then from there you can make decisions about decomposition and relations between different components and change your code accordingly.

Hope this helps and happy refactoring!

I am going to be giving developers at my company a crash course on design patterns (after coming across some scary code recently).

One of the most important things I want to put across is that they save time in both the long and short term (which they really do!) - as developers here are put under quite a bit of time strain. All in all I need to demonstrate the every day benefits - things that will let them go home early.

Telling them that it might mean less bugs probably isn't going to hit home. I need stuff that is going to sink in.

I will probably do three to four sessions of an hour each. Do you guys have any suggestions on what to touch on/to do?

Head First Design Patterns would be a great place to start. It covers the mainstream design patterns.

Refactoring to Patterns might also be of interest.

If you can't buy a book for every developer, buy a few and spread them around.

You are in a unique position for a course-giver: you know the developers and you know the code that they are working with.

A possible approach would be to look at some of the scary code, and go along the lines of "How could we go about improving this? As it happens, there's a design pattern called Observer...."

This might end up being a mix of design patterns and refactoring. But that might be appropriate, given that you're trying to reach developers working on an existing code base.

Good opening slides for any education course in my opinion are:
1) Why are we here? (Where has the need for this course been identified?)
2) What do I expect to learn?
3) Who should take this course? (What are the intended students, prerequisites, etc?)
4) When can I apply what I’ve learned?
5) Expectations of you (Participation, homework, tests, minimum classes to attend, etc)

For design patterns I could expect several visual tools or "job aids".

I would follow a structure similar to the Elements of Reusable Object-Oriented Software book:

1) UML – Class Diagram Overview
2) OOP – Abstraction, Encapsulation, Polymorphism, Inheritance
3) Cohesion and Coupling
4) What is a design Pattern? – Pattern Name, The Problem, The solution, The consequences
5) Why are design patterns so hard to learn?
6) Why use design Patterns?
7) How to Select a Design Pattern
8) How to Use a Design Pattern
9) Cover various GoF design patterns with examples – Show examples of code before applying a design pattern, and how it looks after like Vince Huston does in his examples.
10) Conclusion

As already mentioned, design patterns are really ideas, so when teaching you must convey the idea. If they understand the problem, solution and consequences of the design pattern, then they will be far better off than trying to force patterns into the code (and that will become a nightmare). Recognition of where and what patterns (if any) can be applied is the real goal. The Huston examples are really good for putting out an example of code to the class and seeing if they can identify a pattern to improve it. Hope this helps.

I'm just reviewing some of my old code (have some spare time), and I noticed a rather lengthy switch statement. Due to gaining new knowledge, I have since refactored it in the following form:

private Dictionary<string, Action> createView
    {
        get
        {
            return new Dictionary<string, Action>()
            {
                {"Standard", CreateStudySummaryView},
                {"By Group", CreateStudySummaryByGroupView},
                {"By Group/Time", CreateViewGroupByHour}
            };
        }
    }

Would you consider this good practise, or is this simply a case of being superflous and unneccessary? I'm keen to ensure new techniques that I learn, are not being clever just for the sake of it, and that they actually add benefit to the code.

Thanks.

Long switch statements are a classic bad smell, and are always a target for refactoring.

The "standard" step to perform here is Replace Conditional with Polymorphism. This was one of the steps listed in Martin Fowler's book Refactoring (published 11 years ago in 1999).

Now that it's so easy to treat functions like objects (eg with Action) this might be just as good a solution.

And no, I don't think you're being clever for the sake of it. If I wanted to add another option in the future, I can easily see what needs to be done.

[ This is a result of Best Practice: Should functions return null or an empty object? but I'm trying to be very general. ]

In a lot of legacy (um...production) C++ code that I've seen, there is a tendency to write a lot of NULL (or similar) checks to test pointers. Many of these get added near the end of a release cycle when adding a NULL-check provides a quick fix to a crash caused by the pointer dereference--and there isn't a lot of time to investigate.

To combat this, I started to write code that took a (const) reference parameter instead of the (much) more common technique of passing a pointer. No pointer, no desire to check for NULL (ignoring the corner case of actually having a null reference).

In C#, the same C++ "problem" is present: the desire to check every unknown reference against null (ArgumentNullException) and to quickly fix NullReferenceExceptions by adding a null check.

It seems to me, one way to prevent this is to avoid null objects in the first place by using empty objects (String.Empty, EventArgs.Empty) instead. Another would be to throw an exception rather than return null.

I'm just starting to learn F#, but it appears there are far fewer null objects in that enviroment. So maybe you don't really have to have a lot of null references floating around?

Am I barking up the wrong tree here?

I tend to be dubious of code with lots of NULLs, and try to refactor them away where possible with exceptions, empty collections, and so on.

The "Introduce Null Object" pattern in Martin Fowler's Refactoring (page 260) may also be helpful. A Null Object responds to all the methods a real object would, but in a way that "does the right thing". So rather than always check an Order to see if order.getDiscountPolicy() is NULL, make sure the Order has a NullDiscountPolicy in these cases. This streamlines the control logic.

I didn't find any question that would be this general.

Please post whatever you find to be a useful rule for oject-oriented design.

The best of OOP nothing, any approach that's fit for your project is best. But it is important what are OOP practices before choosing any/many from them for your project.

1: APIE: Abstraction, Polymorphism, Inheritance, Encapsulation.

2: SOLID Principle.

3: OO analysis and design.

4: Design patterns.

5: Code refactoring.

6: Effective Java.

I have recommended the Head First Design Patterns book many times.

It gives you a good intro to the GoF Design Patterns (a more advanced book that you also should read), but also a good intro to sound OOP design principles.

enter image description here

In The Guerilla Guide to Interviewing Joel says that guys who want to get things done, but are not smart will do stupid things like using a visitor design pattern where a simple array would be sufficient.

I find it hard to detect, if the design pattern suggested by the Gang of Four should be applied.

Therefore, I would like some examples from Your work experience

  • When is a simple approach (fixed size array) sufficient?
  • What is the minimum size of a piece of software that justifies the use of the GoF patterns?
  • When to refactor from simple-minded to GoF? Can this be done in a sensible way?

I often find that using test driven development helps guide me when faced with these questions.

  • When is a simple approach sufficient? It is always sufficient to use the simplest approach to get the next test to pass. But knowing when/how to refactor is the real art form.
  • What is the minimum size of a piece of software that justifies the use of the GoF patterns? A rule of thumb I once read is that when you code something once, fine, when you duplicate that code somewhere a second time, make a note and move on. When you find a need for the same code a third time, it's time to refactor to remove duplication and simplify, and often that involves moving to a design pattern.
  • When to refactor from simple-minded to GoF? I like what @anopres said - it's time when you feel the pain of not having the design pattern in place. The pain (or code "smell") may manifest itself in several ways. Code duplication is the most obvious. Refactoring books like Fowler's Refactoring or Kerievsky's Refactoring to Patterns list many such pain points/code stenches.
  • Can this [refactoring] be done in a sensible way? The trick to refactoring is to have a suite of unit tests in place which you have confidence in, and then to refactor without causing any of those tests to fail. Refactoring, by definition, does not change the functionality of your code. Therefore, if your tests continue to pass, you can have a pretty good feeling that you didn't break anything. Although it can be difficult, I actually enjoy this part of TDD, it's almost like a game to make changes without breaking any tests.

In summary, I would say that TDD helps guide me to write the code that is sufficient at the time, and perhaps more importantly helps me to make the changes later when inevitably requirements change, more functionality is required, etc.

I've been studying OOP for quite a while now and I have a good grasp of the theory. I read the Head First book on OOP and, while it reinforced a lot of the theory, I found the case studies to be somewhat trivial.

I find that I'm applying OOP principles to my code each day, but I'm not sure if I'm applying them correctly. I need to get to the point where I am able to look at my code and know whether I'm using inheritance appropriately, whether my object is cohesive enough, etc.

Does anyone have any good recommendations (books, online guides, blogs, walk-throughs, etc.) for taking the next step in developing solid OOP skills?

I am working primarily in .NET (visual basic), but I welcome suggestions that incorporate various platforms.

Consider looking into Design Patterns. Although it seems like they aren't commonly used in enterprise applications (I've seen them more commonly used in API's and Frameworks than embedded into enterprise code), they could be applied to make software simpler or more robust in a lot of situations if only developers knew how to apply them.

The key is to understand the design patterns first, then with experience you'll learn how to apply them.

There is a Head First book on design patterns that teaches the concept pretty simply, although if you want a book that really covers design patterns in detail, check out the Gang of Four design patterns book, which is basically what made design patterns mainstream and is referred to almost every time the topic is brought up.

Design patterns can be applied in pretty much any object-oriented language to some degree or another, although some patterns can be overkill or over engineering in some cases.

EDIT:
I also want to add, you should check out the book Code Complete 2. It's a very influential book in the world of software development. It covers a lot of different concepts and theories. I learn something new every time I read it. It's such a good book that if I read it every 6 months to a year, I look at it from a different perspective that makes me a better programmer just by re-reading it. No matter how much you might think you know, this book will make you realize just how little you really know. It's really a great book. I can't stress how much you should own this book.

Read Refactoring by Martin Fowler, and apply it to your own work.

It will take you through a litany of malodorous characteristics of software code that describe how to detect improperly constructed classes, and even more importantly, how to fix them.

I am currently half-way through the following book:

http://www.amazon.com/Applying-UML-Patterns-Introduction-Object-Oriented/dp/0131489062

I cannot recommend this book strongly enough in terms of learning a real-life, professional-grade, practical approach to drafting and applying a well-formed and iterative design strategy before diving into code.

I, too, read the "Head First" book and felt that I was much better off for having read it.

After having a few years of working-world experience, I now view the Craig Larman book that I am recommending to be a perfect "next step" for me.

About the Presence of "UML" in this Book Title:

Whether you have positive feelings or negative feelings about UML notation, please do not let that influence your decision to buy the book (ISBN 0131489062) in either direction.

The prominence of "UML" in the title is misleading. While the author does use and explain UML notation, these explanations are extremely well-woven into relevant design discussions, and at no time does this book read like a boring UML spec.

In fact, here is a quote taken directly from the book:

What's important is knowing how to think and design in objects, which is a very different and much more valuable skill than knowing UML notation. While drawing a diagram, we need to answer key questions: What are the responsibilities of the object? Who does it collaborate with? What design patterns should be applied? Far more important than knowing the difference between UML 1.4 and 2.0 !

This book at times seems like it is "speaking to" a lead architect or a project manager. What I mean to say by that is that it assumes that the reader has significant control over the planning and direction of a software project.

Nonetheless, even if you are only responsible for some very small piece of your company's projects and products, I would still recommend this book and encourage you to apply some "scaled down" modifications of the book's advice to your piece of the project.

Tortoise HG is extrodanarily well designed piece of OO open source software (written in Python).

If you already understand the basics, building something from scratch in a fully object oriented language will be a good step in fully understanding OOP software architecture. If you don't know Python, Python Essential Reference will take you through the language in full in a few days to a week.

After you understand the language take a look through the software above and you'll have all sorts of epiphanies.

I often heard from professionals blog something like refactoring your code whenever the chance you get. What is it exactly? Rewriting your code in simpler and fewer lines? What is the purpose of doing this?

Martin Fowler has probably done the most to popularize refactoring, but I think good developers have always done these sorts of restructurings. Check out Fowler'srefactoring web site, and his 1999 Refactoring, which is an excellent introduction and catalog of specific refactorings using Java.

And I see he's a co-author of the brand new Refactoring, Ruby Edition, which should be a great resource.

I find that regularly cleaning up your code like this makes it a lot clearer and more maintainable.

To take one example, I wrote a small (Java 1.6) client library for accessing remote web services (using the REST architectural style). The bulk of this library is in one source file, and about half of that deals with the web services, while the other half is a simple in-memory cache of the responses (for performance). Over time both halves have grown in functionality, to the point where the source file was getting too complex. So today I used Fowler's "Extract Class" refactoring to move the cache logic into a new class. Before that I had to do some "Extract Methods" to isolate the caching logic. Along the way I did a few "Rename Methods" and an "Introduce Explaining Variable".

As other folks have noted, it's very important to have a good set of unit tests to apply after you make each change. They help ensure that you're not introducing new bugs, among other good things.

I’ve almost 6 years of experience in application development using .net technologies. Over the years I have improved as a better OO programmer but when I see code written by other guys (especially the likes of Jeffrey Richter, Peter Golde, Ayende Rahien, Jeremy Miller etc), I feel there is a generation gap between mine and their designs. I usually design my classes on the fly with some help from tools like ReSharper for refactoring and code organization.

So, my question is “what does it takes to be a better OO programmer”. Is it

a) Experience

b) Books (reference please)

c) Process (tdd or uml)

d) patterns

e) anything else?

And how should one validate that the design is good, easy to understand and maintainable. As there are so many buzzwords in industry like dependency injection, IoC, MVC, MVP, etc where should one concentrate more in design. I feel abstraction is the key. What else?

To have your design reviewed by someone is quite important. To review and maintain legacy code helps you to realize what makes the software rotten. Thinking is also very important; One one hand don't rush into implementing the first idea. On the other hand, don't think everything at once. Do it iteratively.

Regular reading of books/articles, like Eric Evan's Model Driven Design, or learning new languages (Smalltalk, Self, Scala) that take different approach to OO, helps you to really understand.

Software, and OO, is all about abstractions, responsibilities, dependencies and duplication (or lack of it). Keep them on your mind on your journey, and your learning will be steady.

It takes being a better programmer to be a better OO programmer.

OO has been evolving over the years, and it has a lot to do with changing paradigms and technologies like n-tier architecture, garbage collection, Web Services, etc.. the kind of things you've already seen. There are fundamental principles such as maintainability, reusability, low coupling, KISS, DRY, Amdahl's law, etc. you have to learn, read, experience, and apply it yourself.

OO is not an end on its own, but rather a means to achieve programming solutions. Like games, sports, and arts, practices cannot be understood without principles; and principles cannot be understood without practices.

To be more specific, here are some of the skills that may make one a better programmer. Listen to the domain experts. Know how to write tests. Know how to design a GUI desktop software. Know how to persist data into database. Separate UI layer and logic layer. Know how to write a class that acts like a built-in class. Know how to write a graphical component that acts like a built-in component. Know how to design a client/server software. Know networking, security, concurrency, and reliability.

Design patterns, MVC, UML, Refactoring, TDD, etc. address many of the issues, often extending OO in creative ways. For example, to decouple UI layer dependencies from logic layer, an interface may be introduced to wrap the UI class. From pure object-oriented point of view, it may not make much sense, but it makes sense from the point of view of separation of UI layer and logic layer.

Finally, realizing the limitations of OO is important too. In modern application architecture, the purist data + logic view of OO doesn't always mesh very well. Data transfer object (Java, MS, Fowler) for example intentionally strips away logic part of the object to make it carry only the data. This way the object can turn itself into a binary data stream or XML/JSON. The logic part may be handled both at client and server side in some way.

Something that's worked for me is Reading. I just had a Bulb moment with this book... David West's Object Thinking which elaborates Alan Kay's comment of 'The object revolution has yet to happen'. OO is different things to different people.. couple that with with the fact that your tools influence how you go about solving a problem. So learn multiple languages.

Object Thinking David West

Personally I think understanding philosophy, principles and values behind a practice rather than mimic-ing a practice helps a lot.

A number of years back, my webhost changed from 32-bit to 64-bit, and a critical PHP script stopped working. It was a due to the << and >> (bit shift) operations having changed. I was able to fix my problem by replacing the rotateleft and rotateright routines with rotateleft32 and rotateright32 like this:

function rotateleft($value, $numleft) {
  return (($value << $numleft) | ($value >> (32-$numleft)));
}
function rotateleft32($value, $numleft) {
  return ((($value << $numleft) | ($value >> (32-$numleft))) & 0xFFFFFFFF);
}

function rotateright($value, $numright) {
  return (($value >> $numright) | ($value << (32-$numright)));
}
function rotateright32($value, $numright) {
  return ((($value >> $numright) | ($value << (32-$numright))) & 0xFFFFFFFF);
}

I have now come across a new set of code that seems to be exactly the same issue, but it is more complicated:

function ECC_RotateLeft($a)
{
    $copya = makecopy($a);
    $bit = ($copya->e[0] & ECC_UPRBIT) ? 1 : 0;

    /* looped
    for ($i = 0; $i < ECC_MAXLONG - 1; $i++)
    $copya->e[$i] = ($copya->e[$i] << 1) | (($copya->e[$i + 1] & ECC_MSB) ? 1 : 0);
    $copya->e[0] &= ECC_UPRMASK;
    looped */

    /* unlooped */
    // These lines are optimized for ECC_MAXLONG==4 only!
    $bit = ($copya->e[0] & ECC_UPRBIT) ? 1 : 0;
    $copya->e[0] = (($copya->e[0] << 1) & ECC_UPRMASK) | (($copya->e[1] & ECC_MSB) ? 1 : 0);
    $copya->e[1] = ($copya->e[1] << 1) | (($copya->e[2] & ECC_MSB) ? 1 : 0);
    $copya->e[2] = ($copya->e[2] << 1) | (($copya->e[3] & ECC_MSB) ? 1 : 0);
    /* unlooped */

    $copya->e[3] = ($copya->e[3] << 1) | $bit;
    return $copya;
}

function ECC_RotateRight($a)
{
    $copya = makecopy($a);
    $bit = ($copya->e[ECC_NUMWORD] & 1) ? ECC_UPRBIT : 0;

    /* looped
    for ($i = ECC_MAXLONG - 1; $i > 0; $i--)
    $copya->e[$i] = (($copya->e[$i] >> 1) & 0x7FFFFFFF) | (($copya->e[$i - 1] & 1) ? ECC_MSB : 0);
    looped */

    /* unlooped */
    // Thes lines are optimized for ECC_MAXLONG==4 only!
    $copya->e[3] = (($copya->e[3] >> 1) & 0x7FFFFFFF) | (($copya->e[2] & 1) ? ECC_MSB : 0);
    $copya->e[2] = (($copya->e[2] >> 1) & 0x7FFFFFFF) | (($copya->e[1] & 1) ? ECC_MSB : 0);
    $copya->e[1] = (($copya->e[1] >> 1) & 0x7FFFFFFF) | (($copya->e[0] & 1) ? ECC_MSB : 0);
    /* unlooped */

    $copya->e[0] = (($copya->e[0] >> 1) & 0x7FFFFFFF) | $bit;
    return $copya;
}

I have three problems in trying to fix this myself:

  1. It is not my code, so I am not familiar with what it's trying to do.
  2. I no longer have a 32-bit server to test it against
  3. I am adequate, but not an expert in PHP.

I'd like to know if anyone does see a simple fix to allow this code to work on a 64-bit server and give the same result as it would have on a 32-bit server.

If not, how would you recommend I debug this given that I don't have a 32-bit result to compare against?


Here is some discussion regarding this problem, and an attempt to get the developer to fix it: How to get the outdated 32bit keymaker.php Script Working on 64 bit

Answering all four of your questions:

 1. It is not my code, so I am not familiar with what it is trying to do.

While I could go into tracing and debugging procedures in detail, I will instead recommend the classics. I highly recommend picking this up if this is your day job or you have more than a passing interest in refactoring code in the future.

 2. I no longer have a 32-bit server to test it against

As mentioned by Oli in the errata, you'll want to set up a 32-bit VM or chroot, dependent upon the OS your server is running.

 3. I am adequate, but not an expert in PHP.

As above, if this is more than just a spot issue, I recommend the classics.

 4. (Fixing the actual code)

First off, eww. No documentation, comment cruft, duplicate logic, and unexpressive variable names that fail to encapsulate their logic effectively. It's not the worst code I've seen, but I empathize with you here.

Still, the result isn't necessarily wrong. If you don't have a series of unit tests for it in your code base, I recommend adding them.

If you wish to benchmark the efficiency of this function, I highly recommend comparing it to the results of the algorithms defined in the notes here. Ideally, you'll want one of the implementations closest to this reference implementation.

Stealing one off the top of that thread and repurposing it to your API:

function ECC_RotateLeft($value,$amount) {
    if ($amount>0) {
        $amount %= 32;
        $value = ($value<<$amount) | ($value>>(32-$amount));
    }
    return $value;
}

function ECC_RotateRight($value,$amount) {
    if ($amount>0) {
        $amount %= 32;
        $value = ($value>>$amount) | ($value<<(32-$amount));
    }
    return $value;
}

(No surprise that this looks similar to the implementation you initially provided.)

Why am I including $amount as part of the specification? Simple: it doesn't violate encapsulation like the code you're refactoring. It looks like this can be set to ($copya->e[0] & ECC_UPRBIT) ? 1 : 0 as needed.

In short: the easiest way to refactor code isn't necessarily to look at its contained logic. Sometimes, determining intent and finding a good reference implementation is all that's needed.

Having just read the first four chapters of Refactoring: Improving the Design of Existing Code, I embarked on my first refactoring and almost immediately came to a roadblock. It stems from the requirement that before you begin refactoring, you should put unit tests around the legacy code. That allows you to be sure your refactoring didn't change what the original code did (only how it did it).

So my first question is this: how do I unit-test a method in legacy code? How can I put a unit test around a 500 line (if I'm lucky) method that doesn't do just one task? It seems to me that I would have to refactor my legacy code just to make it unit-testable.

Does anyone have any experience refactoring using unit tests? And, if so, do you have any practical examples you can share with me?

My second question is somewhat hard to explain. Here's an example: I want to refactor a legacy method that populates an object from a database record. Wouldn't I have to write a unit test that compares an object retrieved using the old method, with an object retrieved using my refactored method? Otherwise, how would I know that my refactored method produces the same results as the old method? If that is true, then how long do I leave the old deprecated method in the source code? Do I just whack it after I test a few different records? Or, do I need to keep it around for a while in case I encounter a bug in my refactored code?

Lastly, since a couple people have asked...the legacy code was originally written in VB6 and then ported to VB.NET with minimal architecture changes.

Good example of theory meeting reality. Unit tests are meant to test a single operation and many pattern purists insist on Single Responsibilty, so we have lovely clean code and tests to go with it. However, in the real (messy) world, code (especially legacy code) does lots of things and has no tests. What this needs is dose of refactoring to clean the mess.

My approach is to build tests, using the Unit Test tools, that test lots of things in a single test. In one test, I may be checking the DB connection is open, changing lots of data, and doing a before/after check on the DB. I inevitably find myself writing helper classes to do the checking, and more often than not those helpers can then be added into the code base, as they have encapsulated emergent behaviour/logic/requirements. I don't mean I have a single huge test, what I do mean is mnay tests are doing work which a purist would call an integration test - does such a thing still exist? Also I've found it useful to create a test template and then create many tests from that, to check boundary conditions, complex processing etc.

BTW which language environment are we talking about? Some languages lend themselves to refactoring better than others.

For instructions on how to refactor legacy code, you might want to read the book Working Effectively with Legacy Code. There's also a short PDF version available here.

I am on the quest to be a good OO-developer. OO intrigues me, because I understand the patterns, know why composition gives you more flexibility then inheritance, and more of such wisdom. However, I came to the conclusion that I know how to implement a factory of a singleton, but that I do not know how to come up with a robust OO design.

I have a bunch of books

  1. Design Patterns by the GoF
  2. AntiPatterns Brown et al.
  3. Refactoring by Fowler
  4. Code complete 2

They might be very good books, but they don't teach you to architect an application. I am often paralysed by some very basic decisions (example). I am looking for a book that teaches the when and why. There are many books about hammers and nails, but I have yet to find a book that tells you something about their practical relationship.

What book was most instrumental in making you a confident OO-architect/designer?

  1. "Object-oriented software construction" by Bertrand Meyer

Most fundamental work about object-orientation ever published. This is absolutely must have book for every "object-oriented" programmmer.

2. "Object-Oriented Analysis and Design with Applications" by Grady Booch et al

Not so formal as Meyer's book, but this book can open your eyes on many questions in object-oriented world and in software development in general

3. "Design Patterns: Elements of Reusable Object-Oriented Software" by Erich Gamma et al.

This is famous "Gang of Four" book about design patterns

4. "Refactoring: Improving the Design of Existing Code" by Martin Fowler et al.

This is another classical book. First part perfectly describe many problem that modern software developer may faced during his work: code smells, readability vs performance, premature optimization drawbacks and many other topics.

5. "Thinking in Java" by Bruce Eckel

This book may help many beginners not only in Java language but in object-oriented way of thinking too.

6. "Touch of Class: Learning to Program Well with Objects and Contracts" by Bertrand Meyer

Excellent textbook by famous author.

Craig Larman's Applying UML and Patterns summarized a lot about what I had learned from experience. What I like about it is that it addresses all the aspects of software design -- which includes things like iterative design and development. Don't stare too hard at the use of UML: design descriptions are a means towards an end, and I found Larman's approach fairly pragmatic. You can't just code: you have to communicate your intentions (and understand what is needed). UML and cleanly designed, well commented code are some of the means towards that end.

And, of course, as others mention: no book will make you a good developer or designer. But it might help accelerate the process.

I have been used to do some refactorings by introducing compilation errors. For example, if I want to remove a field from my class and make it a parameter to some methods, I usually remove the field first, which causes a compilation error for the class. Then I would introduce the parameter to my methods, which would break callers. And so on. This usually gave me a sense of security. I haven't actually read any books (yet) about refactoring, but I used to think this is a relatively safe way of doing it. But I wonder, is it really safe? Or is it a bad way of doing things?

When you are ready to read books on the subject, I recommend Michael Feather's "Working Effectively with Legacy Code". (Added by non-author: also Fowler's classic book "Refactoring" - and the Refactoring web site may be useful.)

He talks about identifiying the characteristics of code you are working before you make a change and doing what he calls scratch refactoring. That is refectoring to find characteristics of the code and then throwing the results away.

What you are doing is using the compiler as an auto-test. It will test that your code compiles but not if the behaviour has changed due to your refactoring or if there were any side affects.

Consider this

class myClass {
     void megaMethod() 
     {
         int x,y,z;
         //lots of lines of code
         z = mysideEffect(x)+y;
         //lots more lines of code 
         a = b + c;
     }
}

you could refactor out the addtion

class myClass {
     void megaMethod() 
     {
         int a,b,c,x,y,z;
         //lots of lines of code
         z = addition(x,y);
         //lots more lines of code
         a = addition(b,c);  
     }

     int addition(int a, b)
     {
          return mysideaffect(a)+b;
     }
}

and this would work but the second additon would be wrong as it invoked the method. Further tests would be needed other than just compilation.

This sounds similar to an absolutely standard method used in Test-Driven Development: write the test referring to a nonexistent class, so that the first step to make the test pass is to add the class, then the methods, and so on. See Beck's book for exhaustive Java examples.

Your method for refactoring sounds dangerous because you don't have any tests for safety (or at least you don't mention that you have any). You might create compiling code that doesn't actually do what you want, or breaks other parts of your application.

I'd suggest you add a simple rule to your practise: make noncompiling changes only in unit test code. That way you are sure to have at least a local test for each modification, and you are recording the intent of the modification in your test before making it.

By the way, Eclipse makes this "fail, stub, write" method absurdly easy in Java: each nonexistent object is marked for you, and Ctrl-1 plus a menu choice tells Eclipse to write a (compilable) stub for you! I'd be interested to know if other languages and IDEs provide similar support.

Over the course of time, my team has created a central class that handles an agglomeration of responsibilities and runs to over 8,000 lines, all of it hand-written, not auto-generated.

The mandate has come down. We need to refactor the monster class. The biggest part of the plan is to define categories of functionality into their own classes with a has-a relationship with the monster class.

That means that a lot of references that currently read like this:

var monster = new orMonster();
var timeToOpen = monster.OpeningTime.Subtract(DateTime.Now);

will soon read like this:

var monster = new Monster();
var timeToOpen = monster.TimeKeeper.OpeningTime.Subtract(DateTime.Now);

The question is: How on Earth do we coordinate such a change? References to "orMonster" litter every single business class. Some methods are called in literally thousands of places in the code. It's guaranteed that, any time we make such a chance, someone else (probably multiple someone elses) on the team will have code checked out that calls the .OpeningTime property

How do you coordinate such a large scale change without productivity grinding to a halt?

You should make the old method call the new method. Then over time change the references to the old method to call the new method instead. Once all the client references are changed, you can delete the old method.

For more information, see Move Method in Martin Fowler's classic, Refactoring.

We are in the initial phase of trying to implement TDD. I demo'd the Visual Studio Team System code coverage / TDD tools and the team is excited at the possibilities. Currently we use Devpartner for code coverage, but we want to eliminate it because its expensive. We have very limited experience in TDD and want to make sure we don't go a wrong direction. Currently we are using SourceSafe for source control but will be migrating to Team System in about a year.

I can tell you our applications are very data centric. We have about 900 tables, 6000 stored procedures, and about 45GB of data. We have lots of calculations that are based upon userdata and different rates in the system. Also a lot of our code is based upon time (calculate interest to the current date). Some of these calculations are very complex and very intensive (only a few people know the details for some of them).

We want to implement TDD to solve QA issues. A lot of developers are forced to fix bugs in areas they are not familiar with and end up breaking something. There are also areas that developers are almost afraid to touch because the code is used by everything in the system. We want to mitigate this problem.

I'm afraid since our code is so data centric that implementing TDD might be a little bit more complex than most systems. I'm trying to come up with a gameplan that I can present to management but I want to hopefully not get caught up in some of the TDD beginner mistakes. Also if tools / facilities in Team System make TDD more complete then that would be nice but we don't want to wait for Team System to get started.

The first question we our asking is should we just start with the tools in visual studio? I have read post were people complain about the intrinsic tools in visual studio (need to create a separate project to create your test) but the one thing about the tools in visual studio is they are free and the integration is good. If we decide to go the other route in using something like XUnit, MBUnit, or NUnit then we are most likely going to have some maybe significant cost:

1) If we want IDE Integration (failed to mention most of our code is VB.Net)
---TestDriven.Net or Resharper or ?????

2) If we want code coverage
---NCover (Seems pretty pricey for its functionality)

Also I've seen some pretty cool functionality demoed in visual studio 2010. Like the ability to do input testing (data entered on a form) or the ability to record what the user has done and then feed that into your unit test to reproduce a problem.

Also, although I don't quite grasp the mocking object concept yet, I know a lot of people feel it's a must. The question is can all the mocking frameworks plug into visual studio's version of TDD (MSTEST)?

I have advised management that we should probably just add regression testing going forward (new development or found bugs) but not try to go through all our code and put in unit tests. It would be WAY too big of project.

Anyways, I would appreciate anyones help.

With regard to getting started, I would also recommend reading Fowler's Refactoring. The first chapter gives a good feel for what it means to introduce tests then safely introduce change (although the emphasis here is on behaviour preserving change). Furthermore, this talk describes some practices which can help improve the testability of your code. Misko Hevery also has this guide to writing testable code, which summarises the talk.

From your description, it sounds as though you want to test the core parts of your system - the parts with a lot of dependencies where changes are scary. Depending on the degree to which data access is decoupled from business logic, you will probably need to refactor towards a state where the code is more testable - where it is easy and fast to instantiate sets of test data in order to verify the logic in isolation. This may be a big job, and may not be worth the effort if changes here are infrequent and the code base is well proven.

My advice would be to be pragmatic, and use the experience of the team to find areas where it is easiest to add tests that add value. I think having many focussed unit tests is the best way to drive quality, but it is probably easier to test the code at a higher level using integration or scenario tests, certainly in the beginning. This way you can detect big failures in your core systems early. Be clear on what your tests cover. Scenario tests will cover a lot of code, but probably won't surface subtle bugs.

Moving from SourceSafe to Team System is a big step, how big depends on how much you want to do in Team System. I think you can get a lot of value from using Visual Studio's built in test framework. For example, as a first step you could implement some basic test suites for the core system/core use cases. Developers can run these themselves in Visual Studio as they work and prior to check in. These suites can be expanded gradually over time. Later when you get TFS, you can look at running these suites on check in and as part of an automated build process. You can follow a similar path regardless of the specific tools.

Be clear from the outset that there is overhead in maintaining test code, and having well designed tests can pay dividends. I have seen situations where tests are copy pasted then edited slightly etc. Test code duplication like this can lead to an explosion in the number of lines of test code you need to maintain when effecting a small product code change. This kind of hassle can erode the perceived benefit of having the tests.

Visual Studio 2008 will only show you block coverage, although the code analysis will also give other metrics like cyclomatic complexity per assembly/class/method. Getting high block coverage with your tests is certainly important, and allows you to easily identify areas of the system that are totally untested.

However, I think it is important to remember that high block coverage is only a simple measurement of the effectiveness of your tests. For example, say you write a class to purge a file archive and keep the 5 newest files. Then you write a test case which checks if you start with 10 files then run the purger you are left with 5. An implementation which passes the test could delete the newest files, but could easily give 100% coverage. This test only verifies 1 of the requirements.

First thing to do is get this book:

Working Effectively with Legacy Code

For such a large project, read it and internalize it. TDD on a data driven application is hard enough. On a legacy one you need some serious planning and effort. Worth it in my view, but it is still a big curve.

100 thumbs up for Working Effectively with Legacy Code recommended by Yishai. I also recommend Pragmatic Unit Testing in C# with NUnit as you're using .NET (although I'm assuming C#). It's been very useful for teaching the basics of unit testing, providing a solid foundation to work from.

Martin Fowler says that we should do refactoring before adding new features (given that the original program is not well-structured).

So we all want to refactor this dirty codebase, that's for sure. We also know that without unit-testing code it's very easy to introduce subtle bugs.

But it's a large codebase. Adding a complete suite of tests to it seems impracticable.

What would you do in this case?

Let me recommend the book Working effectively with legacy code by Michael Feathers. It contains a lot of realistic examples and shows good techniques for tackling the legacy code beast.

Fowler also suggests that you never refactor without the safety of tests. But, how do you get those tests in place? And, how far do you go?

The previously recommended book (Working Effectively with Legacy Code by Michael Feathers) is the definitive work on the subject.

Need a quicker read? Take a look at Michael's earlier article (PDF) of the same name.

I'm sure others agree, when code hits the pavement it is much easier to think of all the elements that will go into your application. Meaning, once you get going, you think of the what's next because you have reached a point where you have to design another table or class.

The drawbacks of this approach are obvious and we don't need to go into it.

The question is, for those of us who can't conceptualize the design without "seeing it", how do we go from design at the seat of our pants to a more structured methodology?

The only solution I can think of is detailed specifications, which forces one to think of exactly how things are going to work.

Is this just a case of how people think differently, or is there something else I am ignoring?

If you haven't looked at this book Refactoring by Martin Fowler you really should. The crime isn't so much the rapid prototyping/development it's knowing how to role that last little bit into something more maintainable in fairly short order. Yes you will discover new objects as you go, now spend a little time cleaning up and then push on. Often you end up with a better product than if you tried to design most of the solution before hand.

Background:

An art teacher once gave me a design problem to draw a tiger using only 3 lines. The idea being that I study a tiger and learn the 3 lines to draw for people to still be able to tell it is a tiger.

The solution for this problem is to start with a full drawing of a tiger and remove elements until you get to the three parts that are most recognizable as a tiger.

I love this problem as it can be applied in multiple disciplines like software development, especially in removing complexity.

At work I deal with maintaining a large software system that has been hacked to death and is to the point of becoming unmaintainable. It is my job to remove the burdensome complexity that was caused by past developers.

Question:

Is there a set process for removing complexity in software systems - a kind of reduction process template to be applied to the problem?

This is a loaded question :-)

First, how do we measure "complexity"? Without any metric decided apriori, it may be hard to justify any "reduction" project.

Second, is the choice entirely yours? If we may take an example, assume that, in some code base, the hammer of "inheritance" is used to solve every other problem. While using inheritance is perfectly right for some cases, it may not be right for all cases. What do you in such cases?

Third, can it be proved that behavior/functionality of the program did not change due to refactoring? (This gets more complex when the code is part of a shipping product.)

Fourth, you can start with start with simpler things like: (a) avoid global variables, (b) avoid macros, (c) use const pointers and const references as much as possible, (d) use const qualified methods wherever it is the logical thing to do. I know these are not refactoring techniques, but I think they might help you proceed towards your goal.

Finally, in my humble opinion, I think any such refactoring project is more of people issue than technology issue. All programmers want to write good code, but the perception of good vs. bad is very subjective and varies across members in the same team. I would suggest to establish a "design convention" for the project (Something like C++ Coding Standards). If you can achieve that, you are mostly done. The remaining part is modify the parts of code which does not follow the design convention. (I know, this is very easy to say, but much difficult to do. Good wishes to you.)

Checkout the book, Working Effectively with Legacy Code

The topics covered include

  • Understanding the mechanics of software change: adding features, fixing bugs, improving design, optimizing performance
  • Getting legacy code into a test harness
  • Writing tests that protect you against introducing new problems
  • Techniques that can be used with any language or platform—with examples in Java, C++, C, and C#
  • Accurately identifying where code changes need to be made
  • Coping with legacy systems that aren't object-oriented
  • Handling applications that don't seem to have any structure

This book also includes a catalog of twenty-four dependency-breaking techniques that help you work with program elements in isolation and make safer changes.

Check out the book Anti-Patterns for a well-written book on the whole subject of moving from bad (or maladaptive) design to better. It provides ways to recover from a whole host of problems typically found in software systems. I would then add support to Kristopher's recommendation of Refactoring as an important second step.

I have two buttons: btnAdd and btnUpdate. I have written a jquery function for button btnUpdate to validate some fields in the web page like:

$(function() {

  $('#<%=btnUpdate.ClientID %>').click(function() {
    code here
    });
});

I want to do the same thing when btnAdd is clicked. So I have to write the same function again for btnAdd. Is there other way to avoid this?

(If I write one javascript function and call the same function in the button click event of both buttons, it's fine. But is there any way using jQuery?)

We call this Refactoring and it's something that will help you all the way, read more about, invest in yourself and buy the fantastic one and only Refactoring book

in your case, you should do this:

$(function() {

  $('#<%=btnUpdate.ClientID %>').click(function() {
        validate($this);
    });
  $('#<%=btnAdd.ClientID %>').click(function() {
        validate($this);
    });    

});

function validate(myButton)
{
    // code here
}

As you should always do.

but as jQuery you can have selectors with multiple inputs, so all you need to do is:

$('#<%=btnUpdate.ClientID %>, #<%=btnAdd.ClientID %>').click(function() { 
    validate($this);
}

Refactoring is the process of improving the existing system design without changing its behavior.

Besides Martin Fowler's seminal book "Refactoring - Improving the design of existing code" and Joshua Kerievsky's book "Refactoring to Patterns", are there any good resources on refactoring?

Working Effectively with Legacy Code focuses on dealing with existing code-bases that need to evolve to be testable. Many techniques are used in the book to accomplish this, and is an excellent resource for refactoring.

I would recommend reading Working Effectively with Legacy Code, then Refactoring - Improving the design of existing code. Martin Fowler's book is more like a receipt book for me, it explains how. Working effectively with legacy code, explains the why in my opinion.

below is some other books relating to refactoring:

antipatterns refactoring software architectures and projects in crisis

refactoring in large software projects performing complex restructurings

refactoring sql applications

Prefactoring

Until asking a question on here I never considered (enums) to be a "bad thing." For those out there that consider them not to be best practice, what are some approachs/patterns for avoiding their use in code?

Edit:

public Enum SomeStatus
 Approved = 1
 Denied = 2
 Pending =3
end Enum

The problem with enums is described in Fowler's Refactoring, where it is considered a code smell. It has nothing to do with type safety, but rather that it forces you to sprinkle switch statements all over your code, thus violating the DRY Principle.

The State pattern is a better model of the same structure because it lets you implement and vary the logic related to the same state in the same class. This also increases cohesion and lessens class coupling.

Yes, the dreaded 'M' word.

You've got a workstation, source control and half a million lines of source code that you didn't write. The documentation was out of date the moment that it was approved and published. The original developers are LTAO, at the next project/startup/loony bin and not answering email.

What are you going to do?

{favourite editor} and Grep will get you started on your spelunking through the gnarling guts of the code base but what other tools should be in the maintenance engineers toolbox?

To start the ball-rolling; I don't think I could live without source-insight for C/C++ spelunking. (DISCLAIMER: I don't work for 'em).

Just like eating the elephant - one bite at a time :)

Sometimes the big picture can be a real demotivator, and you need to pick a spot and tackle it piece by piece.

Of course, still need to choose the bit to start on... Typically this is driven most by the users/business with top priority specific changes required (yesterday...) but if you have a little flexibility or familiarization time, metrics are often useful. Tools here vary with the technology and language, but tools like NDepend and JDepend, any built in Code Metrics (like in in Visual Studio Team System, or the various available Eclipse plugins) or a tool like Simian to get a feel for the size of the copy and paste problem.

Hopefully the number of unit tests and coverage is greater than zero, and so a good first step is always to get whatever tests you can running in a Continuous Integration environment, as a foundation for adding more tests as you learn.

And as others have said - assuming options are available for the language - a good IDE with code navigation and automated refactoring is a must (Eclipse, Visual Studio (with or without ReSharper).

A couple of morale-boosting books:

Good luck :)

Question

My question is how can you teach the methods and importance of tidying-up and refactoring code?

Background

I was recently working on a code review for a colleague. They had made some modifications to a long-gone colleagues work. During the new changes, my colleague had tried to refactor items but gave up as soon as they hit a crash or some other problem (rather than chasing the rabbit down the hole to find the root of the issue) and so reimplemented the problem code and built more on top of that. This left the code in a tangle of workarounds and magic numbers, so I sat down with them to go through refactoring it.

I tried to explain how I was identifying the places we could refactor and how each refactoring can often highlight new areas. For example, there were two variables that stored the same information - why? I guessed it was a workaround for a bigger issue so I took out one variable and chased the rabbit down the hole, discovering other problems as we went. This eventually led to finding a problem where we were looping over the same things several times. This was due in no small part to the use of arrays of magic number sizes that obfuscated what was being done - fixing the initial "double-variable" problem led to this discovery (and others).

As I went on this refactoring journey with my colleague, it was evident that she wasn't always able to grasp why we made certain changes and how we could be sure the new functionality matched the original, so I took the time to explain and prove each change by comparing with earlier versions and stepping through the changes on paper. I also explained, through examples, how to tell if a refactoring choice was a bad idea, when to choose comments instead of code changes, and how to select good variable names.

I felt that the process of sitting together to do this was worthwhile for both myself (I got to learn a bit more about how best to explain things to others) and my colleague (they got to understand more of our code and our coding practices) but, the experience led me to wonder if there was a better way to teach the refactoring process.

...and finally...

I understand that what does or does not need refactoring, and how to refactor it are very subjective so I want to steer clear of that discussion, but I am interested to learn how others would tackle the challenge of teaching this important skill, and if others here have had similar experiences and what they learned from them (either as the teacher or the student).

If you haven't read it, Martin Fowler has an excellent book on the subject called Refactoring: Improving the Design of Existing Code. He goes into substantial detail about how and why a specific piece of code should be refactored.

I hesitated to even mention it for fear that knowledge of this book is assumed by someone asking about refactoring, and that you would think, "Duh, I meant besides the Fowler book." But what the hey, there you go. :-)

I have a Python unittest, with some tests having the same type object tested. The basic outline in one test-class is:

class TestClass(unittest.TestCase):
    def setup(self):
        ...

    def checkObjects(self, obj):
        for i in [...values...]:
            self.assertEqual(starttags(i,obj), endtags(i,obj))

    def testOne(self):
        #Get object one.
        checkObjects(objone)

    def testAnother(self):
        #Access another object.
        checkObjects(another)

    ... various tests for similar objects.

Although it's modular, I noticed that any failures will give an error like AssertionError: number != anothernumber, and the line of code generating the error, self.assertEqual(starttags(i,obj), endtags(i,obj)). If I had listed the tests out instead of placing in a for loop, I would have something like:

self.assertEqual(starttags(value1,obj), endtags(value1,obj))
self.assertEqual(starttags(value2,obj), endtags(value2,obj))

Which shows exactly which case causes the error, but is copy-paste code, which I thought was generally not recommended. I noticed the problem recently when a contributor reworked a more clean unit-test, that unfortunately would give little debug-info on assertion failures. So, what is the best-practice in these cases? Something like a list of tuples, fed into a for-loop with assertEquals is "cleaner", but copy-paste with different values on different lines gives useful stack traces.

If by cleaner you mean less code, then this kind of cleaner code is not always more readable code. In fact, it usually is less readable (especially when you come back to it). You can always go with fancy refactorings but you need to know when to stop. In longer run, it's always better to use more obvious, simple code than trying to squeeze one less line of code for artificial gains - not just in terms of unit testing.

Unit tests go with their own rules. For example they usually allow different naming conventions than what your regular code standards say, you hardly ever document it - they're kind of special part of your codebase. Also, having repeated code is not that uncommon. Actually, it's rather typical to have many similar looking, small tests.

Design your tests (code) with simplicity in mind

And at the moment, your tests are confusing even at the stage of writing them - imagine coming back to that code in 3 months from now. Imagine one of those tests breaking as a result of somebody else doing some change in some other place. It won't get any better.

Design your tests in such a way, that when one of them breaks, you immediately know why it did so and where. Not only that - design them in such a way, that you can tell what they are doing in a blink of an eye. Having for loops, ifs and basically any other kind of control flow mechanism (or too extensive refactoring) in test code, usually results in one question springing to your mind - What are we doing here? This is the kind of question you don't want to find yourself asking.

To summarize this longish post with words of people smarter than myself:

Any fool can write code that a computer can understand. Good programmers write code that humans can understand.

-Martin Fowler et al, Refactoring: Improving the Design of Existing Code, 1999

Do yourself a favor and stick to that rule.

I'll start by saying I am pretty new to unit testing and I'd like to start using a TDD approach, but for now am writing unit tests for some existing classes to verify their functionality in all cases.

I've been able to test the majority of my code using NUnit and Rhino mocks without much trouble. However, I've been wondering about unit testing functions that end up calling a lot of other methods within the same class. I can't do something like

classUnderTest.AssertWasCalled(cut => cut.SomeMethod(someArgs))

because the class under test isn't a fake. Furthermore, if a method I'm testing calls other methods in the class under test that in turn also call methods in the same class, I'm going to need to fake a ton of values just to test the "top level" method. Since I'm also unit testing all of these "sub methods", I should be able to assume that "SomeMethod" works as expected if it passes the unit test and not need to worry about the details of those lower-level methods.

Here is some example code I've been working with to help illustrate my point (I've written a class to manage import/export of Excel files using NPOI):

    public DataSet ExportExcelDocToDataSet(bool headerRowProvided)
    {
        DataSet ds = new DataSet();

        for (int i = 0; i < currentWorkbook.NumberOfSheets; i++)
        {               
            ISheet tmpSheet = currentWorkbook.GetSheetAt(i);

            if (tmpSheet.PhysicalNumberOfRows == 0) { continue; }
            DataTable dt = GetDataTableFromExcelSheet(headerRowProvided, ds, tmpSheet);

            if (dt.Rows.Count > 0)
            {
                AddNonEmptyTableToDataSet(ds, dt);
            }
        }

        return ds;
    }

    public DataTable GetDataTableFromExcelSheet(bool headerRowProvided, DataSet ds, ISheet tmpSheet)
    {
        DataTable dt = new DataTable();
        for (int sheetRowIndex = 0; sheetRowIndex <= tmpSheet.LastRowNum; sheetRowIndex++)
        {
            DataRow dataRow = GetDataRowFromExcelRow(dt, tmpSheet, headerRowProvided, sheetRowIndex);
            if (dataRow != null && dataRow.ItemArray.Count<object>(obj => obj != DBNull.Value) > 0)
            {
                dt.Rows.Add(dataRow);
            }
        }

        return dt;
    }

...

You can see that ExportExcelDocToDataSet (my "top-level" method in this case) calls GetDataTableFromExcelSheet which calls GetDataRowFromExcelRow, which calls a couple of other methods that are defined within this same class.

So what is the recommended strategy for refactoring this code to make it more unit testable without having to stub values called by submethods? Is there a way to fake method calls within the class under test?

Thanks in advance for any help or advice!

Modify the subject under test (SUT). If something is hard to unit test, then the design might be awkward.

Faking method calls within the class under test leads to over specified tests. The result are very brittle tests: As soon as you modify or refactor the class, then it is very likely that you also need modify the unit tests. This leads too high maintenance costs of unit testing.

To avoid over specified tests, concentrate on public methods. If this method calls other methods within the class, do not test these calls. On the other hand: Method calls on other dependend on component (DOCs) should be tested.

If you stick to that and have the feeling that you miss some important thing in your tests, then it might be a sign for a class or a method which is doing too much. In case of a class: Look for violations of the Single Responsibility Principle (SRP). Extract classes out of it and test them separately. In case of a method: Split it up the method in several public methods and test each of them separately. If this is still too awkward, you definitely have a class which violates the SRP.

In your specific case you can do the following: Extract the methods ExportExcelDocToDataSet and GetDataTableFromExcelSheet into two different classes (maybe call them ExcelToDataSetExporter and ExcelSheetToDataTableExporter). The original class which contained both methods should reference both classes and call those methods, which you previously extracted. Now you are able to test all three classes in isolation. Apply the Extract Class refactoring (book) to achieve the modification of your original class.

Also note that retrofitting tests are always a bit cumbersome to write and maintain. The reason is that the SUTs, which are written without unit tests, tend to have an awkward design and thus are harder to test. This means that the problems with unit tests must be solved by modifying the SUTs and cannot be solved by pimping up the unit tests.

I'm trying to make my code easily understood by future readers.

I've always had issues with how to word my if statement comments to make it the most understandable.

Maybe it seems trivial, but it's something that's always bothered me

Here's an example:

if ( !$request ) {
    $request = $_SERVER['REQUEST_URI'];
}

Here are some way I can think of commenting it

// If request doesn't exist
if ( !$request ) {
    // Set request to current request_uri
    $request = $_SERVER['REQUEST_URI'];
}

// Check for a request
if ( !$request ) {
    $request = $_SERVER['REQUEST_URI'];
}

// Request doesn't exist
if ( !$request ) {
    // Set request
    $request = $_SERVER['REQUEST_URI'];
}

Not the best example, but the way I see it there are infinite ways to word it.

I've never really worked on a team so I don't have much experience on other people reading my code.

What are your experiences on the best way to word that to make it readable for future coders.

Read the book Clean Code by Robert Martin.

http://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882

Comments should be only used to explain concepts that the code can't explain itself.

As said before, maybe your example is a bit too simple in order really have a relevant comment on it, but here are a couple of general suggestions:

  • usually is easier to read "positive" conditions rather than negations (not condition)

  • don't hesitate to extract methods that will have a detailed name that will communicate the intent and avoid thus the need of some of the comments. In your case, say you create:

    function getRequest( $req ) {
        if( isset( $req ) ) {
            return $req;
        } else {
            return $_SERVER['REQUEST_URI'];
        }
    }
    

    but again, we need a more appropriate example, in your case it might be overkill

  • must read :

I tend to successfully write compact applications that can encapsulate many business logic in a simple and non-redundant way. I tend to create small methods, but over time I arrive to methods that have too many parameters. I know every code requires its design, but I see an anti-pattern in my behavior and I am not sure which would be the best way to fight against it.

A typical situation would be something like:

   public loanStructure CalculateSomeComplicatedInterestRate(enum clientType, 
        int totalAmout, bool useTaxes, bool doYearlySummary, bool doNotReloadClient, etc)
   {
        loanStructure ret = new loanStructure();
        if (doNotReloadClient) ... ;
        ...
        if (doYearlySummary) GroupResults(ret);
        return ret;
   }

and inside the method, a tree of calls forwards the boolean settings (doYearlySummary, doNotReloadClient, etc) to different business rules that acts on the calculation.

I.E., the problem does not reside on the fact that all parameters could be encapsulated into an object (bigParameterStructure)... I am not comfortable with the masterMethod pattern, but making overloads such as CalculateSomeComplicatedInterestRateMonthly and CalculateSomeComplicatedInterestRateYearly would just hide a private CalculateSomeComplicatedInterestRate.... some problem!!

Of coarse object-orienting design would help... but I still end-up having this kind of methods somewhere on my objects...

Well guys... any help is welcome.

Pablo.

When I face code smells I scan Refactoring: Improving the Design of Existing Code and Refactoring to Patterns. This books are plenty of useful advice to refactor code.

My project invovles me to make a lot of changes on the production code. The requirement keeps coming up and I need to make changes and deploy it as soon as possible. I sometimes end up creating patch work sort of code because some of the requirement would not fit into the overall design of the software. How can this be handled effectively? Any design pattern to handle this?

Besides the obvious answers of revision control, unit tests, and an automated build system, it sounds like you also need to do some heavy refactoring. If your design keeps changing based on changing requirements then you should try to isolate the parts of your code that keep changing in their own modules.

When I receive code I have not seen before to refactor it into some sane state, I normally fix "cosmetic" things (like converting StringTokenizers to String#split(), replacing pre-1.2 collections by newer collections, making fields final, converting C-style arrays to Java-style arrays, ...) while reading the source code I have to get familiar with.

Are there many people using this strategy (maybe it is some kind of "best practice" I don't know?) or is this considered too dangerous, and not touching old code if it is not absolutely necessary is generally prefered? Or is it more common to combine the "cosmetic cleanup" step with the more invasive "general refactoring" step?

What are the common "low-hanging fruits" when doing "cosmetic clean-up" (vs. refactoring with more invasive changes)?

You can buy a copy of Refactoring: Improving the Design of Existing Code from Martin Fowler, you'll find a lot of things you can do during your refactoring operation.

Plus you can use tools provided by your IDE and others code analyzers such as Findbugs or PMD to detect problems in your code.


Resources :

On the same topic :

I'm trying to design a simple game ---Pong, Arkanoid--- using strictly "Proper OO", whatever that means.

So, by designing myself to death, I'm trying to get to a point where I'll know what to do or not do next time...

Who should handle, for example, colissions? And scorekeeping?

My first idea was giving a couple jobs to the ball, but that started expanding exponentially into a god object: The ball'd know where it is, who it crashed into, and report to the scorekeeping objects.

And that's too much for the poor ball.

A good or bad design reveals itself by how well it accomodates unexpected requirements, so I would suggest keeping a stock of potential "game features" handy to inform your design reflexions. Since you're doing this as a learning project you can afford to go crazy.

Arkanoid is a very good choice for this, it offers so many options. Make different bricks score different amounts of points. Make some bricks change the score of other bricks when hit. Make some bricks require multiple hits. Give superpowers to the ball, paddle, or bricks. Vary these powers: one of them makes the ball keyboard-controllable, another makes it transparent, another reverses "gravity", and so on. Make bricks drop objects.

The goal is that when you make such a change, it impacts the minimum possible number of classes and methods. Get a feel for how your design must change to fit this criterion.

Use an IDE that has a Refactoring menu, in particular the move method refactoring. (If you haven't, read the book Refactoring.) Experiment with placing your various methods here and there. Notice what becomes hard to change when the method is placed "wrong", and what becomes easier when you place it elsewhere. Methods are placed right when objects take care of their own state; you can "tell" an object to do something, rather than "ask" it questions about its state and then make decisions based on its answers.

Let's assume that in your design each sprite is an object instance. (You could choose other strategies.) Generally, motion alters the state of a sprite, so the method that describes motion for a particular kind of sprite probably belongs on that sprite's class.

Collision detection is a sensitive part of the code, as it potentially involves checking all possible pairs of sprites. You'll want to distinguish checking for collisions and informing objects of collisions. Your ball object needs to alter its motion on colliding with the paddle, for instance. But the algorithm for detecting collisions in general won't belong on the ball class, since other pairs of objects may collide with consequences that matter to the game.

And so on...

I've been tinkering with small functions on my own time, trying to find ways to refactor them (I recently read Martin Fowler's book Refactoring: Improving the Design of Existing Code). I found the following function MakeNiceString() while updating another part of the codebase near it, and it looked like a good candidate to mess with. As it is, there's no real reason to replace it, but it's small enough and does something small so it's easy to follow and yet still get a 'good' experience from.

private static string MakeNiceString(string str)
        {
            char[] ca = str.ToCharArray();
            string result = null;
            int i = 0;
            result += System.Convert.ToString(ca[0]);
            for (i = 1; i <= ca.Length - 1; i++)
            {
                if (!(char.IsLower(ca[i])))
                {
                    result += " ";
                }
                result += System.Convert.ToString(ca[i]);
            }
            return result;
        }


static string SplitCamelCase(string str)
    {
        string[] temp = Regex.Split(str, @"(?<!^)(?=[A-Z])");
        string result = String.Join(" ", temp);
        return result;
    }

The first function MakeNiceString() is the function I found in some code I was updating at work. The purpose of the function is to translate ThisIsAString to This Is A String. It's used in a half-dozen places in the code, and is pretty insignificant in the whole scheme of things.

I built the second function purely as an academic exercise to see if using a regular expression would take longer or not.

Well, here are the results:

With 10 Iterations:

MakeNiceString took 2649 ticks
SplitCamelCase took 2502 ticks

However, it changes drastically over the longhaul:

With 10,000 Iterations:

MakeNiceString took 121625 ticks
SplitCamelCase took 443001 ticks

Refactoring MakeNiceString()

The process of refactoring MakeNiceString() started with simply removing the conversions that were taking place. Doing that yielded the following results:

MakeNiceString took 124716 ticks
ImprovedMakeNiceString took 118486

Here's the code after Refactor #1:

private static string ImprovedMakeNiceString(string str)
        { //Removed Convert.ToString()
            char[] ca = str.ToCharArray();
            string result = null;
            int i = 0;
            result += ca[0];
            for (i = 1; i <= ca.Length - 1; i++)
            {
                if (!(char.IsLower(ca[i])))
                {
                    result += " ";
                }
                result += ca[i];
            }
            return result;
        }

Refactor#2 - Use StringBuilder

My second task was to use StringBuilder instead of String. Since String is immutable, unnecessary copies were being created throughout the loop. The benchmark for using that is below, as is the code:

static string RefactoredMakeNiceString(string str)
        {
            char[] ca = str.ToCharArray();
            StringBuilder sb = new StringBuilder((str.Length * 5 / 4));
            int i = 0;
            sb.Append(ca[0]);
            for (i = 1; i <= ca.Length - 1; i++)
            {
                if (!(char.IsLower(ca[i])))
                {
                    sb.Append(" ");
                }
                sb.Append(ca[i]);
            }
            return sb.ToString();
        }

This results in the following Benchmark:

MakeNiceString Took:           124497 Ticks   //Original
SplitCamelCase Took:           464459 Ticks   //Regex
ImprovedMakeNiceString Took:   117369 Ticks   //Remove Conversion
RefactoredMakeNiceString Took:  38542 Ticks   //Using StringBuilder

Changing the for loop to a foreach loop resulted in the following benchmark result:

static string RefactoredForEachMakeNiceString(string str)
        {
            char[] ca = str.ToCharArray();
            StringBuilder sb1 = new StringBuilder((str.Length * 5 / 4));
            sb1.Append(ca[0]);
            foreach (char c in ca)
            {
                if (!(char.IsLower(c)))
                {
                    sb1.Append(" ");
                }
                sb1.Append(c);
            }
            return sb1.ToString();
        }
RefactoredForEachMakeNiceString    Took:  45163 Ticks

As you can see, maintenance-wise, the foreach loop will be the easiest to maintain and have the 'cleanest' look. It is slightly slower than the for loop, but infinitely easier to follow.

Alternate Refactor: Use Compiled Regex

I moved the Regex to right before the loop is begun, in hopes that since it only compiles it once, it'll execute faster. What I found out (and I'm sure I have a bug somewhere) is that that doesn't happen like it ought to:

static void runTest5()
        {
            Regex rg = new Regex(@"(?<!^)(?=[A-Z])", RegexOptions.Compiled);
            for (int i = 0; i < 10000; i++)
            {
                CompiledRegex(rg, myString);
            }
        }
 static string CompiledRegex(Regex regex, string str)
    {
        string result = null;
        Regex rg1 = regex;
        string[] temp = rg1.Split(str);
        result = String.Join(" ", temp);
        return result;
    }

Final Benchmark Results:

MakeNiceString Took                   139363 Ticks
SplitCamelCase Took                   489174 Ticks
ImprovedMakeNiceString Took           115478 Ticks
RefactoredMakeNiceString Took          38819 Ticks
RefactoredForEachMakeNiceString Took   44700 Ticks
CompiledRegex Took                    227021 Ticks

Or, if you prefer milliseconds:

MakeNiceString Took                  38 ms
SplitCamelCase Took                 123 ms
ImprovedMakeNiceString Took          33 ms
RefactoredMakeNiceString Took        11 ms
RefactoredForEachMakeNiceString Took 12 ms
CompiledRegex Took                   63 ms

So the percentage gains are:

MakeNiceString                   38 ms   Baseline
SplitCamelCase                  123 ms   223% slower
ImprovedMakeNiceString           33 ms   13.15% faster
RefactoredMakeNiceString         11 ms   71.05% faster
RefactoredForEachMakeNiceString  12 ms   68.42% faster
CompiledRegex                    63 ms   65.79% slower

(Please check my math)

In the end, I'm going to replace what's there with the RefactoredForEachMakeNiceString() and while I'm at it, I'm going to rename it to something useful, like SplitStringOnUpperCase.

Benchmark Test:

To benchmark, I simply invoke a new Stopwatch for each method call:

       string myString = "ThisIsAUpperCaseString";
       Stopwatch sw = new Stopwatch();
       sw.Start();
       runTest();
       sw.Stop();

     static void runTest()
        {

            for (int i = 0; i < 10000; i++)
            {
                MakeNiceString(myString);
            }


        }

Questions

  • What causes these functions to be so different 'over the long haul', and
  • How can I improve this function a) to be more maintainable or b) to run faster?
  • How would I do memory benchmarks on these to see which used less memory?

Thank you for your responses thus far. I've inserted all of the suggestions made by @Jon Skeet, and would like feedback on the updated questions I've asked as a result.

NB: This question is meant to explore ways to refactor string handling functions in C#. I copied/pasted the first code as is. I'm well aware you can remove the System.Convert.ToString() in the first method, and I did just that. If anyone is aware of any implications of removing the System.Convert.ToString(), that would also be helpful to know.

1) Use a StringBuilder, preferrably set with a reasonable initial capacity (e.g. string length * 5/4, to allow one extra space per four characters).

2) Try using a foreach loop instead of a for loop - it may well be simpler

3) You don't need to convert the string into a char array first - foreach will work over a string already, or use the indexer.

4) Don't do extra string conversions everywhere - calling Convert.ToString(char) and then appending that string is pointless; there's no need for the single character string

5) For the second option, just build the regex once, outside the method. Try it with RegexOptions.Compiled as well.

EDIT: Okay, full benchmark results. I've tried a few more things, and also executed the code with rather more iterations to get a more accurate result. This is only running on an Eee PC, so no doubt it'll run faster on "real" PCs, but I suspect the broad results are appropriate. First the code:

using System;
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Reflection;
using System.Text;
using System.Text.RegularExpressions;

class Benchmark
{
    const string TestData = "ThisIsAUpperCaseString";
    const string ValidResult = "This Is A Upper Case String";
    const int Iterations = 1000000;

    static void Main(string[] args)
    {
        Test(BenchmarkOverhead);
        Test(MakeNiceString);
        Test(ImprovedMakeNiceString);
        Test(RefactoredMakeNiceString);
        Test(MakeNiceStringWithStringIndexer);
        Test(MakeNiceStringWithForeach);
        Test(MakeNiceStringWithForeachAndLinqSkip);
        Test(MakeNiceStringWithForeachAndCustomSkip);
        Test(SplitCamelCase);
        Test(SplitCamelCaseCachedRegex);
        Test(SplitCamelCaseCompiledRegex);        
    }

    static void Test(Func<string,string> function)
    {
        Console.Write("{0}... ", function.Method.Name);
        Stopwatch sw = Stopwatch.StartNew();
        for (int i=0; i < Iterations; i++)
        {
            string result = function(TestData);
            if (result.Length != ValidResult.Length)
            {
                throw new Exception("Bad result: " + result);
            }
        }
        sw.Stop();
        Console.WriteLine(" {0}ms", sw.ElapsedMilliseconds);
        GC.Collect();
    }

    private static string BenchmarkOverhead(string str)
    {
        return ValidResult;
    }

    private static string MakeNiceString(string str)
    {
        char[] ca = str.ToCharArray();
        string result = null;
        int i = 0;
        result += System.Convert.ToString(ca[0]);
        for (i = 1; i <= ca.Length - 1; i++)
        {
            if (!(char.IsLower(ca[i])))
            {
                result += " ";
            }
            result += System.Convert.ToString(ca[i]);
        }
        return result;
    }

    private static string ImprovedMakeNiceString(string str)
    { //Removed Convert.ToString()
        char[] ca = str.ToCharArray();
        string result = null;
        int i = 0;
        result += ca[0];
        for (i = 1; i <= ca.Length - 1; i++)
        {
            if (!(char.IsLower(ca[i])))
            {
                result += " ";
            }
            result += ca[i];
        }
        return result;
    }

    private static string RefactoredMakeNiceString(string str)
    {
        char[] ca = str.ToCharArray();
        StringBuilder sb = new StringBuilder((str.Length * 5 / 4));
        int i = 0;
        sb.Append(ca[0]);
        for (i = 1; i <= ca.Length - 1; i++)
        {
            if (!(char.IsLower(ca[i])))
            {
                sb.Append(" ");
            }
            sb.Append(ca[i]);
        }
        return sb.ToString();
    }

    private static string MakeNiceStringWithStringIndexer(string str)
    {
        StringBuilder sb = new StringBuilder((str.Length * 5 / 4));
        sb.Append(str[0]);
        for (int i = 1; i < str.Length; i++)
        {
            char c = str[i];
            if (!(char.IsLower(c)))
            {
                sb.Append(" ");
            }
            sb.Append(c);
        }
        return sb.ToString();
    }

    private static string MakeNiceStringWithForeach(string str)
    {
        StringBuilder sb = new StringBuilder(str.Length * 5 / 4);
        bool first = true;      
        foreach (char c in str)
        {
            if (!first && char.IsUpper(c))
            {
                sb.Append(" ");
            }
            sb.Append(c);
            first = false;
        }
        return sb.ToString();
    }

    private static string MakeNiceStringWithForeachAndLinqSkip(string str)
    {
        StringBuilder sb = new StringBuilder(str.Length * 5 / 4);
        sb.Append(str[0]);
        foreach (char c in str.Skip(1))
        {
            if (char.IsUpper(c))
            {
                sb.Append(" ");
            }
            sb.Append(c);
        }
        return sb.ToString();
    }

    private static string MakeNiceStringWithForeachAndCustomSkip(string str)
    {
        StringBuilder sb = new StringBuilder(str.Length * 5 / 4);
        sb.Append(str[0]);
        foreach (char c in new SkipEnumerable<char>(str, 1))
        {
            if (char.IsUpper(c))
            {
                sb.Append(" ");
            }
            sb.Append(c);
        }
        return sb.ToString();
    }

    private static string SplitCamelCase(string str)
    {
        string[] temp = Regex.Split(str, @"(?<!^)(?=[A-Z])");
        string result = String.Join(" ", temp);
        return result;
    }

    private static readonly Regex CachedRegex = new Regex("(?<!^)(?=[A-Z])");    
    private static string SplitCamelCaseCachedRegex(string str)
    {
        string[] temp = CachedRegex.Split(str);
        string result = String.Join(" ", temp);
        return result;
    }

    private static readonly Regex CompiledRegex =
        new Regex("(?<!^)(?=[A-Z])", RegexOptions.Compiled);    
    private static string SplitCamelCaseCompiledRegex(string str)
    {
        string[] temp = CompiledRegex.Split(str);
        string result = String.Join(" ", temp);
        return result;
    }

    private class SkipEnumerable<T> : IEnumerable<T>
    {
        private readonly IEnumerable<T> original;
        private readonly int skip;

        public SkipEnumerable(IEnumerable<T> original, int skip)
        {
            this.original = original;
            this.skip = skip;
        }

        public IEnumerator<T> GetEnumerator()
        {
            IEnumerator<T> ret = original.GetEnumerator();
            for (int i=0; i < skip; i++)
            {
                ret.MoveNext();
            }
            return ret;
        }

        IEnumerator IEnumerable.GetEnumerator()
        {
            return GetEnumerator();
        }
    }
}

Now the results:

BenchmarkOverhead...  22ms
MakeNiceString...  10062ms
ImprovedMakeNiceString...  12367ms
RefactoredMakeNiceString...  3489ms
MakeNiceStringWithStringIndexer...  3115ms
MakeNiceStringWithForeach...  3292ms
MakeNiceStringWithForeachAndLinqSkip...  5702ms
MakeNiceStringWithForeachAndCustomSkip...  4490ms
SplitCamelCase...  68267ms
SplitCamelCaseCachedRegex...  52529ms
SplitCamelCaseCompiledRegex...  26806ms

As you can see, the string indexer version is the winner - it's also pretty simple code.

Hope this helps... and don't forget, there are bound to be other options I haven't thought of!

Part of our core product is a website CMS which makes use of various page widgets. These widgets are responsible for displaying content, listing products, handling event registration, etc. Each widget is represented by class which derives from the base widget class. When rendering a page the server grabs the page's widget from the database and then creates an instance of the correct class. The factory method right?

Private Function WidgetFactory(typeId)
    Dim oWidget
    Select Case typeId
        Case widgetType.ContentBlock
            Set oWidget = New ContentWidget
        Case widgetType.Registration
            Set oWidget = New RegistrationWidget
        Case widgetType.DocumentList
            Set oWidget = New DocumentListWidget
        Case widgetType.DocumentDisplay
    End Select
    Set WidgetFactory = oWidget
End Function

Anyways, this is all fine but as time has gone on the number of types of widgets has increased to around 50 meaning the factory method is rather long. Every time I create a new type of widget I go to add another couple of lines to the method and a little alarm rings in my head that maybe this isn't the best way to do things. I tend to just ignore that alarm but it's getting louder.

So, am I doing it wrong? Is there a better way to handle this scenario?

The WidgetFactory method is really a mapping from a typeId enumeration to concrete classes. In general it's best if you can avoid enumerations entirely, but sometimes (particularly in web applications) you need to round-trip to an environment (e.g. the browser) that doesn't understand polymorphism and you need such measures.

Refactoring contains a pretty good explanation of why switch/select case statements are code smells, but that mainly addresses the case where you have many similar switches.

If your WidgetFactory method is the only place where you switch on that particular enum, I would say that you don't have to worry. You need to have that map somewhere.

As an alternative, you could define the map as a dictionary, but the amount of code lines wouldn't decrease significantly - you may be able to cut the lines of code in half, but the degree of complexity would stay equivalent.

After months of frustration and of time spent in inserting needles in voodoo dolls of previous developers I decided that it is better try to refactor the legacy code.

I already ordered Micheal Feather's book, I am into Fowler's refactoring and I made some sample projects with DUnit.

So even if I don't master the subject I feel it is time to act and put some ideas into practice.

Almost 100% of the code I work on has the business logic trapped in the UI, moreover all is procedural programming (with some few exceptions). The application started as quick & dirty and continued as such.

Now writing tests for all the application is a meaningless task in my case, but I would like to try to unittest something that I need to refactor.

One of the complex tasks one big "TForm business logic class" does is to read DB data, make some computations and populate a scheduler component. I would like to remove the reading DB data and computation part and assign to a new class this task. Of course this is a way to improve the current design, it is not the best way for starting from scratch, but I'd like to do this because the data returned by this new class is useful also in other ways, for example now I've been ask to send e-mail notifications of scheduler data.

So to avoid a massive copy and paste operation I need the new class.

Now the scheduler is populated from a huge dataset (huge in size and in number of fields), probably a first refactoring step could be obtaining the dataset from the new class. But then in the future I'd better use a new class (like TSchedulerData or some other name less bound to scheduler) to manage the data, and instead of having a dataset as result i can have a TSchedulerData object.

Since refactor occurs at at small steps and tests are needed to refactor better I am a little confused on how to proceed.

The following points are not clear to me:

1) how to test a complex dataset? Should I run the working application, save one result set to xml, and write a test where I use a TClientDataSet containing that xml data?

2) How much do I have to care about TSchedulerData? I mean I am not 100% sure I will use TSchedulerData, may be I will stick with the Dataset, anyway thinking of creating complex tests that will be discarded in 2 weeks is not appealing for a DUnitNewbee. Anyway probably this is how it works. I can't imagine the number of bugs that I would face without a test.

Final note: I know someone thinks rewriting from scratch is a better option, but this is not an option. "The application is huge and it is sold today and new features are required today not to get out of business". This is what I have been told, anyway refactoring can save my life and extend the application life.

Your eventual goal is to separate the UI, data storage and business logic into distinct layers.

Its very difficult to test a UI with automatic testing frameworks. You'll want to eventually separate as much of the business logic from the UI as possible. This can be accomplished using one of the various Model/View/* patterns. I prefer MVP passive view, which attempts to make the UI nothing more than an interface. If you're using a Dataset MVP Supervising Controller may be a better fit.

Data storage needs to have its own suite of tests but these are different from unit tests (though you can use the same unit testing framework) and there are usually fewer of them. You can get away with this because most of the heavy lifting is being done by third party data components and a dbms (in your case T*Dataset). These are integration tests. Basically making sure your code plays nice with the vendor's code. Also needed if you have any stored procedures defined in the DB. They are much slower that unit tests and don't need to be run as often.

The business logic is what you want to test the most. Every calculation, loop or branch should have at least one test(more is preferable). In legacy code this logic often touches the UI and db directly and does multiple things in a single function. Here Extract Method is your friend. Good places to extract methods are:

for I:=0 to List.Count - 1 do
begin
  //HERE
end;

if /*HERE if its a complex condition*/ then
begin
  //HERE
end
else
begin
  //HERE
end

Answer := Var1 / Var2 + Var1 * Var3; //HERE

When you come across one of these extraction points

  1. Decide what you want the method signature to look like for your new method: Method name, parameters, return value.
  2. Write a test that calls it and checks the expected outcome.
  3. Extract the method.

If all goes well you will have a newly extracted method with at least one passing unit test.

Delphi's built in Extract Method doesn't give you any way to adjust the signature so if that's your own option you'll have to make do and fix it after extraction. You'll also want to make the new method public so your test can access it. Some people balk at making a private utility method public but at this early stage you have little choice. Once you've made sufficient progress you'll start to see that some utility methods you've extracted belong in their own class (in which case they'd have to be public anyway) while others can be made private/protected and tested indirectly by testing methods that depend on them.

As your test suite grows you'll want to run them after each change to ensure your latest change hasn't broken something elsewhere.

This topic is much too large to cover completely in an answer. You'll find the vast majority of your questions are covered when that book arrives.

I am gearing up for a rewrite of our company's software which was written over the past x years without use of any frameworks, conventions or even common sense. Result is a very vast, powerful application that is simply a nightmare to maintain, add to and fix.

Enough to say that this is a sort of ordering system on steroids that is supposed to be modular, versatile with nice reporting, very configurable (both interface and backend) etc.

As long as there is this requirement for modules, most of them are quite interconnected with the rest of the modules...

Now my question is - which methodology would be best to use?

I was leaning towards the 'conservative' approach of creating a grand plan, detailed specification, design a database, write migration scripts for old customers and then if all goes well sit down and start writing according to plan.

However some people pointed out that it is quite difficult to create a detailed and true plan for entire application (especially with such complexity) and then just do it. Some suggested Agile/iterative and incremental methodologies when I would go, pick a module, design database for it, write tests, create back-end and front-end and then get on a next module...

It all seems to be quite nice because we may have new 'ideas' as we write but wont that be more pain to then integrate modules between each other rather than have it all designed from the scratch?

If anyone has any experience with anything similar please give me few pointers to steer me in a right direction.

Thanks in advance!

Unless you've done this kind of refactoring a few times already, going with a plan that gives you intermediate points of completion and points to reconsider if you are doing the right thing, yes, iterative sounds much, much better. And even if you have, there are compelling arguments against starting over. Joel Spolsky has an article on the idea of restarting from scratch. I don't think I have much to add to that.

Martin Fowler has written a good book on Refactoring, well worth checking out for this kind of work.

I'm going to create the comparison table of existing automated C++ refactoring tools as well as explore an ability of creation of such tool, free and open-source.

My question is: what refactorings do you really use in your everyday work? There are obvious things like renaming variable/class/method, but is there something specific for C++, like dealing with templates, STL, copy constructors, initializers, etc., etc?

I'm interested in building of realistic picture of all that little problems that C++ developer is facing each day in his coding and that could be automated at least in theory. I'm talking to my colleagues but that's probably not enough.

Thanks in advance.

Take a look in Martin Fowler's Refactoring: Improving the Design of Existing Code and Refactoring to Patterns by Joshua Kerievsky. These in turn reference the GoF Design Patterns book so get that too.

If you can beyond the basic Rename Feature and Extract Function then you might be onto a winner.

I'm reading a lot about good and bad practices in OOP design. It's nice to know your design is bad, or good. But how do you get from bad to good design? I've split the interface (xaml) and codebehind from the main businesslogic class. That last class is growing big. I've tried splitting it up into smaller classes, but I'm stuck now. Any ideas on how to split large classes? The main class has 1 list of data of different types. I'm doing calculations on the total, but also on the individual types. I've got methods to perform these calculations which are called from events handled in the codebehind. Any ideas where to go from here?

Additional Info:

We are already about 6 months into this project. I've worked with object oriented laguages for years (first c++, java and now c#), but never on a large project like this one. I believe we've made some wrong turns in the beginning and I think we need to correct these. I can't specify any details on this project at the moment. I'm going to order one or two books about design. If I separate all the classes, how do I stick them back together? Maybe it's even better to continue this way to the first release and rebuilt parts after that, for a second release?

I highly recommend picking up Code Complete. It's a great book that offers tons of good advice on questions like yours.

To give you a quick answer to your question about how to split large classes, here's a good rule of thumb: make your class responsible for one thing, and one thing only. When you start thinking like that, you quickly can identify code that doesn't belong. If something doesn't belong, factor it out into a new class, and use it from your original class.

Edit: Take that thinking down to the "method" level, too - make your methods responsible for one thing, and one thing only. Helps break large (>50 line) methods down very quickly into reusable chunks of code.

Refactoring by Martin Fowler is an excellent book about how to change the design of your software without breaking it.

Design Patterns works similarly to algorithims but tells you how to combine objects to perform various useful tasks.

Finally Martin Fowler has a variety of useful design pattern for applications. For example Passive View

Michael Feathers's "Working Effectively with Legacy Code" is supposed to be very good, but I'll confess that I haven't read it myself.

Same goes for "Refactoring to Patterns."

I was asked this question : I was not able to answer - Any answers here?

What can be said about a java class which has 1000 lines of code , with 1-n methods having 100 lines and n+1 to m methods having 200 lines of code?

I thought, Classes should be okay to have 1000 lines of code and methods are also okay to have 100-200 lines of code - So I didn't answer considering that the class is perfectly okay.

Are there known compile time performance related to the number of lines a java class has? or a java method has ? are there any standards - if so, how can one justify technically?

thanks!

I think that the main problem here is not the compile-time performance. A 1000 lines class is difficult to read and understand.

Perhaps the class should be decomposed in other classes (or subclasses, if in your case inheritance is more valuable than composition), so every class has a well defined responsibility in the system.

But I can't say very much about this problem if I can't view the class' implementation.

There are a lot of good books that explain how to solve this and others design problems. Two well known books are Code Complete 2nd Edition by Steve McConnell and Refactoring by Martin Fowler et al.

Man, I just had this project given to me - expand on this they say.

This is an example of ONE function:

<?php


//500+ lines of pure wonder.
function page_content_vc($content) {
    global $_DBH, $_TPL, $_SET;

 $_SET['ignoreTimezone'] = true;

    lu_CheckUpdateLogin();

    if($_SESSION['dash']['VC']['switch'] == 'unmanned' || $_SESSION['dash']['VC']['switch'] == 'touchscreen') {
        if($content['page_name'] != 'vc') {
            header('Location: /vc/');
            die();
        }
    }

    if($_GET['l']) {
        unset($_SESSION['dash']['VC']);
        if($loc_id = lu_GetFieldValue('ID', 'Location', $_GET['l'])) {

            if(lu_CheckPermissions('vc', $loc_id)) {

    $timezone = lu_GetFieldValue('Time Zone', 'Location', $loc_id, 'ID');
    if(strlen($timezone) > 0) {
     $_SESSION['time_zone'] = $timezone;
    }

                $_SESSION['dash']['VC']['loc_ID'] = $loc_id;
                header('Location: /vc/');
                die();
            }
        }
    }

    if($_SESSION['dash']['VC']['loc_ID']) {

  $timezone = lu_GetFieldValue('Time Zone', 'Location', $_SESSION['dash']['VC']['loc_ID'], 'ID');
  if(strlen($timezone) > 0) {
   $_SESSION['time_zone'] = $timezone;
  }

        $loc_id = $_SESSION['dash']['VC']['loc_ID'];
        $org_id = lu_GetFieldValue('record_ID', 'Location', $loc_id);

        $_TPL->assign('loc_id', $loc_id);

        $location_name = lu_GetFieldValue('Location Name', 'Location', $loc_id);
        $_TPL->assign('LocationName', $location_name);

        $customer_name = lu_GetFieldValue('Customer Name', 'Organisation', $org_id);
        $_TPL->assign('CustomerName', $customer_name);

        $enable_visitor_snap = lu_GetFieldValue('VisitorSnap', 'Location', $loc_id);
        $_TPL->assign('EnableVisitorSnap', $enable_visitor_snap);

  $lacps = explode("\n", lu_GetFieldValue('Location Access Control Point', 'Location', $loc_id));
        array_walk($lacps, 'trim_value');

  if(count($lacps) > 0) {
   if(count($lacps) == 1) {
    $_SESSION['dash']['VC']['lacp'] = $lacps[0];
   }
   else {
    if($_GET['changeLACP'] && in_array($_GET['changeLACP'], $lacps)) {
     $_SESSION['dash']['VC']['lacp'] = $_GET['changeLACP'];
     header('Location: /vc/');
     die();
    }
    else if(!in_array($_SESSION['dash']['VC']['lacp'], $lacps)) {
     $_SESSION['dash']['VC']['lacp'] = $lacps[0];
    }
    $_TPL->assign('LACP_array', $lacps);
   }
   $_TPL->assign('current_LACP', $_SESSION['dash']['VC']['lacp']);
   $_TPL->assign('showContractorSearch', true);
   /*
   if($contractorStaff = lu_GetTableRow('ContractorStaff', $org_id, 'record_ID', 'record_Inactive != "checked"')) {
    foreach($contractorStaff['rows'] as $contractor) {
     $lacp_rights = lu_OrganiseCustomDataFunctionMultiselect($contractor[lu_GetFieldName('Location Access Rights', 'ContractorStaff')]);
     if(in_array($_SESSION['dash']['VC']['lacp'], $lacp_rights)) {
      $_TPL->assign('showContractorSearch', true);
     }
    }
   }
   */
  }

  $selectedOptions = explode(',', lu_GetFieldValue('Included Fields', 'Location', $_SESSION['dash']['VC']['loc_ID']));
  $newOptions = array();
  foreach($selectedOptions as $selOption) {
   $so_array = explode('|', $selOption, 2);
   if(count($so_array) > 1) {
    $newOptions[$so_array[0]] = $so_array[1];
   }
   else {
    $newOptions[$so_array[0]] = "Both";
   }
  }  

  if($newOptions[lu_GetFieldName('Expected Length of Visit', 'Visitor')]) {

   $alert = false;

            if($visitors = lu_OrganiseVisitors(
                lu_GetTableRow('Visitor', 'checked',
                    lu_GetFieldName('Checked In', 'Visitor'),
                    lu_GetFieldName('Location for Visit', 'Visitor').'="'.$_SESSION['dash']['VC']['loc_ID'].'"
                    AND '.lu_GetFieldName('Checked Out', 'Visitor').' != "checked"'), false, true, true)) {

                foreach($visitors['rows'] as $key => $visitor) {
     if($visitor['expected'] && $visitor['expected'] + (60*30) < time()) {
      $alert = true;
     }
                }
            }
   if($alert == true) {
    $_TPL->assign('showAlert', 'red');
   }
   else {
    //$_TPL->assign('showAlert', 'green');
   }
  }


        $_TPL->assign('switch', $_SESSION['dash']['VC']['switch']);
  if($_SESSION['dash']['VC']['switch'] == 'touchscreen') {
   $_TPL->assign('VC_unmanned', true);
  }

        if($_GET['check'] == 'in') {
   if($_SESSION['dash']['VC']['switch'] == 'touchscreen') {
    lu_CheckInTouchScreen();
            }
   else {
    lu_CheckIn();
   }
        }
        else if($_GET['check'] == 'out') {
   if($_SESSION['dash']['VC']['switch'] == 'touchscreen') {
    lu_CheckOutTouchScreen();
            }
   else {
    lu_CheckOut();
   }
        }
        else if($_GET['switch'] == 'unmanned') {
            $_SESSION['dash']['VC']['switch'] = 'unmanned';
            if($_GET['printing'] == true && (lu_GetFieldValue('Printing', 'Location', $_SESSION['dash']['VC']['loc_ID']) != "No" && lu_GetFieldValue('Printing', 'Location', $_SESSION['dash']['VC']['loc_ID']) != "")) {
                $_SESSION['dash']['VC']['printing'] = true;
            }
            else {
                $_SESSION['dash']['VC']['printing'] = false;
            }
            header('Location: /vc/');
            die();
        }
        else if($_GET['switch'] == 'touchscreen') {
            $_SESSION['dash']['VC']['switch'] = 'touchscreen';
            if($_GET['printing'] == true && (lu_GetFieldValue('Printing', 'Location', $_SESSION['dash']['VC']['loc_ID']) != "No" && lu_GetFieldValue('Printing', 'Location', $_SESSION['dash']['VC']['loc_ID']) != "")) {
                $_SESSION['dash']['VC']['printing'] = true;
            }
            else {
                $_SESSION['dash']['VC']['printing'] = false;
            }
            header('Location: /vc/');
            die();
        }

        else if($_GET['switch'] == 'manned') {
            if($_POST['password']) {
                if(md5($_POST['password']) == $_SESSION['dash']['password']) {
                    unset($_SESSION['dash']['VC']['switch']);
                    //setcookie('email', "", time() - 3600);
                    //setcookie('location', "", time() - 3600);
                    header('Location: /vc/');
                    die();
                }
                else {
                    $_TPL->assign('switchLoginError', 'Incorrect Password');
                }
            }

            $_TPL->assign('switchLogin', 'true');
        }
        else if($_GET['m'] == 'visitor') {
            lu_ModifyVisitorVC();
        }

        else if($_GET['m'] == 'enote') {
            lu_ModifyEnoteVC();
        }

        else if($_GET['m'] == 'medical') {
            lu_ModifyMedicalVC();
        }

        else if($_GET['print'] == 'label' && $_GET['v']) {
            lu_PrintLabelVC();
        }

        else {
            unset($_SESSION['dash']['VC']['checkin']);
            unset($_SESSION['dash']['VC']['checkout']);

            $_TPL->assign('icon', 'GroupCheckin');

            if($_SESSION['dash']['VC']['switch'] != 'unmanned' && $_SESSION['dash']['VC']['switch'] != 'touchscreen') {

                $staff_ids = array();
                if($staffs = lu_GetTableRow('Staff', $_SESSION['dash']['VC']['loc_ID'], 'record_ID')) {
                    foreach($staffs['rows'] as $staff) {
                        $staff_ids[] = $staff['ID'];
                    }
                }

                if($_GET['view'] == "tomorrow") {
                    $dateStart = date('Y-m-d', mktime(0, 0, 0, date("m")  , date("d")+1, date("Y")));
                    $dateEnd = date('Y-m-d', mktime(0, 0, 0, date("m")  , date("d")+1, date("Y")));
                }
                else if($_GET['view'] == "month") {
                    $dateStart = date('Y-m-d', mktime(0, 0, 0, date("m"), date("d"), date("Y")));
                    $dateEnd = date('Y-m-d', mktime(0, 0, 0, date("m"), date("d")+30, date("Y")));
                }
                else if($_GET['view'] == "week") {
                    $dateStart = date('Y-m-d', mktime(0, 0, 0, date("m"), date("d"), date("Y")));
                    $dateEnd = date('Y-m-d', mktime(0, 0, 0, date("m"), date("d")+7, date("Y")));
                }
                else {
                    $dateStart = date('Y-m-d');
                    $dateEnd = date('Y-m-d');
                }

    if(lu_GetFieldValue('Enable Survey', 'Location', $_SESSION['dash']['VC']['loc_ID']) == 'checked'
     && lu_GetFieldValue('Add Survey', 'Location', $_SESSION['dash']['VC']['loc_ID']) == 'checked') {
      $_TPL->assign('enableSurvey', true);
    }

                //lu_GetFieldName('Checked In', 'Visitor')
                //!= "checked"

                //date('d/m/Y'), lu_GetFieldName('Date of Visit', 'Visitor')

                if($visitors = lu_OrganiseVisitors(lu_GetTableRow('Visitor', $_SESSION['dash']['VC']['loc_ID'], lu_GetFieldName('Location for Visit', 'Visitor'), lu_GetFieldName('Checked In', 'Visitor').' != "checked" AND '.lu_GetFieldName('Checked Out', 'Visitor').' != "checked" AND '.lu_GetFieldName('Date of Visit', 'Visitor').' >= "'.$dateStart.'" AND '.lu_GetFieldName('Date of Visit', 'Visitor').' <= "'.$dateEnd.'"'))) {
                    foreach($visitors['days'] as $day => $visitors_day) {
                        foreach($visitors_day['rows'] as $key => $visitor) {
                            $visitors['days'][$day]['rows'][$key]['visiting'] = lu_GetTableRow('Staff', $visitor['record_ID'], 'ID');
                            $visitors['days'][$day]['rows'][$key]['visiting']['notify'] = $_DBH->getRow('SELECT * FROM lu_notification WHERE ent_ID = "'.$visitor['record_ID'].'"');
                        }
                    }

                    //array_dump($visitors);

                    $_TPL->assign('visitors', $visitors);
                }

    if($_GET['conGroup']) {
     if($_GET['action'] == 'add') {
      $_SESSION['dash']['VC']['conGroup'][$_GET['conGroup']] = $_GET['conGroup'];
     }
     else {
      unset($_SESSION['dash']['VC']['conGroup'][$_GET['conGroup']]);
     }
    }

    if(count($_SESSION['dash']['VC']['conGroup']) > 0) {
                    if($conGroupResult = lu_GetTableRow('ContractorStaff', '1', '1', ' ID IN ('.implode(',', $_SESSION['dash']['VC']['conGroup']).')')) {

      if($_POST['_submit'] == 'Check-In Group >>') {

                         $form = lu_GetForm('VisitorStandard');
       $standarddata = array();

       foreach($form['items'] as $key=>$item) {
        $standarddata[$key] = $_POST[lu_GetFieldName($item['name'], 'Visitor')];
       }

       foreach($conGroupResult['rows'] as $conStaff) {
        $data = $standarddata;

        foreach($form['items'] as $key=>$item) {
         if($key != 'ID' && $key != 'record_ID' && $conStaff[lu_GetFieldName(lu_GetNameField($key, 'Visitor'), 'ContractorStaff')]) {
          $data[$key] = $conStaff[lu_GetFieldName(lu_GetNameField($key, 'Visitor'), 'ContractorStaff')];
         }
        }

        $data['record_ID'] = $data[lu_GetFieldName('Visiting', 'Visitor')];
                          $data[lu_GetFieldName('Date of Visit', 'Visitor')] = date('Y-m-d');
                          $data[lu_GetFieldName('Time of Visit', 'Visitor')] = date('H:i');
                          $data[lu_GetFieldName('Checked In', 'Visitor')] = 'checked';
                          $data[lu_GetFieldName('Location for Visit', 'Visitor')] = $_SESSION['dash']['VC']['loc_ID'];
                          $data[lu_GetFieldName('ConStaff ID', 'Visitor')] = $conStaff['ID'];
        $data[lu_GetFieldName('From', 'Visitor')] = lu_GetFieldValue('Legal Name', 'Contractor', $conStaff[lu_GetFieldName('Contractor', 'ContractorStaff')]);

                          $id = lu_UpdateData($form, $data);
                          lu_VisitorCheckIn($id);

        //array_dump($data);
        //array_dump($id);
       }


       unset($_SESSION['dash']['VC']['conGroup']);
       header('Location: /vc/');
       die();
      }

      if(count($conGroupResult['rows'])) {
                         foreach($conGroupResult['rows'] as $key => $cstaff) {
                             $conGroupResult['rows'][$key]['contractor'] = lu_GetTableRow('Contractor', $cstaff[lu_GetFieldName('Contractor', 'ContractorStaff')], 'ID');
                         }
                         $_TPL->assign('conGroupResult', $conGroupResult);
      }

      $conGroupForm = lu_GetForm('VisitorConGroup');
      $conGroupForm = lu_OrganiseVisitorForm($conGroupForm, $_SESSION['dash']['VC']['loc_ID'], 'Contractor');

            $secure_options_array = lu_GetSecureOptions($org_id);
            if($secure_options_array[$_SESSION['dash']['VC']['loc_ID']]) {
             $conGroupForm['items'][lu_GetFieldName('Secure Area', 'Visitor')]['options']['values'] = $secure_options_array[$_SESSION['dash']['VC']['loc_ID']];
       $conGroupForm['items'][lu_GetFieldName('Secure Area', 'Visitor')]['name'] = 'Secure  Area';
      }
      else {
                unset($conGroupForm['items'][lu_GetFieldName('Secure Area', 'Visitor')]);
      }

         if($secure_options_array) {
             $form['items'][lu_GetFieldName('Secure Area', 'Visitor')]['options']['values'] = $secure_options_array;
       $form['items'][lu_GetFieldName('Secure Area', 'Visitor')]['name'] = 'Secure  Area';
         }
         else {
             unset($form['items'][lu_GetFieldName('Secure Area', 'Visitor')]);
         }

         $_TPL->assign('conGroupForm', $conGroupForm);
      $_TPL->assign('hideFormCancel', true);
     }
    }

                if($_GET['searchVisitors']) {
                    $_TPL->assign('searchVisitorsQuery', $_GET['searchVisitors']);
                    $where = '';

                    if($_GET['searchVisitorsIn'] == 'Yes') {
                        $where .= ' AND '.lu_GetFieldName('Checked In', 'Visitor').' = "checked"';
                        $_TPL->assign('searchVisitorsIn', 'Yes');
                    }
                    else {
                        $where .= ' AND '.lu_GetFieldName('Checked In', 'Visitor').' != "checked"';
                        $_TPL->assign('searchVisitorsIn', 'No');
                    }

                    if($_GET['searchVisitorsOut'] == 'Yes') {
                        $where = '';
                        $where .= ' AND '.lu_GetFieldName('Checked Out', 'Visitor').' = "checked"';
                        $_TPL->assign('searchVisitorsOut', 'Yes');
                    }
                    else {
                        $where .= ' AND '.lu_GetFieldName('Checked Out', 'Visitor').' != "checked"';
                        $_TPL->assign('searchVisitorsOut', 'No');
                    }

                    if($searchVisitors = lu_OrganiseVisitors(lu_GetTableRow('Visitor', $_GET['searchVisitors'], '#search#', lu_GetFieldName('Location for Visit', 'Visitor').'="'.$_SESSION['dash']['VC']['loc_ID'].'"'.$where))) {
                        foreach($searchVisitors['rows'] as $key => $visitor) {
                            $searchVisitors['rows'][$key]['visiting'] = lu_GetTableRow('Staff', $visitor['record_ID'], 'ID');
                        }

                        $_TPL->assign('searchVisitors', $searchVisitors);
                    }
                    else {
                        $_TPL->assign('searchVisitorsNotFound', true);
                    }
                }
                else if($_GET['searchStaff']) {

                    if($_POST['staff_id']) {
                        if(lu_CheckPermissions('staff', $_POST['staff_id'])) {
                            $_DBH->query('UPDATE '.lu_GetTableName('Staff').' SET '.lu_GetFieldName('Current Location', 'Staff').' = "'.$_POST['current_location'].'" WHERE ID="'.$_POST['staff_id'].'"');
                        }
                    }


                    $locations = lu_GetTableRow('Location', $org_id, 'record_ID');

                    if(count($locations['rows']) > 1) {
                        $_TPL->assign('staffLocations', $locations);
                    }

                    $loc_ids = array();
                    foreach($locations['rows'] as $location) {
                        $loc_ids[] = $location['ID'];
                    }

                    // array_dump($locations);
                    // array_dump($_POST);

                    $_TPL->assign('searchStaffQuery', $_GET['searchStaff']);
                    $where = ' AND record_Inactive != "checked"';

                    if($_GET['searchStaffIn'] == 'Yes' && $_GET['searchStaffOut'] != 'Yes') {
                        $where .= ' AND ('.lu_GetFieldName('Staff Status', 'Staff').' = "" OR '.lu_GetFieldName('Staff Status', 'Staff').' = "On-Site")'.

                        $_TPL->assign('searchStaffIn', 'Yes');
                        $_TPL->assign('searchStaffOut', 'No');
                    }
                    else if($_GET['searchStaffOut'] == 'Yes' && $_GET['searchStaffIn'] != 'Yes') {
                        $where .= ' AND ('.lu_GetFieldName('Staff Status', 'Staff').' != "" AND '.lu_GetFieldName('Staff Status', 'Staff').' != "On-Site")'.
                        $_TPL->assign('searchStaffOut', 'Yes');
                        $_TPL->assign('searchStaffIn', 'No');
                    }
                    else {
                        $_TPL->assign('searchStaffOut', 'Yes');
                        $_TPL->assign('searchStaffIn', 'Yes');
                    }

                    if($searchStaffs = lu_GetTableRow('Staff', $_GET['searchStaff'], '#search#', 'record_ID IN ('.implode(',', $loc_ids).')'.$where, lu_GetFieldName('First Name', 'Staff').','.lu_GetFieldName('Surname', 'Staff'))) {
                        $_TPL->assign('searchStaffs', $searchStaffs);
                    }
                    else {
                        $_TPL->assign('searchStaffNotFound', true);
                    }
                }
    else if($_GET['searchContractor']) {

                    $_TPL->assign('searchContractorQuery', $_GET['searchContractor']);
                    //$where = ' AND '.lu_GetTableName('ContractorStaff').'.record_Inactive != "checked"';
     $where = ' ';

                    if($_GET['searchContractorIn'] == 'Yes' && $_GET['searchContractorOut'] != 'Yes') {
                        $where .= ' AND ('.lu_GetFieldName('Onsite Status', 'ContractorStaff').' = "Onsite")';

                        $_TPL->assign('searchContractorIn', 'Yes');
                        $_TPL->assign('searchContractorOut', 'No');
                    }
                    else if($_GET['searchContractorOut'] == 'Yes' && $_GET['searchContractorIn'] != 'Yes') {
                        $where .= ' AND ('.lu_GetFieldName('Onsite Status', 'ContractorStaff').' != "Onsite")'.
                        $_TPL->assign('searchContractorOut', 'Yes');
                        $_TPL->assign('searchContractorIn', 'No');
                    }
                    else {
                        $_TPL->assign('searchContractorOut', 'Yes');
                        $_TPL->assign('searchContractorIn', 'Yes');
                    }

     $join = 'LEFT JOIN '.lu_GetTableName('Contractor').' ON '.lu_GetTableName('Contractor').'.ID = '.lu_GetTableName('ContractorStaff').'.'.lu_GetFieldName('Contractor', 'ContractorStaff');

     $extrasearch = array (
      lu_GetTableName('Contractor').'.'.lu_GetFieldName('Legal Name', 'Contractor')
     );

                    if($searchContractorResult = lu_GetTableRow('ContractorStaff', $_GET['searchContractor'], '#search#', lu_GetTableName('ContractorStaff').'.record_ID = "'.$org_id.'" '.$where, lu_GetFieldName('First Name', 'ContractorStaff').','.lu_GetFieldName('Surname', 'ContractorStaff'), $join, $extrasearch)) {

      /*
      foreach($searchContractorResult['rows'] as $key=>$contractor) {
       $lacp_rights = lu_OrganiseCustomDataFunctionMultiselect($contractor[lu_GetFieldName('Location Access Rights', 'ContractorStaff')]);
       if(!in_array($_SESSION['dash']['VC']['lacp'], $lacp_rights)) {
        unset($searchContractorResult['rows'][$key]);
       }
      }
      */

      if(count($searchContractorResult['rows'])) {
                         foreach($searchContractorResult['rows'] as $key => $cstaff) {
        /*
        if($cstaff[lu_GetFieldName('Onsite_Status', 'Contractor')] == 'Onsite')) {
         if($visitor['rows'][0][lu_GetFieldName('ConStaff ID', 'Visitor')]) {
                   $_DBH->query('UPDATE '.lu_GetTableName('ContractorStaff').' SET '.lu_GetFieldName('Onsite Status', 'ContractorStaff').' = "" WHERE ID="'.$visitor['rows'][0][lu_GetFieldName('ConStaff ID', 'Visitor')].'"');
         }
        }
        */
        if($cstaff[lu_GetFieldName('SACN Expiry Date', 'ContractorStaff')] != '0000-00-00') {
         if(strtotime($cstaff[lu_GetFieldName('SACN Expiry Date', 'ContractorStaff')]) < time()) {
          $searchContractorResult['rows'][$key]['sacn_expiry'] = true;
         }
         else {
          $searchContractorResult['rows'][$key]['sacn_expiry'] = false;
         }
        }
        else {
         $searchContractorResult['rows'][$key]['sacn_expiry'] = false;
        }

        if($cstaff[lu_GetFieldName('Induction Valid Until', 'ContractorStaff')] != '0000-00-00') {
         if(strtotime($cstaff[lu_GetFieldName('Induction Valid Until', 'ContractorStaff')]) < time()) {
          $searchContractorResult['rows'][$key]['induction_expiry'] = true;
         }
         else {
          $searchContractorResult['rows'][$key]['induction_expiry'] = false;
         }
        }
        else {
         $searchContractorResult['rows'][$key]['induction_expiry'] = false;
        }

                             $searchContractorResult['rows'][$key]['contractor'] = lu_GetTableRow('Contractor', $cstaff[lu_GetFieldName('Contractor', 'ContractorStaff')], 'ID');
                         }
                         $_TPL->assign('searchContractorResult', $searchContractorResult);
      }
      else {
                         $_TPL->assign('searchContractorNotFound', true);
      }
                    }
                    else {
                        $_TPL->assign('searchContractorNotFound', true);
                    }
    }

                $occupancy = array();
                $occupancy['staffNumber'] = $_DBH->getOne('SELECT count(*) FROM '.lu_GetTableName('Staff').' WHERE record_ID = "'.$_SESSION['dash']['VC']['loc_ID'].'" AND record_Inactive != "checked" AND '.lu_GetFieldName('Ignore Counts', 'Staff').' != "checked"');
                $occupancy['staffNumberOnsite']= $_DBH->getOne(
                        'SELECT count(*) FROM '.lu_GetTableName('Staff').' WHERE
                        (
                            (record_ID = "'.$_SESSION['dash']['VC']['loc_ID'].'"
                                AND
                                ('.lu_GetFieldName('Staff Status', 'Staff').' = "" OR  '.lu_GetFieldName('Staff Status', 'Staff').' = "On-Site"))
                                OR '.lu_GetFieldName('Current Location', 'Staff').' = "'.$_SESSION['dash']['VC']['loc_ID'].'")
                                AND record_Inactive != "checked"
        AND '.lu_GetFieldName('Ignore Counts', 'Staff').' != "checked"');

                $occupancy['visitorsOnsite'] = $_DBH->getOne('SELECT count(*) FROM '.lu_GetTableName('Visitor').' WHERE '.lu_GetFieldName('Location for Visit', 'Visitor').' = "'.$_SESSION['dash']['VC']['loc_ID'].'" AND '.lu_GetFieldName('Checked In', 'Visitor').' = "checked" AND '.lu_GetFieldName('Checked Out', 'Visitor').' != "checked"');
                $_TPL->assign('occupancy', $occupancy);

                if($enotes = lu_GetTableRow('Enote', $org_id, 'record_ID', lu_GetFieldName('Note Emailed', 'Enote').' = "0000-00-00" AND '.lu_GetFieldName('Note Passed On', 'Enote').' != "Yes"')) {
                    $_TPL->assign('EnoteNotice', true);
                }

                if($medical = lu_GetTableRow('MedicalRoom', $_SESSION['dash']['VC']['loc_ID'], 'record_ID', 'record_Inactive != "Yes"')) {
                    $_TPL->assign('MedicalNotice', true);
                }

                if(lu_GetFieldValue('Printing', 'Location', $_SESSION['dash']['VC']['loc_ID']) != "No" && lu_GetFieldValue('Printing', 'Location', $_SESSION['dash']['VC']['loc_ID']) != "") {
                    $_TPL->assign('UnmannedPrinting', true);
                }
            }
            else {
                if($_SESSION['dash']['VC']['printing'] == true) {
                    $_TPL->assign('UnmannedPrinting', true);
                }
            }

   // enable if contractor check-in buttons should be enabled
            if(lu_GetFieldValue('Enable Contractor Check In', 'Location', $_SESSION['dash']['VC']['loc_ID']) == "checked") {
                $_TPL->assign('ContractorCheckin', true);
            }

        }


  if($_SESSION['dash']['entity_id'] && $_GET['fixupCon'] == 'true') {
         $conStaffs = lu_GetTableRow('ContractorStaff', $_SESSION['dash']['ModifyConStaffs']['org_ID'], 'record_ID', '', lu_GetFieldName('First Name', 'ContractorStaff').','.lu_GetFieldName('Surname', 'ContractorStaff'));
      foreach($conStaffs['rows'] as $key => $cstaff) {
    if($cstaff[lu_GetFieldName('Site Access Card Number', 'ContractorStaff')] && $cstaff[lu_GetFieldName('Site Access Card Type', 'ContractorStaff')]) {
     echo $cstaff['ID'].' ';
     $_DBH->query('UPDATE '.lu_GetTableName('Visitor').' SET '.lu_GetFieldName('Site Access Card Number', 'Visitor').' = "'.$cstaff[lu_GetFieldName('Site Access Card Number', 'ContractorStaff')].'", '.lu_GetFieldName('Site Access Card Type', 'Visitor').' = "'.$cstaff[lu_GetFieldName('Site Access Card Type', 'ContractorStaff')].'" WHERE '.lu_GetFieldName('ConStaff ID', 'Visitor').'="'.$cstaff['ID'].'"');
    }
      }
  }

    }
    else {
        if($_SESSION['dash']['staffs']) {
            foreach($_SESSION['dash']['staffs']['rows'] as $staff) {
                if($staff[lu_GetFieldName('Reception Manager', 'Staff')] == 'checked') {
                    $loc_id = $staff['record_ID'];
                    unset($_SESSION['dash']['VC']);
                    if($loc_id = lu_GetFieldValue('ID', 'Location', $loc_id)) {
                        $_SESSION['dash']['VC']['loc_ID'] = $loc_id;
                        header('Location: /vc/');
                        die();
                    }
                }
            }
        }

        $_TPL->assign('mode', 'public');
    }

    $content['page_content'] = $_TPL->fetch('modules/vc.htm');
    return $content;
}


?>

This question will probably be closed - I just need some support from my coding brothers and sisters. *SOB*

Believe me, I've seen much worse code. As the other guys said, apply some refactoring.

My usual plan of attack:

  • If code is not tightly coupled to everything else in the system, write a lot unit tests. Those will help you make sure the behaviour of refactored code has not changed.
  • Extract as many functions as you can, dividing by logical operations. Even if that means some function will contain two lines of code.
  • Inline some of the 200 methods you created :)

Recommended read:

alt text

what I mean by that is: I basically have a class that has too many properties and functions now. To remain performant and understandable, it needs to shrink somehow. But I still need all those properties and methods somewhere. It's like this right now:

class Apple

  float seedCount;
  ...
  ...about 25 variables and properties here.
  void Update() <-- a huge method that checks for each property and updates if so

In most cases the class needs almost none of those properties. In some cases in needs to be able to grow very selectively and gain a feature or lose a feature. The only solution I have come up with, is that I create a bunch of classes and place some properties in there. I only initialize this classes object when one of those properties is needed, otherwise it remains null.

class Apple

  Seed seed;

Many problems because of that: I constantly have to check for every single object and feature each frame. If the seed is not initialized I don't have to calculate anything for it. If it is, I have to. If I decided to put more than 1 property/feature into the Seed class, I need to check every single one of those aswell. It just gets more and more complicated. The problem I have is therefore, that I need granular control over all the features and can't split them intelligently into larger subclasses. Any form of subclass would just contain a bunch of properties that need to be checked and updated if wanted. I can't exactly create subclasses of Apple, because of the need for such high granular control. It would be madness to create as many classes as there are combinations of properties. My main goal: I want short code.

Maybe your methods are not were they are supposed to be?

If you separated the Seed class from the Apple class, why don't you move the methods that use the Seed information to the Seed class too?

If those methods need information on other Apple properties, you can pass it as a parameter.

By doing this, I guess you can eliminate the initialization checks...

This is a great book about how to solve this kind of problem:

Refactoring

Possible Duplicate:
When is it good (if ever) to scrap production code and start over?

Say you have been given a project, and you look at the code. Although it works, and is functional, you realize that to make future changes it would be easier to rewrite a large portion or all of the code. Is it better to do the rewrite? If it costs you a delay now, but saves you delays later (and possible bug fixes later), is it worth it?

Or do you simply fix what you see as you go along and are touching that part of the code?

Or do you fix it only if a bug is reported that would require touching that code?

This is a complicated issue many of us face, me included. Consider reading a good book on the subject, because this is a huge topic. How much time/money do you have, and do you really understand the scope? If (1) you have enough time/money, and (2) you're sure the solution will be active long enough to make back the investment, and (3) you are certain you understand the scope of a total revamp, then the total revamp may well be better. But that is a lot of "ifs".

Because of time/money constraints and uncertainty concerning how long the solution will be relevant, piecemeal refactoring is more often the better choice (meaning, lower risk). How agile are you? If your flexibility and designing-on-the-fly skills are your strong point, and your client/boss supports you in this, then gradual refactoring will almost always be the better choice.

  • Use the same balancing skills you use when evaluating how exactly to write new code.
  • Compare time spent now against maintainability/testability/simplicity later.
  • Refactor the code most likely to need to be debugged/fixed/reconfigured in the future.
  • Don't bite off more than you can chew; usually there isn't a lot of time available for refactoring.
  • Unit tests are your best friend when refactoring.

And again, don't try to do too much! When your client/boss asks why features aren't being delivered, it doesn't sound good to say "well, I started refactoring a bunch of stuff, and it caused a lot of bugs, and I'm still trying to fix those." Better to get just one key part of your code in good shape.

This is what refactoring is all about. Changing the design of existing code without changing its functionality. You say you "realize" that it would be easier to rewrite, but that entails a certain presumption: that you know what will be required of the code in the future, that you know how much work rewriting it would be, and that you know how much work reworking it would be. I haven't known any programmers for whom that is true (myself very much included).

Refactoring - versus rewriting - offers some protection against this presumption. Done well, you can work only on what you need today, design the code you have so it can serve the immediate need you have, then modify it to serve that need. Bit by bit, your code gets cleaner and more malleable. And the bits that change are the ones that need changing - without making impossible predictions about the future. If you haven't read Martin Fowler's Refactoring, have a look at it; also Michael Feathers' Working Effectively with Legacy Code.

Currently I have started on very large Legacy Project and am in a situation which I always feared Reading and Understanding Other People's code, I have known that this is an essential skill which is required but haven't developed it as till date it was not required and now its like necessity to develop this skill rather than hobby and so I would like to know from SO Readers about:

  1. How you have overcome the hurdle of reading other people's code ?
  2. What techniques or skill have you developed to polish your art of reading and understanding other people code ?
  3. Are there any books or articles which you have referred to or in general how did you developed the skill of reading and understanding other people's code ?

I would highly appreciate useful answers to this questions as now I can understand how one would feel while trying to understand my code.

Patience: Understand that reading code is more difficult than writing new code. Then you need to respect the code, even if it is not very readable, for it does its job and in many cases pretty efficiently. You need to give the code time and effort to understand it.

Understand the Architecture: It is best if there is any documentation on this. Try talking to people who know more about it if they are available.

Test it: You need to spend some time testing and debugging the code so you know what it does. For those parts you understand, write some unit tests if possible so you can use them later.

Be Unassuming: Many times the names of the patterns are misused. The classes have names which do not indicate their purpose. So don't assume anything about them.

Learn Refactoring: The best book I found on this topic is Refactoring: Improving the Design of Existing Code - By Martin Fowler. Working Effectively with Legacy Code is another awesome one.

Michael Feathers' Working Effectively with Legacy Code is a great resource that contains a large number of techniques for working with older code.

I am looking to run a load of automated functionality tests on a user interface application of mine and was wondering what is the best software out there to carry out these tests. Preferably the software will be able to intergrate with Visuall C++ 2005. I have googled various software however there is so much out there I'm not sure what is best for what I need. Any help would be awesome, thanks.

for automated software unit tests I would recommend google test. There is a very good q&a on this platform, which you can find here.

Additionally, there is CPPUnitLite, which is developed by the author of "Working Effectively with Legacy Code", Michael Feathers.

I used AutoIt Scripts for testing a MFC application just a little bit, but it was not that easy to maintain them properly and build an effective logging system for failed tests.

However, the unit tests depend heavily on the architecture of your program and the structure of your class - especially the dependencies to other components / classes. So if you already have an existing MFC application, which was not built with unit tests in mind, you probably have to refactor a lot of things. Therefore, I would recommend the mentioned book. You can also use the classic "Refactoring" by Martin Fowler.

I'm trying to see if I understand depedency injection.

I have a project that is used as a parser. It can parse delimited text, key-value and will also regex.

The first way this was done was in one function with a switch. The next way was to put it in seperate functions and call it based on a switch

The next way I was thinking was to create an interface and than implement a class for each type. Is that to much work? Does the question come down to function or will doing this show benefits that I don't see yet.

I believe my problems stems that I was initially going to implement an interface and than for each time I needed a different parsing implement a new class. But than that would still require me going in and adding that to some type of logic flow as I don't see how to do that with injection frameworks.

So say I add in another way to parse that is based on tags or xml. Create the class implementing the interfaces and than I would need to add them to the flow logic to instantiate them as that interface if a user chooses to parse that type of text. Any clearner way to do that?

What you are describing is only remotely connected to Dependency Injection. It is, however, a poster example of the Replace Conditional with Polymorphism Refactoring (p. 255), so it's definitely a good idea.

It only becomes DI if you decide to completely dispense with the conditional and instead inject the Strategy into the consumer.

In my rails app, I have a method like this:

def cart
    if user_signed_in?
        @user = current_user
        if @user.cart.present?
            @cart = @user.cart
        else
            @cart = false
        end
    else
        cart_id = session[:cart_id]

        if cart_id.present?
            @cart = Cart.find(cart_id.to_i)
        else
            @cart = false
        end
    end
end

Rubocop flagged this method as Method had too many lines. Why is it bad to write a method with too many lines? What if we have to do a lot of work in it? How can I re-factor this and write good code?

One way is you that you can refactor it using ternary operator, but at the cost of readability.

def cart
  if user_signed_in?
    @user = current_user
    @cart = @user.cart.present? ? @user.cart : false
  else
    cart_id = session[:cart_id]
    @cart = cart_id.present? ? Cart.find(cart_id.to_i) : false
  end
end

Secondly, if you are compelled to write a very long method, it means there is something wrong with your Object Oriented Design. Maybe, you need to design a new class for the extra functionality, or you need to split the functionality in the same class by writing multiple methods that when combined, do the job of a single long method.

Why is it bad to write a method with too many lines?

Just like an essay with big paragraphs is harder to read, likewise a program with longer methods is difficult to read, and less likely to re-usable. The more chunks you divide your code in, the more moduler, re-usable and understandable your code will be.

What if we have to do a lot of work in it?

If you have to do a lot of work in it; it surely means that you have designed your classes in a way that isn't good. Perhaps, you need to design a new class to handle this functionality, or you need to split up this method into smaller chunks.

How can I re-factor this and write good code?

For that, I highly recommend to you a great book named: Refactoring by Martin Fowler, he is incredibly amazing at explaining how, when, and why to refactor your code.

I was using .Net Reflector on an Internal App to try and understand what the previous Dev was doing and also to learn. I have never had actual instruction on how to develop Apps so I take from where I can (Hooray Stack Overflow). That being said I found something that has me confused. A class Library called WinConstant containing the below code.

Here are my actual question:

  1. What possible use could this be?

  2. What value is there in storing a bunch of constant's in a class library?

  3. Is this considered a "Best Practice"?

Thoughts and guidance appreciated!


Public Class clsConstant
    Public Const cAccess As String = "Access"
    Public Const cAddress As String = "Address"
    Public Const cCancel As String = "Cancel"
    Public Const cCity As String = "City"
    Public Const cClear As String = "Clear"
    Public Const cClickOnMessage As String = "Click on any row in top pane to see the detail fields in the bottom pane."
    Public Const cClientID As String = "ClientID"
    Public Const cColon As String = ": "
    Public Const cComma As String = ","
    Public Const cContactID As String = "ContactID"
    Public Const cCounty As String = "County"
    Public Const cDash As String = "-"
    Public Const cDelete As String = "Delete"
    Public Const cDepartment As String = "Department"
    Public Const cError As String = "Error"
    Public Const cExec As String = "Exec"
    Public Const cFalse As String = "False"
    Public Const cFavorite As String = "Favorite"
    Public Const cFederal As String = "Federal"
    Public Const cFriday As String = "Friday"
    Public Const cfrmMain As String = "frmMain"
    Public Const cfrmModuleLogin As String = "frmModuleLogin"
    Public Const cfrmModuleSplash As String = "frmModuleSplash"
    Public Const cHelp As String = "Help"
    Public Const cHint As String = "Hint"
    Public Const cImagePath As String = "../../image"
    Public Const cIn As String = "In"
    Public Const cInformation As String = "Information"
    Public Const cInitialScreenID As String = "InitialScreenID"
    Public Const cInsert As String = "Insert"
    Public Const cJuvenileID As String = "JuvenileID"
    Public Const cLetter As String = "Letter"
    Public Const cManual As String = "Manual"
    Public Const cMasterID As String = "MasterID"
    Public Const cModuleID As String = "ModuleID"
    Public Const cModuleName As String = "ModuleName"
    Public Const cMonday As String = "Monday"
    Public Const cName As String = "Name"
    Public Const cNegative As String = "Negative"
     _
    Public Shared ReadOnly cNLowDate As DateTime = New DateTime(&H851055320574000)
     _
    Public Shared ReadOnly cNullDate As DateTime = New DateTime
    Public Const cNullDateString As String = "12:00:00 AM"
    Public Const cOfficeIDDefault As String = "01"
    Public Const cOne As Integer = 1
    Public Const cOut As String = "Out"
    Public Const cPopUp As String = "PopUp"
    Public Const cPositive As String = "Positive"
    Public Const cProcess As String = "Process"
    Public Const cProviderID As String = "ProviderID"
    Public Const cQuestion As String = "Question"
    Public Const cRead As String = "Read"
    Public Const cReferralID As String = "ReferralID"
    Public Const cReminder As String = "Reminder"
    Public Const cReport As String = "Report"
    Public Const cReportEngine As String = "ReportEngine"
    Public Const cReportEnginePath As String = "ReportEnginePath"
    Public Const cReportingServices As String = "ReportingServices"
    Public Const cReportServer As String = "ReportServer"
    Public Const cReportService As String = "ReportService"
    Public Const cReportServiceLocal As String = "ReportServiceLocal"
    Public Const cReportServiceServer As String = "ReportServiceServer"
    Public Const cSaturday As String = "Saturday"
    Public Const cSearch As String = "Search"
    Public Const cSelect As String = "Select"
    Public Const cSpace As String = " "
    Public Const cSQLLoginError As String = "SQL Server login/password invalid"
    Public Const cStart As String = "Select a module"
    Public Const cState As String = "State"
    Public Const cSubjectID As String = "SubjectID"
    Public Const cSunday As String = "Sunday"
    Public Const cThursday As String = "Thursday"
    Public Const cTooltipCancel As String = "Reset form data values back to before all manual changes."
    Public Const cTooltipClear As String = "Clears all data entry fields prior to an Insert"
    Public Const cTooltipClient As String = "Display a Client popup window."
    Public Const cTooltipClose As String = "Close this form"
    Public Const cTooltipDelete As String = "Delete the current record being displayed, no undo possible."
    Public Const cTooltipExe As String = "Initiate a batch process."
    Public Const cTooltipInsert As String = "Insert a brand new record"
    Public Const cTooltipSearch As String = "Perform a Search for values entered."
    Public Const cTooltipSelect As String = "Perform a Select for values entered."
    Public Const cTooltipUpdate As String = "Update an existing record"
    Public Const cTrue As String = "True"
    Public Const cTuesday As String = "Tuesday"
    Public Const cUnderscore As String = "____________________________________________________________"
    Public Const cUpdate As String = "Update"
    Public Const cWarning As String = "Warning"
    Public Const cWeb As String = "Web"
    Public Const cWednesday As String = "Wednesday"
    Public Const cWorkerID As String = "WorkerID"
    Public Const cZero As Integer = 0
    Public Shared strLongDate As String() = DateAndTime.Now.ToLongDateString.Split(New Char() { ","c })
    Public Shared strModuleMainStatusStripFormID As String = Nothing
End Class

Back in the days of coding windows applications in c, there were similar files #included in windows which consisted of long lists of #defines creating constants. Various c applications emulated this approach in their own files. The "class" seems to be a "transliteration" of this "c-ism". The fundamental principle of Object Oriented Design is to mix code and data into related functional units: objects. As jfullerton wrote:

From a programming point of view, object-orientation involves program objects, encapsulation, inheritance, and polymorphism. The conceptual objects are modeled in the program code. Encapsulation keeps an object's data and methods that use the data together as part of the object.

So clearly, this constant list does not conform to OO practices but is a throw back to the old days.

To answer your questions:

  1. -- This class holds constants, that is it
  2. -- The old developer probably did this because that was what he was used to doing
  3. -- It's not current best practices.

Naturally, if this is part of your application, you can't just throw this away. Rather, this is something to refactor over time, assuming you use current best practices of Test Driven Development and Refactoring

I was wondering, what technically would be the most efficient way.

What would be smarter to code with:

    if (args[0].toLowerCase().toString().equals("server")) {
        System.out.println("SERVER");
    }

Or:

    String host = args[0].toLowerCase();

    if (host.equals("server")) {
        System.out.println("SERVER");
    }

Keep in mind, I know that this variable is redundant. And that an argument can also contain a number, but that is not the case of my question. It's just a example.

When should you create a variable for an if statement? Always? Is it safer? Should I do it not, because I am wasting memory? Does it impact the performance at all?

By all means, create the variable. A single variable like that will never, ever be a performance bottleneck, never.

Always favor readability. Performance problems usually appear only in specifics parts of a system. When they arise, and not before measuring those (aka having numbers), you treat them case by case. Never forget: Premature optimization is the root of all evil.

In your specific case, you could go even further and create another variable, explaining the intent of the comparison:

String host = args[0].toLowerCase();
boolean isRunningUnderProduction = host.equals("server");
if (isRunningUnderProduction) {
    System.out.println("SERVER");
}

Much better than a comment. And explains the intent of the code within a glimpse.


A quote from Martin Fowler's Refactoring book:

The interesting thing about performance is that if you analyze most programs, you find that they waste most of their time in a small fraction of the code. If you optimize all the code equally, you end up with 90 percent of the optimizations wasted, because you are optimizing code that isn't run much. The time spent making the program fast, the time lost because of lack of clarity, is all wasted time.

In other words: if your programs are easier to read, they are easier to fix the slow parts, when the time comes (if it comes).

I would like to know about books that talk about design issues like when to use namespaces and other coding standards to write good quality efficient C++ code. One that talks about Code testing will also be appreciated.

"Large-Scale C++ Software Design" by John Lakos worked great for me years ago on how to organise code in large projects.

On testing, this is not my area, and I cannot recommend a great book. What I can do is discourage you from getting "Testing Computer Software", 2nd edition by Cem Kaner, Jack Falk and Hung Q. Nguyen. I found it severely dated and extremely clumsy. But please take this with a grain of salt.

For big projects, it is essential to follow a common design and coding style. Consistently.

I found the following book useful to have a common ground in a big project.

C++ Coding Standards: 101 Rules, Guidelines, and Best Practices by Andrei Alexandrescu, Herb Sutter

This question has in the back of my mind for some time, sorry if it appears subjective. There are some disadvantages in using bool in public properties and constructors for data objects. Consider the following code as an example.

Using bool:

public class Room
{
    public string Name { get; set; }
    public bool Bookable { get; set; }

    public Room(string name, bool bookable);
}

and the use of this class

Room r = new Room ("101", true);

This is suitably functional, there is however another way to implement it:

Using enum:

public enum BookingStatus
{
    Bookable,
    NotBookable
}

public class Room
{
    public string Name { get; set; }
    public BookingStatus Bookable { get; set; }

    public Room(string name, BookingStatus bookable);
}

and the use of this class

Room r = new Room ("101", BookingStatus.Bookable);

To me the two appear functionally equivalent, there are some advantages and disadvantages of each however:

  • When setting properties the Enum method is more verbose (you can infer the usage of the enum from the code alone)
  • Enumerations can be extended to support further states (particularly useful for an API)
  • Enumerations require considerably more typing (although reduces this vastly)
  • Enumerations can not be used in conditionals (i.e. if (r.bookable)), although I appreciate this is trivial to resolve.

Am I missing something, totally off the mark? I am not sure why this bugs me so much, perhaps I am too OCD for my own good!

In his book Refactoring, Martin Fowler explains why he thinks enums are a code smell, and I can only agree. In your example, a better approach would be to make an abstract Room class:

public abstract class Room
{
     public string Name { get; set; }

     public abstract bool Bookable { get; }
}

Then you can make derived BookableRoom and NonBookableRoom classes.

public class BookableRoom : Room
{
    public override bool Bookable
    {
        get { return true; }
    }
}

public class NonBookableRoom : Room
{
    public override bool Bookable
    {
        get { return false; }
    }
}

What tips/suggestions do you have for writing more understandable code?

I've been somewhat frustrated by the lack of structure and bad format of some code I've been maintaining lately and would like to propose a series of guidelines for writing more understandable code.

Any suggestion might help, no matter the language.

Regards.

The key to writing maintainable code is to follow some fundamental code design principles:

  1. Single Responsibility Principle - SRP - One class must implement one responsibility.
  2. DRY - almost reciprocal to SRP - Don't repeat Yourself - in other words don't let the same responsibility be implemented by multiple classes (for it would result in repetition of the same code)
  3. Make vertical slices of the application and call each slice a module. Come up with a modular structure with clear cut dependencies between modules. Publish a module structure for the project and enforce that the team complies to it. Obviously no cyclical dependencies. Use a tool like maven or apache ivy for dependency management during builds.
  4. Have an approach to implement non functional requirements as horizontal requirements using strategies such as AOP, decorators etc.

With these things in place, most of the code would become maintainable. Each of the points above is itself pretty involved. I absolutely love this book.

Look at blogs etc where these things are discussed. All the best

I liked these books:

You should also read code. If the code is hard to read, ask yourself what exactly the author did or didn't do that makes it hard to understand, and more importantly, how you can use what you've learned to write better code yourself.

Duplicate:

Learning implementing design patterns for newbies

I have been a developer for years and have my way of developing and have always kept up with the latest techologies. I want to start using a design pattern in the hope it will improve my development speed but I need to find one to apply and I need to find a full open source sample that demonstrates it.

I use and have an application that uses LINQ to SQL and .net 3.5 I tried to apply the repository pattern but found the structure complex and having to hack my way through it.

Any advice for someone who wants to better their programming style?

Two books that I would suggest reading are:

Refactoring: Improving the Design of Existing Code (ISBN: 0-201-48567-2) and Refactoring To Patterns (ISBN: 0-321-21335-1)

Both are great books that will help you, at a high level, understand the when's and why's to applying patterns to your code. In addition, they are great reference material for some of the most commonly used patterns out there.

To be clear, these books are by no means the "complete library" of design patterns.

Currently, I have a project, where I have to add some features on to it, but the coding hasn't maintained any standards and it is extremely tough to break it down into manageable and understandable parts, to get started. And there is no documentation, to help out?

How would you start such project, if you had to?

Steps:

  1. (Optional) Cover system with hi-level automatic tests, which test system through UI example
  2. Carefully refactor to testable code
  3. Cover functionality with unit tests
  4. Refactor code to make implementation of new features possible
  5. Implement new features

Manuals:

  1. Book by Michael Feathers: Working Effectively With Legacy Code
  2. Book by Martin Fowler: Refactoring

I have a class hierarchy as such:

        +-- VirtualNode
        |
INode --+                  +-- SiteNode
        |                  |
        +-- AbstractNode --+
                           |
                           +-- SiteSubNode

And a corresponding NodeCollection class that is build on INode. In order to display a NodeCollection I need to know the final type of each member. So I need a function like this

foreach (INode n in myNodeCollection)
{
    switch(n.GetType())
    {
        case(typeof(SiteNode)):
        // Display n as SiteNode
    }
}

Now, this is really not an object oriented way of doing it. Are there any patterns or recommended ways of doing the same thing, in your opinion?

EDIT
I already thought of adding a Display or Render method to the INode interface. That has the side effect of coupling the view to the model, which I would really like to avoid.

Polymorphism:

When ever you have a select statement using the type of an object, it is a prime candidate for refactoring to polymorphism.

Check out the book Refactoring by Martin Fowler:

"One of the most obvious symptoms of object-oriented code is its comparative lack of switch (or case) statements. The problem with switch statements is essentially that of duplication. Often you find the same switch statement scattered about a program in different places. If you add a new clause to the switch, you have to find all these switch, statements and change them. The objectoriented notion of polymorphism gives you an elegant way to deal with this problem.

Most times you see a switch statement you should consider polymorphism. The issue is where the polymorphism should occur. Often the switch statement switches on a type code. You want the method or class that hosts the type code value. So use Extract Method to extract the switch statement and then Move Method to get it onto the class where the polymorphism is needed. At that point you have to decide whether to Replace Type Code with Subclasses or Replace Type Code with State/Strategy. When you have set up the inheritance structure, you can use Replace Conditional with Polymorphism."

Here is one approach to using polymorphism in your situation:

  1. Define an abstract method in AbstractNode named something like Display().

  2. Then actually implement Display() in each of the SiteNode and SiteSubNode classes.

  3. Then, when you need to display these nodes, you could simply iterate through a collection containing items of type AbstractNode and call Display() for each.

  4. The call to Display() will automatically resolve to the actual concrete implementation for the real type of that item.

  5. Note: You could also move the Display() method from AbstractNode to the INode interface if VirtualNode is to be displayed.

I have the following code copy/pasted multiple times. The values that change are the string literals ("TabStates" changes to "ContentStates" etc..) and the value of the dictionary (RadTabSetting -> ContentSetting).

public static SerializableDictionary<string, RadTabSetting> GetTabStates()
{
    SerializableDictionary<string, RadTabSetting> _tabStates = new SerializableDictionary<string, RadTabSetting>();

    if (!object.Equals(DashboardSessionRepository.Instance.GetSession("TabStates"), null))
    {
        _tabStates = DashboardSessionRepository.Instance.GetSession("TabStates") as SerializableDictionary<string, RadTabSetting>;
    }
    else
    {
        XmlSerializer serializer = new XmlSerializer(_tabStates.GetType());

        string data = DashoardDatabaseRepository.Instance.GetWebLayoutData("TabStates");

        if ( !string.IsNullOrEmpty(data) )
        {
            byte[] dataAsArray = Convert.FromBase64String(data);
            MemoryStream stream = new MemoryStream(dataAsArray);
            _tabStates = serializer.Deserialize(stream) as SerializableDictionary<string, RadTabSetting>;
            }
            DashboardSessionRepository.Instance.SetSession("TabStates", _tabStates);
        }

        return _tabStates;
    }

    public static void SetTabStates(SerializableDictionary<string, RadTabSetting> tabStates)
    {
        DashboardSessionRepository.Instance.SetSession("TabStates", tabStates);
        DashboardDatabaseRepository.Instance.SaveToDatabase("TabStates", tabStates);
    }

I'm not looking for an answer, just curious what I should read about to learn how to rewrite this. I'm sure it's simple enough, just not sure what it's called. Is it just function templating?

    public static T GetStates<T>() where T: new()
    {
        T _states = new T();//(T)Activator.CreateInstance(typeof(T));

        string stateName = StateDictionary.GetStateName(typeof(T));

        if (!object.Equals(DashboardSessionRepository.Instance.GetSession(stateName), null))
        {
            _states = (T)DashboardSessionRepository.Instance.GetSession(stateName);

            //Work-Around
            System.IO.MemoryStream memoryStream = new System.IO.MemoryStream();
            System.Xml.Serialization.XmlSerializer xmlSerializer = new System.Xml.Serialization.XmlSerializer(_states.GetType());
            xmlSerializer.Serialize(memoryStream, _states);
            string data = System.Convert.ToBase64String(memoryStream.ToArray());
            string otherData = DashboardDatabaseRepository.Instance.GetWebLayoutData(stateName);

            if (!string.IsNullOrEmpty(data))
            {
                XmlSerializer serializer = new XmlSerializer(_states.GetType());
                byte[] dataAsArray = Convert.FromBase64String(data);
                MemoryStream stream = new MemoryStream(dataAsArray);

                _states = (T)serializer.Deserialize(stream);
            }
            //Work-Around
        }
        else
        {
            XmlSerializer serializer = new XmlSerializer(_states.GetType());

            string data = DashboardDatabaseRepository.Instance.GetWebLayoutData(stateName);

            if (!string.IsNullOrEmpty(data))
            {
                byte[] dataAsArray = Convert.FromBase64String(data);
                MemoryStream stream = new MemoryStream(dataAsArray);
                _states = (T)serializer.Deserialize(stream);
            }
            DashboardSessionRepository.Instance.SetSession(stateName, _states);
        }

        return _states;
    }

    public static void SetStates<T>(T states) where T: new()
    {
        string stateName = StateDictionary.GetStateName(typeof(T));

        DashboardSessionRepository.Instance.SetSession(stateName, states);
        DashboardDatabaseRepository.Instance.SaveToDatabase(stateName);
    }

static class StateDictionary
{   
    //TODO: Might (should?) be able to redo this polymorphically.
    static IDictionary<Type, string> _stateDictionary = new Dictionary<Type, string>
    {
        {typeof(SerializableDictionary<string, RadTabSetting>), "TabStates"},
        {typeof(SerializableDictionary<string, RadDockContentSetting>), "ContentStates"},
        {typeof(SerializableDictionary<string, RadPaneSetting>), "PaneStates"},
        {typeof(SerializableDictionary<string, RadDockSetting>), "DockStates"},
        {typeof(SerializableDictionary<string, RadDockZoneSetting>), "DockZoneStates"},
        {typeof(SerializableDictionary<string, RadSplitterSetting>), "SplitterStates"},
        {typeof(SerializableDictionary<string, RadSplitBarSetting>), "SplitBarStates"},
        {typeof(KnownGlobalSettings), "GlobalSettings"},
    };

    public static string GetStateName(Type type)
    {
        string stateName = string.Empty;

        if (_stateDictionary.ContainsKey(type))
        {
            stateName = _stateDictionary[type];
        }

        return stateName;
    }
}

Martin Fowler's book is the standard source for refactoring.

But for your specific example, just create a new method that takes in parameters representing the things that change in your example. So you'll want a string parameter for your "TabStates" or "ContentStates" value, and another representing your Value for Key ContentSetting in your dictionary.

Does that make sense? Am I understanding your question fully?

EDIT

Based on your comment, you want to use generics. Something like this should get you going:

public static Dictionary<string, T> GetTabStates<T>()

Just note that you won't be able to do much with your type T unless you add some generic constraints.

If you want to create a new instance of T, then you would need

public static Dictionary<string, T> GetTabStates<T>() where T : new() {

And if you want to access actual properties on an instance of T, then hopefully all possible values for T will implement some sort of interface, in which case you would say:

public interface IFoo {
    int Id { get; set; }
}

public static Dictionary<string, T> GetTabStates<T>() where T : new(), IFoo {
   T Tval = new T();
   Tval.Id = 1;
   //etc       

I'm coding an interface class at the moment to manage showing of user comments on two different kinds of pages, one page is the profiles and the other is the media pages.

Both sets of comments are stored in different tables but I'm wondering whether I should use one function or split both tables into a separate function.

Is the overall goal of OOP to have code that works well for your site or to be able to use it over in different sections without the need to modify lots?

I could have:

showComments($pageId, $type, $userType)
{
    if($type == 'media')
        $sql = "SELECT comment FROM mediatable WHERE id=:pageId";
    elseif($type == 'profile')
        $sql = "SELECT comment FROM profileTable WHERE id=:pageId";

    if($userType == 'moderator')
        //show Moderation Tools

    //Rest of code goes here
}

Or I could seperate it into different functions like so:

showMediaComments($id);
moderateMediaComments($id);

showProfileComments($id);
moderateProfileComments($id);

I'm thinking the second method would be better as I could then use the code again easier but it would required more lines of code ...

Is the overall goal of OOP to have code that works well for your site

Think about it, OOP or not, you'll always want code that works well for your site :)

or to be able to use it over in different sections without the need to modify lots?

I agree pretty much with the answer from @Xeoncross for this one, but I like to view OOP from a more conceptual point of view: objects that collaborates throught message sends with the goal of modelling a subset of the reality (that is, your domain problem). I found it easier to think in terms of domain concepts instead of classes and interfaces (that's the way you use to express those domain concepts).

About the sample you posted, it's good to separate each branch of the conditional in its own method.

To learn why the previous is important, let´s take the following example: suppose there´s a bug in the media comments part of then code (and rest is working fine). For fixing it, you'll have to modify that part, potentially making changes that breaks already working code just because all belongs to the same method (remember, "If it ain't broke, don't fix it").

Follow @Darhazer advice and put each pice of logic in the class where it belongs. That way a class will only know about one and only one thing (MediaComments contains only the code that deals with media comments, ProfileComments does the same for profile comments, and so on). That will make it easy to locate, maintain and modify code related to a particular concept.

You can learn more about the reasons behind this decision reading about The Single Responsibility Principle here and here.

Also, I think you'll find interesting the "Replace Conditional with Polymorphism" section of Martin Fowler's book: Refactoring: Improving the Design of Existing Code that explains the drawbacks of having a conditional statement like yours and gives you an object oriented technique to get rid of it.

Implementing agile in projects requires the ability to do refactoring. It is not really a must, but code refactoring has proven to be a good engineering practice.

In an agile (Scrum) project on the iSeries platform, which requires development (new code and modifications to legacy code) in RPG, RPG LE, is it possible to implement refactoring? If so what are the techniques to do it?

If someone who has tried it could share their experience or just point to references, I would greatly appreciate it.

There is also this book:

http://www.amazon.com/Refactoring-Improving-Design-Existing-Code/dp/0201485672/ref=sr_1_1?ie=UTF8&s=books&qid=1276528002&sr=8-1

Although largely from an OO perspective, it also provides a process that can be applied to any language.

I wonder how do programmers refactor code that is written in languages like Ruby, Python?

Assuming you get code after a 'previous' guy—so you cannot be sure about quality of tests and their coverage.

Do you use any specific approach?

Without an IDE, you will have to take smaller steps, well protected by comprehensive unit tests. Martin Fowler's Refactoring, written before all the software tools were available, is a pretty good guide to how to refactor safely. You take small steps, checking all along that you're not breaking anything, frequently leaving original code in place until the replacement has been completed. It's tedious but doable.

Many of our design decisions are based on our gut feeling about how to avoid complexity and bloating. Some of our complexity-fears are true, we have plenty of painful experience on throwing away deprecated code. Other times we learn that some particular task isn't really that complex as we though it to be. We notice for example that upkeeping 3000 lines of code in one file isn't that difficult... or that using special purpose "dirty flags" isn't really bad OO practice... or that in some cases it's more convenient to have 50 variables in one class that have 5 different classes with shared responsibilities... One friend has even stated that adding functions to the program isn't really adding complexity to your system.

So, what do you think, where does bloated complexity creep from? Is it variable count, function count, code line count, code line count per function, or something else?

i think its came from your problem domain, any software project sooner or later would become bloated and complex as the new bug is found or new features needed.

at first design you will never know or understand what problems lies ahead, it is natural, and the modification to work around this problem that will make your code evolve out of initial design path and mostly we see it as complicated as it is not our initial design.

to prevent this one of the best practice would be refactoring your codes regularly, and let your codes evolve as no codes are perfect in just single iteration. you might want to read a book about refactoring, it will explain all technique to handle complexity and bloated codes.

for example: http://www.amazon.com/Refactoring-Improving-Design-Existing-Code/dp/0201485672

Obviously, "Hello World" doesn't require a separated, modular front-end and back-end. But any sort of Enterprise-grade project does.

Assuming some sort of spectrum between these points, at which stage should an application be (conceptually, or at a design level) multi-layered? When a database, or some external resource is introduced? When you find that the you're anticipating spaghetti code in your methods/functions?

There is no real answer to this question. It depends largely on your application's needs, and numerous other factors. I'd suggest reading some books on design patterns and enterprise application architecture. These two are invaluable:

Design Patterns: Elements of Reusable Object-Oriented Software

Patterns of Enterprise Application Architecture

Some other books that I highly recommend are:

The Pragmatic Programmer: From Journeyman to Master

Refactoring: Improving the Design of Existing Code

No matter your skill level, reading these will really open your eyes to a world of possibilities.

Ok, so I am designing a class here and I have two options. I can either write multiple methods or a single method which takes say an Enum.

I'm trying to figure out the best way to do this.

Lets take an example:

public class myClass
{ ...
    public void DoStuff1()
    { ... Do Stuff ... }

    public void DoStuff2()
    { ... Do Stuff ... }

    public void DoStuff3()
    { ... Do Stuff ... }
}

Ok, all makes sence, now an alternative way would be:

public class myClass
{ ...

    public Enum Option
    {
        Option1,
        Option2,
        Option3
    }

    public void DoStuff(Option option)
    { ... Do Stuff ... }
}

In terms of DRY, they are not that bad becaues the code pretty much calls internal methods anyhow, so it's only what is visible to the user for them to choose.

So which do you prefer an why and are there any guidelines around this already?

You need to take into account the purpose of each execution path. Is the behavior for Options 1, 2, and 3 very similar, changing only in minor ways? Or are all three options different, changing in more significant ways? Even if each option is similar, what is a clearer way of representing the operations for those options? An enum with values Option1, Option2, and Option3 is pretty vauge.

You should also be asking yourself...will I stop at three options? What is the possibility of needing more than three in the future? Will you need many more? Perhaps a more object oriented approach is needed.

There are many tools to help you decide what you need to solve your problem. Design patterns, anti-patterns, and probably books on refactoring could help provide some problem-solving fuel.

On on hand:
1. You never get time to do it.
2. "Context switching" is mentally expensive (difficult to leave what you're doing in the middle of it).
3. It usually isn't an easy task.
4. There's always the fear you'll break something that's now working.

On the other:
1. Using that code is error-prone.
2. Over time you might realize that if you had refactored the code the first time you saw it, that would have saved you time on the long run.

So my question is - Practically - When do you decide it's time to refactor your code?

Thanks.

Martin Fowler in his book if the same name suggests you do it the third time you're in a block of code to make another change. First time in the block, you happen to notice you should refactor, but don't have time. Second time back...same thing. Third time back-now refactor. Also, I read the developers of a current release of smalltalk (squeak.org, I think) say they go through a couple weeks of intense coding...then they step back and look at what can be refactored. Personally I have to resist the impulse to refactor as I code or I get 'paralyzed'.

Currently I am working on a bit of code which (I believe) requires quite a few embedded if statements. Is there some standard to how many if statements to embed? Most of my googling has turned up things dealing with excel..don't know why.

If there is a standard, why? Is it for readability or is it to keep code running more smoothly? In my mind, it makes sense that it would be mainly for readability.

An example of my if-structure:

if (!all_fields_are_empty):
    if (id_search() && validId()):
        // do stuff
    else if (name_search):
        if (name_exists):
            if (match < 1):
                // do stuff
        else:
            // do stuff
    else if (name_search_type_2):
        if (exists):
            if (match < 1):
                // do stuff
        else:
            // do stuff
else:
    // you're stupid

I have heard that there's a limit to 2-3 nested for/while loops, but is there some standard for if-statements?

I don't think there is a limit but i wouldn't recommend embeddeding more the two - it's too hard to read, difficult to debug and hard to unit test. Consider taking a look at a couple great books like Refactoring, Design Patterns, and maybe Clean Code

I currently have a job in QA and at my company everyone in QA writes automated tests. Recently I have been reading Refactoring: Improving the Design of Existing Code by Martin Fowler. I have come to realize that a lot of our test classes and test utilities are very messy, redundant, and in dire need of refactoring.

Especially since I've been reading up on the topic, I am eager to dive in and clean things up, but there is one problem. In his book, Fowler emphasizes that unit testing the code under refactor is essential to prevent bugs from being introduced. Since the code I am trying to refactor is test code, it seems kind of silly to unit test it. Do any of you guys have suggestions on how to refactor or even design test code? Also, if it helps, the test framework we use is built on top of JUnit and therefore the test classes I'm referring to are descendants of TestCase.

Thanks!

Nope that's not entirely correct. The trick to re factoring is to be able to verify that you've kept the functionality.

You need to make sure that that all changes are verified before and after. There's not really away around this. However you don't need to write automated tests for that code but it may help.

The trick with complex refactoring of test code is to be able to run the tests against the system under test and get the same results.

So in fact the verifiability is making sure that the output is the same. This should be automated. You can do this in many ways but the simplest would be to output the test you're running with the data you're testing with together with output you expect and the output you receive.

Possibly you can use some machine readable format like XML to make is easier to parse.

I have used to use PHP and MySQL a lot "back in the day" to create all kinds of websites including text-based games.

Back when I was creating these project I was using such code as:

$query = 'SELECT user FROM users WHERE user_id = 1';
$result = mysql_query($query);

To get results from the database. I realise that this is a very bad way to be doing it, my questions is what is the best way to now do SELECT, UPDATE, DELETE etc. As these will be used all over the website should I be making functions in one file and so on. I would like to know the "best/safest" way to do about doing this. I understand having SQL statements on one line is bad as they are open to SQL injections, I would like to know what this means exactly and how to get around this problem safely.

Its mainly handling the database that I seem to not understand. As I have not been paying attention to PHP for a few years now I see many things have moved on from when I had created my projects.

I have looked around the net and have found W3Schools to be a useful resource but I would like to hear it from people that are using this everyday to find out how I should be doing things.

Overall, how do I go about safely connecting a database and how can I grab data form the database safely for the whole website to use.

This includes:

  • Connecting a database
  • Getting database from the database
  • Echo the data from the database onto a page

And anything else that you can think of to help me understand how to structure a "safe" website.

Thanks to anyone that replies to this/these questions, I will be very active in comments for this asking more questions about things I do not full understand.

Side Note: this website will be created using HTML, JavaScript, PHP and using a MYSQL database.

It's all start with Separation of concerns and nowadays the most popular architecture for web applications is Model-View-Controller (you don't need to invent one, you may use some of the existing PHP frameworks, each of them is bundled with some kind of ORM).

Since you are asking about isolating the database code, the models is the first thing you should learn from the MVC paradigm. All operations on the data, including any business logic and calculations, all DB operations, go to the model classes. There are different way you may structure them. One popular pattern is the Active Record - literally a class per table. I'm not a big fan of this - you may create classes after your logical entities (user, game, etc), and they may operate on multiple tables. But those classes may be build upon Active Record anyway.

If you are working with existing code, you can start with isolating the queries in objects, so all code that work with the database is in one place - then you can start restructure objects to follow the chosen architecture : User objects works with the users table, and so on... After you separated the DB code in objects, you can easily switch to PDO or other db-implementations and protect from SQL injections.

In the end I'd like to recommend few books: Refactoring - the best book about how to turn legacy code in beautiful OO solutions

Patterns of enterprise application architecture - hard to read but has a lot of useful info

Implementation Patterns - unlike books on Design Patterns, this is focused on the small decisions, literally on each line of code

I keep hearing how Test Driven Development is an unfortunate name for the technique because TDD is not about Testing, it's about design.

Personally, I've never had a problem with the name simply because I've always thought about it more as a way to program to a specification. I've never had a problem with the fact that the code that is verifying that I've met the spec has the word 'test' in a class name, method name, annotation, or attribute. It's a convention I've followed in order to have test frameworks do the heavy lifting for me.

I'm not the most dogmatic about my approach, although I do try to write my tests first, I often find myself inspired to write some extra code which I then try to wrap in tests. I do tend to find that TDD outsmarts me from an API-design perspective every time I do that though (and I inevitably end up refactoring my non-tested code as soon as I start writing tests around it), but the point is I'm okay with not writing all tests up-front as long as I end up with a test harness around everything of interest when I'm done.

So, back to the question. What would be a better name for TDD? Would a name that didn't involve the word 'test' make a difference?

In my (subjective) opinion, I believe that the reason some people started the campaign against the word 'Test' in Test-Driven Development was because of bad experience with explaining the concept to certain types of developers.

There are developers who, as soon as they hear the word 'Test', stop listening - they think it's not their job, so why should they care? That attitude is obviously detrimental if you need people to adopt the practice of TDD. If one solution to that problem is to rename TDD into something that does not include the word 'Test', then fine by me, but for the rest of us, let's just continue to call it TDD.

I've seen at least the following terms suggested:

  • Test-Driven Development (the original term)
  • Test-Driven Design (because it's more about design)
  • Example-Driven Design (because tests could be viewed as examples of how the code is supposed to work)
  • Design-by-Example (just another phrase for EDD)
  • Behavior-Driven Design (the declared intent was different, but most attempts so far has looked a lot like TDD with some extra verbosity added)

Even though 'Design' is very important to me, I still think the tests provide value. Sure, they are not QA tests, but the entire mass of tests as a by-product of TDD still provide the safety net that allow you to refactor with confidence. In fact, Refactoring has an entire section on unit testing, because Fowler viewed it to be very dangerous to attempt refactoring without a regression test suite.

Anyone really, truly believing that the tests are of no importance in TDD should put their money where their mouth is and delete the unit tests when their code is done.

For me, TDD makes sense. Yes, it is first and foremost about Development and Design, but the Tests are important artifacts too.

I'm toying with the idea of phasing in an ORM into an application I support. The app is not very structured with no unit tests. So any change will be risky. I'm obviously concerned that I've got a good enough reason to change. The idea is that there will be less boiler plate code for data access and there for greater productivity.

Do this ring true with your experiences?
Is it possible or even a good idea to phase it in?
What are the downsides of an ORM?

The "Robert C Martin" book, which was actually written by Michael Feathers ("Uncle Bob" is, it seems, a brand name these days!) is a must.

It's near-impossible - not to mention insanely time-consuming - to put unit tests into an application not developed with them. The code just won't be amenable.

But that's not a problem. Refactoring is about changing design without changing function (I hope I haven't corrupted the meaning too badly there) so you can work in a much broader fashion.

Start out with big chunks. Set up a repeatable execution, and capture what happens as the expected result for subsequent executions. Now you have your app, or part of it at least, under test. Not a very good or comprehensive test, sure, but it's a start and things can only get better from there.

Now you can start to refactor. You want to start extracting your data access code so that it can be replaced with ORM functionality without disturbing too much. Test often: with legacy apps you'll be surprised what breaks; cohesion and coupling are seldom what they might be.

I'd also consider looking at Martin Fowler's Refactoring, which is, obviously enough, the definitive work on the process.

I would strongly recommend getting a copy of Michael Feather's book Working Effectively With Legacy Code (by "Legacy Code" Feathers means any system that isn't adequately covered by unit tests). It is full of good ideas which should help you with your refactoring and phasing in of best practices.

Sure, you could phase in the introduction of an ORM, initially using it for accessing some subset of your domain model. And yes, I have found that use of an ORM speeds up development time - this is one of the key benefits and I certainly don't miss the days when I used to laboriously hand-craft data access layers.

Downsides of ORM - from experience, there is inevitably a bit of a learning curve in getting to grips with the concepts, configuration and idiosyncracies of the chosen ORM solution.

Edit: corrected author's name

I often see functions where other functions are called multiple times instead of storing the result of the function once.

i.e (1):

void ExampleFunction()
{ 
    if (TestFunction() > x || TestFunction() < y || TestFunction() == z)
    {
        a = TestFunction();
        return; 
    }
    b = TestFunction();
}

Instead I would write it that way, (2):

void ExampleFunction()
{
    int test = TestFunction();
    if (test > x || test < y || test == z)
    {
        a = test;
        return;
    }
    b = test;
}

I think version 2 is much better to read and better to debug. But I'm wondering why people do it like in number 1? Is there anything I don't see? Performance Issue? When I look at it, I see in the worst case 4 function calls in number (1) instead of 1 function call in number (2), so performance should be worse in number (1), shouldn't it?

When a temporary is used to store the result of a very simple expression like this one, it can be argued that the temporary introduces unecessary noise that should be eliminated.

In his book "Refactoring: Improving the Design of Existing Code", Martin Fowler lists this elimination of temporaries as a possibly beneficial refactoring (Inline temp).

Whether or not this is a good idea depends on many aspects:

  • Does the temporary provides more information than the original expression, for example through a meaningful name?
  • Is performance important? As you noted, the second version without temporary might be more efficient (most compilers should be able to optimize such code so that the function is called only once, assuming it is free of side-effects).
  • Is the temporary modified later in the function? (If not, it should probably be const)
  • etc.

In the end, the choice to introduce or remove such temporary is a decision that should be made on a case by case basis. If it makes the code more readable, leave it. If it is just noise, remove it. In your particular example, I would say that the temporary does not add much, but this is hard to tell without knowing the real names used in your actual code, and you may feel otherwise.

I saw this question asked about C# I would like an answer for PHP. I have some old code that has 4 pages of foreach loops and conditions which just makes it hard to read and follow. How would I make this more OO? I was thinking of using SPL Functions but don't fully understand whats involved yet.

Some good advice from Johnathan and gnud. Don't worry so much about the SPL, and learn more about refactoring, and look into what refactoring tools are available for PHP.

Also I can't recommend enough reading Working Effectively with Legacy Code:

alt text

And of course the canonical Refactoring Book:

alt text

PHP is a difficult language to keep clean of code smells, but with a little perseverance it can be done! And you will thank yourself every day you have to look at your codebase.

It's a great environment, but when I right click on, say a model .rb file, it is not readily apparent how I might rename a file.

Now RubyMine JetBrains is a brilliant program which I love, so I'm not going to stop using it, I just need to get around this simple problem.

Use the Alt+Shift+R shortcut to rename the file when it's highlighted in the project view panel.

Generally, actions like rename is part of the Refactoring (because you also need to rename the usages of it) so you will also find it in the Refactor menu on top.

I've got lots of problems with project i am currently working on. The project is more than 10 years old and it was based on one of those commercial C++ frameworks which were very populary in the 90's. The problem is with statecharts. The framework provides quite common implementation of state pattern. Each state is a separate class, with action on entry, action in state etc. There is a switch which sets current state according to received events.

Devil is hidden in details. That project is enormous. It's something about 2000 KLOC. There is definitely too much statecharts (i've seen "for" loops implemented using statecharts). What's more ... framework allows to embed statechart in another statechart so there are many statecherts with seven or even more levels of nesting. Because statecharts run in different threads, and it's possible to send events between statecharts we have lots of synchronization problems (and big mess in interfaces).

I must admit that scale of this problem is overwhelming and I don't know how to touch it. My first idea was to remove as much code as I can from statecharts and put it into separate classes. Then delegate these classes from statechart to do a job. But in result we will have many separate functions, which logically don't have any specific functionality and any change in statechart architecture will need also a change of that classes and functions.

So I asking for help: Do you know any books/articles/magic artefacts which can help me to fix this ? I would like to at least separate as much code as I can from statechart without introducing any hidden dependencies and keep separated code maintainable, testable and reusable.

If you have any suggestion how to handle this, please let me know.

Sounds to me like your best bet is (gulp!) likely to start from scratch if it's as horrifically broken as you make out. Is there any documentation? Could you begin to build some saner software based on the docs?

If a complete re-write isn't an option (and they never are in my experience) I'd try some of the following:

  1. If you don't already have it, draw an architectural picture of the whole system. Sketch out how all the bits are supposed to work together and that will help you break the system down into potentially manageable / testable parts.
  2. Do you have any kind of requirements or testing plan in place? If not, can you write one and start to put unit tests in place for the various chunks of code / functionality which exist already? If you can do that, you can start to refactor things without breaking as much of whatever does currently work.
  3. Once you've broken things down a bit, start building your unit tests into integration tests which pull together more of the functionality.

I've not read them myself, but I've heard good things about these books which may have some advice you can use:

  1. Refactoring: Improving the Design of Existing Code (Object Technology Series).
  2. Working Effectively with Legacy Code (Robert C. Martin)

Good luck! :-)

I've heard people saying that good design involves using inheritance instead of littering your code with if block. In what situation should we use inheritance, and when condition block is just fine?

One refactoring like this is called Replace Conditional With Polymorphism.

We actually favor composition over inheritance, but ... it's really too broad of a topic to discuss here, however the book is worth a read and can get you started.

I've inherited some code and within it is a 500 line switch statement. Basically, it switches on a string task and executes the corresponding actions.

I have since moved each case statement in to their own method in a new class. The giant switch statement still exists but instead of inlining the logic each case just calls a method so it's much neater.

The problem is that the methods modify a lot of different things. 50% of the methods require 0 arguments passed in. Some 40% require 5 arguments and the remaining 10% require 10 arguments each.

Currently this works but I'd like to make it better. Either get rid of the switch statement or lower the amount of passed in parameters somehow.

I was thinking of using a Dictionary that mapped strings to Actions to eliminate the entire switch, but this wouldn't work because I'm using a lot of ref parameters (primitive types) and there'd be no way to pass those in to the constructor and have them later be referentially modified.

The obvious solution to that problem is just to place all 16 or so variables in to a separate class and pass that but a lot of them aren't very related so it just replaces one problem with another (long parameter list with non-cohesive data class).

Was wondering if there were any other ways to improve this code. Thanks for reading.

Since your question includes no code, the answer can't really either. I think the best thing to do is to point you to page 82 of one of the all-time best software books: Refactoring: Improving the Design of Existing Code.

"One of the most obvious symptoms of object-oriented code is its comparative lack of switch statements. Most times you see a switch statement you should consider polymorphism."

He then lists some of the specific patterns to be used to help make this happen.

My software company never did BDD or even TDD before. Testing before meant to simply try out the new software some days before deployment.

Our recent project is about 70% done. We also use it as a playground for new technologies, tools and ways of developement. My boss wanted that I switch to it to "test testing".

I tried out Selenium 2 and RSpec. Both are promising, but how to catch up months of developement? Further problems are:

  • new language
  • never wrote a line of code by myself
  • huge parts are written by freelancer
  • lots of fancy hacking
  • no documentation at all besides some source comments and flow charts

All I was able to was to do was to cover a whole process with Selenium. This appeared to be quite painfully (but still possible), since the software was obivously never meant to be testet this way. We have lots of dynamically generated id ´s, fancy jQuery and much more. Dont even know how to get started with RSpec.

So, is it still possible to apply BDD to this project? Or should I run far away and never come back?

Reading the book Working Effectively with Legacy Code might be helpful. There is also a short version in PDF form.

Before you start - have you asked your boss what he values from the testing? I'd clarify this with your boss first. The main benefits of system-level BDD are in the conversations with business stakeholders before the code is written. You won't be able to get this if all you're doing is wrapping existing code. At a unit level, the main benefits are in questioning the responsibilities of classes and their behavior, which, again, you won't be able to get. It might be useful for helping you understand the value of the application and each level of code, though.

If your boss just wants to try out BDD or TDD it may be simpler to start a new project, or even grab an existing project from someone else to wrap some tests around. If he genuinely wants to experiment with BDD over legacy code, then it may be worth persisting with what you have - @Esko's book suggestion rocks. Putting higher-level system tests around the existing functionality can help you to avoid breaking things when you refactor the lower-level code (and you will need to do so, if you want to put tests in place). I additionally recommend Martin Fowler's "Refactoring".

RSpec is best suited to applying BDD at a unit level, as a variant of TDD. If you're looking to wrap automated tests around your whole environment, take a look at Cucumber. It makes reusing steps a lot simpler. You can then call Selenium directly from your steps.

I put together a page of links on BDD here, which I hope will help newcomers to understand it better. Best of luck.

if (getchar == '+') {
    answer = getnum1+getnum2;   // if the random operation is add, it will add
    addtemp++;                 // <---- disregard this
    if (answer == getanswer)   //  the answer from my textbox which is
    {                         //  user-input it is stored on "getanswer"
        correct++;            // it is compared if its correct or wrong
        addcomp++;
    }
    else { wrong++; }
}
else if (getchar == '-') {
    subtemp++;
    answer = nextValue - nextValue1;
    if (answer == getanswer) {
        correct++;
        subcomp++;
    }
    else { wrong++; }
}
else if (getchar == '*') {
    multemp++;
    answer = nextValue * nextValue1;
    if (answer == getanswer) {
        correct++;
        mulcomp++;
    }
    else { wrong++; }
}
else if (getchar == '/') {
    divtemp++;
    answer = nextValue / nextValue1;
    if (answer == getanswer) {
        correct++;
        divcomp++;
    }
    else { wrong++; }
}
else if (getchar == '%') {
    modtemp++;
    answer = nextValue % nextValue1;
    if (answer == getanswer) {
        correct++;
        modcomp++;
    }
    else { wrong++; }
}

C# programming HELP! Now whenever i press the button "SCORES" it is a MessageBox.Show(correct or wrong) , the values are wrong. it sometimes increment corrected but just once or twice.Is there something wrong with my code?

/////////////////////////////////////////////////////////// ENTIRE CODE for @boncodigo

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;

namespace A1_ALS_Noroña
{
    public partial class Form1 : Form
    {
        int minV=0, maxV=0,ques=0,tempques=1;

        int addtemp, subtemp, multemp, divtemp, modtemp;
        int addcomp, subcomp, mulcomp, divcomp, modcomp;
        int answer,getanswer;
        int getnum1, getnum2;

        char getchar;
        char[] select = new char[5];
        int count=0;
        int correct, wrong;

        public Form1()
        {
            InitializeComponent();
        }



        private void bttnstart_Click(object sender, EventArgs e)
        {
            bttnanswer.Enabled = true;
            grpbox1.Enabled = false;
            bttnanswer.Enabled = true;
            lblnum1.Visible = true;
            lblnum2.Visible = true;
            lbloperator.Visible = true;
            bttnstop.Enabled = true;
            bttnscore.Enabled = true;
            bttnstart.Enabled = false;


            Random random = new Random();
            int nextValue = random.Next(minV, maxV);
            int nextValue1 = random.Next(minV, maxV);
            lblnum1.Text = nextValue.ToString();
            lblnum2.Text = nextValue1.ToString();

            var rand = new Random();

            char num = select[rand.Next(count)];
            lbloperator.Text = Convert.ToString(num);
        }

        private void txtboxmin_TextChanged(object sender, EventArgs e)
        {
            minV = Convert.ToInt32(txtboxmin.Text);
        }

        private void txtbxmax_TextChanged(object sender, EventArgs e)
        {
            maxV = Convert.ToInt32(txtbxmax.Text);
        }

        private void bttnexit_Click(object sender, EventArgs e)
        {
            Application.Exit();
        }

        private void bttnstop_Click(object sender, EventArgs e)
        {
            MessageBox.Show("APPLICATION STOP! The application will restart.");
            Application.Restart();
        }



        private void bttnanswer_Click(object sender, EventArgs e)
        {

            tempques++;
            Random random = new Random();
            int nextValue = random.Next(minV,maxV);
            int nextValue1 = random.Next(minV, maxV);
            lblnum1.Text = nextValue.ToString();

            var rand = new Random();
            char num = select[rand.Next(count)];
            lbloperator.Text = Convert.ToString(num);
            lblnum2.Text = nextValue1.ToString();
            getnum1 = Convert.ToInt32(lblnum1.Text);
            getnum2 = Convert.ToInt32(lblnum2.Text);
            getanswer = Convert.ToInt32(txtbxans.Text);
            getchar = Convert.ToChar(lbloperator.Text);

            if (getchar == '+')
            {

                answer = getnum1 + getnum2;
                addtemp++;              
                if (answer == getanswer)  
                {                      
                    correct++;            
                    addcomp++;
                }
                else
                {
                    wrong++;
                }
            }
            else if (getchar == '-')
            {
                subtemp++;
                answer = nextValue - nextValue1;
                if (answer == getanswer)
                {
                    correct++;
                    subcomp++;
                }
                else
                {
                    wrong++;
                }
            }
            else if (getchar == '*')
            {
                multemp++;
                answer = nextValue * nextValue1;
                if (answer == getanswer)
                {
                    correct++;
                    mulcomp++;
                }
                else
                {
                    wrong++;
                }
            }
            else if (getchar == '/')
            {
                divtemp++;
                answer = nextValue / nextValue1;
                if (answer == getanswer)
                {
                    correct++;
                    divcomp++;
                }
                else
                {
                    wrong++;
                }
            }
            else if (getchar == '%')
            {
                modtemp++;
                answer = nextValue % nextValue1;
                if (answer == getanswer)
                {
                    correct++;
                    modcomp++;
                }
                else
                {
                    wrong++;
                }
            }







        }

        private void txtbxques_TextChanged(object sender, EventArgs e)
        {
            ques = Convert.ToInt32(txtbxques.Text);

        }

        private void chkbxtimer_CheckedChanged(object sender, EventArgs e)
        {
            rdoeasy.Enabled = true;
            rdomed.Enabled = true;
            rdohard.Enabled = true;
        }

        private void chkboxAdd_CheckedChanged(object sender, EventArgs e)
        {

            if (chkboxAdd.Checked == true)
            {
                select[count] = '+';
                count++;
            }
            else if (chkboxAdd.Checked == false) 
            {
                Array.Clear(select, 0, select.Length);
                count--;
            }

        }

        private void chkboxSub_CheckedChanged(object sender, EventArgs e)
        {
            if (chkboxSub.Checked == true)
            {
                select[count] = '-';
                count++;

            }
            else if (chkboxSub.Checked == false)
            {
                Array.Clear(select, 0, select.Length);
                count--;
            }

        }

        private void chkboxMul_CheckedChanged(object sender, EventArgs e)
        {
            if (chkboxMul.Checked == true)
            {
                select[count] = '*';
                count++;

            }
            else if (chkboxMul.Checked == false)
            {
                Array.Clear(select, 0, select.Length);
                count--;
            }

        }

        private void chkboxDiv_CheckedChanged(object sender, EventArgs e)
        {
            if (chkboxDiv.Checked == true)
            {
                select[count] = '/';
                count++;

            }
            else if (chkboxDiv.Checked == false)
            {
                Array.Clear(select, 0, select.Length);
                count--;
            }
        }

        private void chkboxMod_CheckedChanged(object sender, EventArgs e)
        {
            if (chkboxMod.Checked == true)
            {
                select[count] = '%';
                count++;

            }
            else if (chkboxMod.Checked == false)
            {
                Array.Clear(select, 0, select.Length);
                count--;
            }
        }

        private void bttnscore_Click(object sender, EventArgs e)
        {
            MessageBox.Show("Correct Answer"+correct);
        }




        }
    }

One thing in advance: I don't know where your bug is. Here's just a couple of tips that I think it would make sense if you think about them, in order to avoid similar bugs in the future:

If I had to review this code, my major issue would be with the amount of repeating code, where very similar multi-line long patterns are copied over and over. I think in the whole code you don't call any method, but implement stuff right away in the event-handlers, thereby repeating yourself and multiplying the potential for bugs. Let's look at this code:

if (chkboxSub.Checked == true)
{
    select[count] = '-';
    count++;

}
else if (chkboxSub.Checked == false)
{
    Array.Clear(select, 0, select.Length);
    count--;
}

apart from the bug with count when you have added several operators to the select-array, this code repeats several times. Let's extract the code into a method and make the bits that change parameterizable:

void AddOrRemoveOperator(bool isChecked, char operatorChar) {
  if (isChecked) {
    select[count] = operatorChar;
    count++;
  }
  else {
    Array.Clear(select, 0, select.Length);
    count--;
  }
}

Now you can call that method many times, e.g. like:

AddOrRemoveOperator(chkboxSub.Checked, '-');

Next point would be lack of .NET Base Class Library knowledge (BCL). E.g., wouldn't it be easier to use a List<T> instead of an array?

The above method becomes:

void AddOrRemoveOperator(bool isChecked, char operatorChar) {
  if (isChecked) {
    select.Add(operatorChar);
  }
  else {
    select.Clear();
  }
}

An Observation: All operators except the add one use the values nextValue, nextValue1 while the add one uses getnum1 and 2. Is that intended?

Short of extracting the code blocks in bttnanswer_Click into their own class, you can also extract the repeating code into a method:

 void PerformComparison(Func<int> answerProvider, 
    ref int operatorCount, 
    ref int operatorSpecificCorrectCount)
 {
    var answer = answerProvider();
    operatorCount++;
    if (answer == getanswer) {
        correct++;
        operatorSpecificCorrectCount++;
    }
    else {
        wrong++;
    }
 }

That code would still drive me mad (because the class you have programmed lacks cohesion), but we have fought code duplication. Now you can call the method e.g. like that:

if (getchar == '+')
{
  PerformComparison(()=>getnum1 + getnum2, ref addtemp, ref addcomp);
}

There are many techniques to morph code into forms that are more easily testable and maintainable (refactoring), we have only used extract method so far. For more techniques the book Refactoring: Improving the Design of Existing Code is still highly recommendable.

This is a design question/discussion.

I created a solution which contains about 50 classes.

I refactored the classes and removed duplicated codes by introducing new abstract classes which contain shared information between classes so that those classes inherit from them.

E.g.

Abstract ColumnBuilder
ColumnBuilderForDepartmentReport : ColumnBuilder
ColumnBuilderForStoreReport : ColumnBuilder

No now my design contains lots of classes and duplicated codes have been removed which is a positive thing.

Is this a good design?

I've heard we should be preferring Composition over Inheritance.

How would it apply to these kind of scenarios where you want to use inheritance to remove duplicated code? Should I refactor the code to use Composition? How?

Thanks

I've heard we should be preferring Composition over Inheritance.

Generally good advice. But it doesn't mean you should replace them all.

If your subclasses need all of what's in the superclasses and each subclass adds small bits of function, you might be fine to stay with inheritance.

How would it apply to these kind of scenarios where you want to use inheritance to remove duplicated code? Should I refactor the code to use Composition? How?

You can also use composition to remove duplicated code.

The refactoring you're looking for is Replace Inheritance with Delegation. Googling this phrase may find you a few references with more detail than a sample diagram, but you really ought to get the book Refactoring.

This might be a duplicate, but as I haven't seen anything hopefully it isn't. Basically, I've never seen a good rule-of-thumb for when to split a chunk of code into separate methods and objects. Does anyone have any good rules they found work well?

If a method doesn't fit on a page, it's too big. :-)

That's not an excuse to use a really small font or use a really big monitor

Go and read;

Although they talk about how to deal with what you already have, if you read them and learn from them you can aim not to create lagacy code to start with!

Typically I implement classes (C#, C++) via many private functions that serve no purpose other than to separate concerns & logic for readability, maintainability, and scope. For example, if I see a function that is 100 lines long and has 3 comments in it separating it out into virtual 1/3 section chunks, that to me is 3 functions. So I break that larger function out into 3 smaller ones and now that original function only calls 3 functions.

I don't know the name for this paradigm. At first I thought it might be functional programming or modular programming, but that doesn't seem to be the case. Can anyone help me figure out what this paradigm is called? Additionally, a link to a wikipedia article I could read that talks about this exact use case would be great (Of course if you tell me the name of the paradigm I could easily look this up myself).

Thanks in advance.

The actual refactoring practice of creating a new, smaller method from existing code in a oversized method is called Extract Method.

Example

Original code

void printOwing() {
    printBanner();

    //print details
    System.out.println ("name:  " + _name);
    System.out.println ("amount " + getOutstanding());
}

After performing Extract Method

void printOwing() {
    printBanner();
    printDetails(getOutstanding());
}

void printDetails (double outstanding) {
    System.out.println ("name:  " + _name);
    System.out.println ("amount " + outstanding);
}

If the code you are trying to pull out of a method doesn't even belong in the class to begin with, then you can similarly use Extract Class.

Martin Fowler, one of the premier software craftsman in the industry wrote a fantastic book on the concepts of refactoring that you can find here. This book will give you step-by-step recipes to tackle a large majority of the refactorings you will ever need.

Also, JetBrains makes some really good IDEs for almost any language that have some really nice refactoring tools built in. They also have a plugin for Visual Studio called ReSharper that provides some of the same features and benefits.

Where can I learn to refactor code?

Fowler's book is a good place to begin, but before doing any refactoring you should make sure you have automated tests for your code. Refactoring without tests is risky at best.

Visual Studio supports the most common refactoring operations, but you may also want to take a look at Resharper, which adds additional tools.

Will it be easy for a C++ developer to read Refactoring: Improving the Design of Existing Code

Is there any other book that I should read about refactoring? Feel free to add any articles on refactoring.

Read a book called Refactoring by Martin Fowler.

I'm the author of Refactoring to Patterns. I have recently completed work on a multimedia album about Refactoring (in C++, Java, and soon, C#).

You can look at samples of this album here:

In addition, if you want to get good at recognizing what kind of code needs refactoring, you can consider studying my album on Code Smells as well. See

If you work with legacy code then it may be worth getting Working Effectively with Legacy Code by Michael Feathers.

Refactoring to Patterns by Joshua Kerievsky

Language used in the book shouldn't matter, the concept is what is important. This book is a practical approach to refactoring.

Consider the case where we might want to make use of a service, such as

class MyService {
    public double calculateSomethingVeryComplex();
}

These kind of functional-looking classes (that contain only a single method) tend to show up only a single public method, although we know (or imagine, a least), they are composed by a lot of smaller methods internally:

class MyService {
    public double calculateSomethingVeryComplex()
    private double getA();
    private double getB();
    private double getC();
    ...
}

and, although the main method could usually pass its objects to getA(), getB(), ..., through method arguments, we generally tend to define them as class attributes (fields), to get the job done:

class MyService {
    private double val1;
    private double val2;
    private double val3;

    public double calculateSomethingVeryComplex()
    private double getA();
    private double getB();
    private double getC();
    ...
}

If not using dependency-injection, each time we need something very complex to be calculated, we simply instantiate a new MyService, call its only method, and we're done.

When using dependency-injection we have the apparent problem of wanting to instantiate MyService in the Composition Root, which is almost okay, although things can get messy with those private fields (for instance, in a multi-threaded scenario).

I see 3 main options here:

  1. Create a MyServiceFactory that I have as dependency to any class that wishes to use MyService. I don't like this approach as it may in certain kind of applications to skyrocket the number of classes and apparent complexity of the system.
  2. Encapsulate the current MyService class in another one, that has the responsibility of instantiating MyService, running it, and then returning the calculated value from it. This seems nice as the public API of the class is the same as the original and we happily use dependency-injection with no trouble.
  3. Let it be just as it is. If we try to run 2 concurrent calculations we may have concurrency problems. If we try to run the same method twice one after another, we may have problems with field reuse, also.

The second point seems a clear winner. Is there any other better alternative to point 2, or any of the shown points?

As described, the MyService class suffers from the Temporary Field code smell as described in Refactoring. Refactor the class and the problem goes away.

If this isn't possible, you'll need to make sure that each client receives a private instance of the service. All DI Containers support that via a lifetime style which is often called Transient.

As a third alternative, you can also use this solution.

Here's the deal: I'm trying, as a learning experience, to convert a C program to C++. This program takes a text file and applies modifications to it according to user-inputted rules. Specifically, it applies sounds changes to a set of words, using rules formatted like "s1/s2/env". s1 represents the characters to be changed, s2 represents what to change it into, and env is the context in which the change should be applied.

I'm sorry that I don't describe this in more depth, but the question would be too long, and the author's site already explains it.

The function I'm having trouble is TryRule. I understand that it's supposed to see if a given rule applies to a given string, but I'm having trouble understanding exactly how it does it. The poor explanation of the parameters confuses me: for example, I don't understand why the strings "s1" and "s2" have to be passed back, or what does "i" represent.

This is the code:

/*
**  TryRule
**
**  See if a rule s1->s2/env applies at position i in the given word.
**
**  If it does, we pass back the index where s1 was found in the
**  word, as well as s1 and s2, and return TRUE.
**
**  Otherwise, we return FALSE, and pass garbage in the output variables.
*/
int TryRule( char *word, int i, char *Rule, int *n, char **s1, char **s2, char *varRep )
    {
        int j, m, cont = 0;
        int catLoc;
        char *env;
        int  optional = FALSE;
        *varRep = '\0';

        if (!Divide( Rule, s1, s2, &env ) || !strchr( env, '_' ))
            return(FALSE);

        for (j = 0, cont = TRUE; cont && j < strlen(env); j++)
        {
            switch( env[j] )
            {
                case '(':
                    optional = TRUE;
                    break;

                case ')':
                    optional = FALSE;
                    break;

                case '#':
                    cont = j ? (i == strlen(word)) : (i == 0); 
                    break;

                case '_':
                    cont = !strncmp( &word[i], *s1, strlen(*s1) );
                    if (cont)
                    {
                        *n = i;
                        i += strlen(*s1);
                    }
                    else
                    {
                        cont = TryCat( *s1, &word[i], &m, &catLoc );
                        if (cont && m)
                        {
                            int c;
                            *n = i;
                            i += m;

                            for (c = 0; c < nCat; c++)
                                if ((*s2)[0] == Cat[c][0] && catLoc < strlen(Cat[c]))
                                    *varRep = Cat[c][catLoc];
                        }
                        else if (cont)
                            cont = FALSE;
                    }
                    break;

                default:
                    cont = TryCat( &env[j], &word[i], &m, &catLoc );
                    if (cont && !m)
                    {
                        /* no category applied */
                        cont = i < strlen(word) && word[i] == env[j];
                        m = 1;
                    }
                    if (cont)
                        i += m;
                    if (!cont && optional)
                        cont = TRUE;
            }
        }
        if (cont && printRules)
            printf( "   %s->%s /%s applies to %s at %i\n", 
            *s1, *s2, env, word, *n );

    return(cont);
}

Given that you are converting from C to C++ you should be refactoring the code to become more readable as well.

One major problem with this code is that the variables have terrible names and I'd wager even the original writer of the routine would need to spend some time analysing it.

Just renaming the variables to be more precise would give you a greater hand in understanding what the code does.

Take a look at some questions tagged under refactoring for some help. There is also Refactoring by Martin Fowler

I am working on a web application which is based on spring MVC. We have various screens for adding different domain components(eg. Account details, Employee details etc). I need to implement an upload feature for each of these domain components i.e. to upload Account, upload employee details etc which will be provided in a csv file (open the file, parse its contents, validate and then persist).

My question is, which design pattern should i consider to implement such a requirement so that upload (open the file, parse its contents, validate and then persist) feature becomes generic. I was thinking about using the template design pattern. Template Pattern

Any suggestions,pointers,links would be highly appreciated.

I am not going to answer your question. That said, let me answer your question! ;-)

I think that design patterns should not be a concern in this stage of development. In spite of their greatness (and I use them all the time), they should not be your primary concern.

My suggestion is for you to implement the first upload feature, then the second and then watching them for what they have that is equal and create a "mother" class. Whenever you come to a third class, repeat the process of generalization. The generic class will come naturally in this process.

Sometimes, I believe that people tend to over engineer and over plan. I am in good company: http://www.joelonsoftware.com/items/2009/09/23.html. Obviouslly, I am not advocating for no design software - that never works well. Nevertheless, looking for similarities after some stuff has been implemented and refactoring them may achieve better results (have you already read http://www.amazon.com/Refactoring-Improving-Design-Existing-Code/dp/0201485672/ref=sr_1_1?ie=UTF8&qid=1337348138&sr=8-1? It is old but stiil great!).

I'd like to start a discussion on the best resources for refactoring tips, with an eye towards Front-End JavaScript refactoring.

A friend whose opinion I respect suggests this book, though it uses examples in Java. I understand that the principles of OO refactoring should translate to another language.

Let's talk about refactoring in general and JS refactoring specifically.

Check out Martin's Clean Code for some inspiration. The examples are in Java but the ideas should translate to JavaScript as well.

In order to refactor properly, you'll need to test your code well as having proper tests in place is a prerequisite for it. Try finding testing tools that give you code coverage as this will be extremely valuable.

In case your code hasn't been tested yet you can think testing as a method of learning. You write tests to prove or disprove your assumptions about it. Once you are done and covered it adequately you should be able to refactor the code using various patterns provided.

As Ira mentioned having a tool to detect clones may be valuable. That's definitely one way to look at it.

I think that often having proper perspective is half the solution. If you can state your design in clear terms, you'll end up with better results. Try not to over-engineer it too much by applying every possible pattern you find. :)

I have a large Shape class, instances of which can (should) be able to do lots of things. I have many "domain" shape classes which inherit from this class, but do not provide any different functionality other than drawing themselves.

I have tried subclassing the Shape class, but then all of the "domain" objects will still inherit this subclass.

How do I break up the class? (it is 300 text lines, C#)

A couple of ideas (more like heuristics):

1) Examine the fields of the class. If a group of fields is only used in a few methods, that might be a sign that that group of fields and the methods that use it might belong in another class.

2) Assuming a well-named class, compare the name of the class to what the class actually does. If you find methods that do things above and beyond what you'd expect from the class' name, that might be a sign that those methods belong in a different class. For example, if your class represents a Customer but also opens, closes, and writes to a log file, break out the log file code into a Logger class. See also: Single Responsibility Principle (PDF) for some interesting ideas .

3) If some of the methods primarily call methods on one other class, that could be a sign that those methods should be moved to the class they're frequently using (e.g. Feature Envy).

CAUTION: Like they say, breaking up is hard to do. If there is risk in breaking up the class, you may want to put some tests in place so that you know you're not breaking anything as you refactor. Consider reading "Working Effectively with Legacy Code" and the "Refactoring" book.

I have a pretty complex java class with over 60 small helper methods used for easy readability and understanding.Just wondering, do you think having these many methods in a class could affect performance in any way?

class MyClass() { 

    //Some static final variables 

    MyInnerClass m = loadAllVariables(); 

    private void update(){ 
    } 
    ... 
    //All 60 methods come here 
    .... 
    private MyInnerClass loadAllVariables() { 
        MyInnerClass m = new MyInnerClass();  
        //load all variables with pre-initialized values 
    } 

    private MyInnerClass() { 
        int a, b, c; //In reality, I have over 20 variables 
    }  
} 

May be you need to use Extract Class. It is common methodology for solving Feature Envy code smell (these are both taken from Martin Fowler's Refactoring).

About the many methods in one class - I reacall that I've read somewhere that the JVM will optimize additionaly your code, will inline methods, etc.

I have lengthy functions in VB.NET (VS2008) and I'd like to shrink them down. I know about the #region directives, but they don't work inside functions.

I was wondering if anyone knew of any plugins for visual studio 2008 that would allow me to fold if statements, loops and try catch statements. I've found a couple of plugins for Visual Studio 2005 but none for Visual Studio 2008.

You may want to read Martin Fowler's book called Refactoring: Improving the Design of Existing Code, as well as Code Complete: 2nd Edition.

refactoring bookcode complete book

If you're having functions and subroutines that long, it means there are larger complexity forces at work that need refactoring.

Update:

I just finished the book Clean Code by "Uncle" Bob Martin, and I have to say that it belongs right next to the other two in the 'Must Read' category.

Clean Code

I'm about to do some work on this hideous old java web app a friend on mine inherited a while ago.

After I've set up tomcat, imported project and all that to my eclipse workspace I get this error that a method in the servlet exceeds the 65536 bytes limit.

The method may very well exceed that limit, it's several thousand loc. But the thing is, I've worked on this app before without getting this error, and according to the commit logs, no code has been added to the servlet since.

Can it be because this time I'm working on a macbook? Last time I worked on the app I used a HP desktop with ubuntu. Different java version, cpu architecture? Is this even possible?

Is there anything I can do except refactor the code?

Is there anything I can do except refactor the code?

No. Refactor the code. No method should be that long. Ever. Write small methods!

Seriously: any IDE there is will guide you through the refactoring, but it needs to be done. You might also want to read Refactoring: Improving the Design of Existing Code for guidance.

I might inherit a somewhat complex multithreaded application, which currently has several files with 2+k loc, lots of global variables accessed from everywhere and other practices that I would consider quite smelly.

Before I start adding new features with the current patterns, I'd like to try and see if I can make the basic architecture of the application better. Here's a short description :

  • App has in memory lists of data, listA, listB
  • App has local copy of the data (for offline functionality) dataFileA, dataFileB
  • App has threads tA1, tB1 which update dirty data from client to server
  • Threads tA2, tB2 update dirty data from server to client
  • Threads tA3, tB3 update dirty data from in memory lists to local files

I'm kinda trouble on what different patterns, strategies, programming practices etc I should look into in order to have the knowledge to make the best decisions on this.

Here's some goals I've invented for myself:

  1. Keep the app as stable as possible
  2. Make it easy for Generic Intern to add new features (big no-no to 50 lines of boilerplate code in each new EditRecordX.cs )
  3. Reduce complexity

Thanks for any keywords or other tips which could help me on this project.

For making these kind of changes in general you will want to look at Martin Fowler's Refactoring: Improving the Design of Existing Code (much of which is on the refactoring website) and also Refactoring to Patterns. You also might find Working Effectively with Legacy Code useful in supporting safe changes. None of this is as much help specifically with multithreading, except in that simpler code is easier to handle in a multithreading environment.

In my project's core library we have a very big class, which is tending to become a God object. One of the reasons is that, over a period of time, tasks which should have been in different modules have been put into this class. For ex -

class HugeClass{
    public void DoModuleXJob(){}
    public void DoModuleYJob(){}
}

One of the problems in refactoring and moving the unwanted, module specific behavior out of this class is that it will be a lot of work for Module X and Module Y to change their code. As a work around I was thinking about converting these methods into extension methods and then move them to their concerned modules. For ex -

// in module X
static class HugeClassExtensions{
    public static void DoModuleXJob(this HugeClass instance){}
} 

// in module Y
static class HugeClassExtensions{
    public static void DoModuleYJob(this HugeClass instance){}
}

I found that this does not create any compilation problems, as long as Module Y is not using DoModuleXJob and vice-versa, which I am sure about.

Is this a good solution and are there any better methods in such a case?

I would suggest that you create the design as it should be, moving the functionality that is in HugeClass to the spot that it should be, and then leave the method call in HugeClass but have it defer to the functionality to where it was moved:

class HugeClass
{
    [Obsolete("Use ModuleX.DoModuleXJob() instead", false)]
    public void DoModuleXJob() {
        ModuleX mod = new ModuleX();
        mod.DoModuleXJob();
    }
}
class ModuleX
{
    public void DoModuleXJob() {
    }
}

In this way over time you are employing the Facade Pattern with HugeClass. The Obsolete attribute that I have applied to the HugeClass method means that every time something is compiled that calls the method in HugeClass a warning will be generated, pointing to where the new functionality is. The 'false' parameter in the attribute is what makes it a warning, this could be changed to true to make it an error. In this way you are not doing much extra work and it represents progress to where you want to be, which I don't believe your extension method technique would do necessarily. Over time the methods in HugeClass can be deleted until it is an empty class and itself can be deleted.

Incidently, if you have not read Martin Fowler's book Refactoring it is an excellent read. In it he discusses this and many other techniques.

In wanting to get some hands-on experience of good OO design I've decided to try to apply separation of concerns on a legacy app.

I decided that I wasn't comfortable with these calls being scattered all over the code base.

ConfigurationManager.AppSettings["key"]

While I've already tackled this before by writing a helper class to encapsulate those calls into static methods I thought it could be an opportunity to go a bit further.

I realise that ultimately I should be aiming to use dependency injection and always be 'coding to interfaces'. But I don't want to take what seems like too big a step. In the meantime I'd like to take smaller steps towards that ultimate goal.

Can anyone enumerate the steps they would recommend?

Here are some that come to mind:

  • Have client code depend on an interface not a concrete implementation

  • Manually inject dependencies into an interface via constructor or property?

  • Before going to the effort of choosing and applying an IoC container how do I keep the code running?

  • In order to fulfil a dependency the default constructor of any class that needs a configuration value could use a Factory (with a static CreateObject() method)?

Surely I'll still have a concrete dependency on the Factory?...

I've dipped into Michael Feathers' book so I know that I need to introduce seams but I'm struggling to know when I've introduced enough or too many!

Update

Imagine that Client calls methods on WidgetLoader passing it the required dependencies (such as an IConfigReader)

WidgetLoader reads config to find out what Widgets to load and asks WidgetFactory to create each in turn

WidgetFactory reads config to know what state to put the Widgets into by default

WidgetFactory delegates to WidgetRepository to do the data access, which reads config to decide what diagnostics it should log

In each case above should the IConfigReader be passed like a hot potato between each member in the call chain?

Is a Factory the answer?

To clarify following some comments:

My primary aim is to gradually migrate some app settings out of the config file and into some other form of persistence. While I realise that with an injected dependency I can Extract and Override to get some unit testing goodness, my primary concern is not testing so much as to encapsulate enough to begin being ignorant of where the settings actually get persisted.

Usually its very difficult to clean a legacy application is small steps, because they are not designed to be changed in this way. If the code is completely intermingled and you have no SoC it is difficult to change on thing without being forced to change everything else... Also it is often very hard to unit test anything.

But in general you have to: 1) Find the simplest (smallest) class not refactored yet 2) Write unit tests for this class so that you have confidence that your refactoring didn't break anything 3) Do the smallest possible change (this depends on the project and your common sense) 4) Make sure all the tests pass 5) Commit and goto 1

I would like to recommend "Refactoring" by Martin Fowler to give you more ideas: http://www.amazon.com/exec/obidos/ASIN/0201485672

I often find myself having a hard time trying to refactor some legacy code we have at work, because of the high complexity and all it's potential problems when touching that code. And to know exactly what that code does, takes a lot of time and effort, something I don't usually have.

So I was thinking, are there any patterns that could be used for this?, like, refactoring something without knowing what the entire code does?.

I've heard about some wrapper-like patterns, but some times they fall short.

Any ideas or best practices are welcome.

However you spin it, changing code that you do not fully understand is always a risk. However, there are systematic refactoring methods available that will make changes in the code less risky and here I'm mostly thinking about Refactoring: Improving the Design of Existing Code by Martin Fowler on Amazon.

Also see https://refactoring.guru for the catalogue of refactorings discussed in the book. I would strongly recommend to use unit tests while refactoring, and do it step by step in small increments, divide and conquer. If you read the book and study the catalogue, you will see opportunities for small, safe refactorings even in messy, hard to read code.

If you are looking for a pattern, the best pattern for this type of problem is probably the Facade pattern where you dress up the legacy in nice clothes (the facade) and your client code communicates with the facade which in turn communicates with the sub system (legacy).

It depends.

a) Sure, but since the entire project gets turned into one .dll, does it really matter from a performance standpoint if you have 7 controllers with 300 lines of code each versus one controller with 2100 lines? (Obviously readability and tight coupling are problems if there is no separation of concerns)

b) As long as separation of concern is taken into consideration, is the number of lines of code in one controller not an issue?

c) How many lines of code in one controller can be a sign of needing to refactor? (500,1000,5000,10000)?

Any fool can write code that a computer can understand. Good programmers write code that humans can understand. --Martin Fowler [ source]

I remember seeing an application in classic asp that was a single page. It rendered differently based on the parameters that came in. Theoretically, you could do the same thing with ASP.NET MVC, make a single controller with a ton of views (or worse a single view that changed based on input to the controller).

Granted, that super controller divides its work up into actions. But regardless, you're still working with something big.

The first SOLID Principle is Single Responsibility. Your module should have only one reason to change. A large controller tells me two things:

  1. It is involved with too much
  2. The actions have too much logic

Both of these are SRP violations because the Controller that does too much has more than one reason to change. The actions that do too much have to change when the logic changes or when the objects they interact with change.

To me a Controller Action should just call a function on a Service (interface, web service, whatever) and return the results of that call. Most of my controller actions are a whopping two lines of code. And most of my controllers are very focused in what they do.

As a rule of thumb, I get uncomfortable when I have to scroll more than two pages for a single class (about 200 lines depending on your font size and/or resolution). Even more important than actual length is how much responsibility my controller has. Minimize the responsibility and file size decreases automatically.

I think I am very close to assembling an MVC repository correctly but just falling apart at the fringe. I created an MVC project with a repository and am returning data successfully, but not accurately as it pertains to DDD. Please tell me where I am incorrect in terms of strict DDD assembly. I guess if the topics are too wide, a book suggestion would be fine. I hope that I am specific enough in my question.

This was one question but I separated them for clarity: Do you create a single namespace for all repository classes called MyStore.Models? Create a repository class for each entity like Product within the Models namespace? Do you put the Pocos in their own classes in the Models namespace but not part of the Repository class itself?

I am currently using Pocos to cut out entities from Linq statements, returning groups of them within IQueryable wrappers like so. I guess here you would somehow remove the IQueryable and replace it with some type of Lazy load? How do you lazy load without being dependent on the original Linq to Sql?

public IQueryable<Product> GetProducts(...) {
return (from p in db.Products
        where ...
        select new myProductPoco {  // Cut out a Poco from Linq
            ID = p.ID,
            Name = p.Name,
            ...
        });
}

Then reference these in MVC views within the inherit page directive:

System.Web.Mvc.ViewPage<IQueryable<MyStore.Models.Product>>

However, the nested generics looks wrong. I assume this requires a re-factor. Where do you define View Model classes that contain references to Entities? Within the controller class (nested class)?

As book suggestions, try Eric Evan's Domain-Driven Design, and maybe Martin Fowler's Refactoring.

I would some ressources for learning software design using uml (Case study, books)

"UML distilled" by Martin Fowler is a very good introduction to UML.

Background: I have a large (several hundred lines) class that manages a concept based on some primitive-type data structures

long[] slist;  //list of unique patterns (related to polyominoes)
int[][][] sref;//patterns at each place [location][depth][<list of indices in slist>]

Question: The two methods that populate and update these data are going to be quite long, with handfuls of 5-20 line tasks, some shared, others unique. I probably want to make a helper method for each sub-task.

update(...){
    //do A
    //do B
    //do C
    //...
}
build(){
    //do D
    //do B
    //do E
    //...
}

The problem is if there are too many unrelated helper methods in one file, readability does not improve.

The answer to this question gets me most of the way there. I can declare the structures in the same package, in their own class, and access the primitive member field or call related methods. But I still want to know the accepted wisdom here because this organization did not come easily to mind.

Would you ever go so far as to put update() and build() functions in their own files? If so, where should their common tasks be declared?

I highly recommend reading Refactoring (Amazon link) by Martin Fowler; it should be in every programmer's library and will help you with situations like this. I will refer to it in the post.

If a class has too much code, then it's usually time to split up the class. That may entail creating member variables that have classes (delegating functionality), or it may mean creating an object on the fly (replace method with method object). Things that share commonalities are good cases for applying inheritance or the state/strategy pattern.

Short answer

Yes, you would go so far as to have those functions in their own files. However, I would instead make them classes. Perhaps Updater and Builder objects. You can inherit from BuilderCommon and UpdaterCommon classes. These new objects will be coupled to the old object, but that's okay. You may consider putting these new sets of classes in their own package. Hierarchical organization will help with readability and reuse of common code. Try to take advantage of concepts like inheritance and abstraction techniques like generics to do the work for you. If you can find commonality between doA, doB, etc., make UpdateHelper classes out of them and put them in a list. Then simply iterate over the list.

This is just one of the many ways to do it:

public class Updater
{
    public Updater(List<IUpdateHelper> helpers)
    {
        helpers = new ArrayList<UpdateHelper>();
        this.helpers.add(helpers);
    }
    public void update()
    {
        for (IUpdateHelper helper : helpers)
        {
            helper.performHelp();
        }
    }

    protected List<IUpdateHelper> helpers;
}

public class UpdaterCommon extends Updater
{
    public UpdaterCommon()
    {
        helpers.add(new UpdateHelperA());
        ... // Etc.
    }
}

/*
 * This uses inheritance for common helpers, but it could just as well use
 * delegation. Also, this assumes that order of invocation for each helper 
 * doesn't matter.
 */
public class UpdaterOne extends UpdaterCommon {...}

interface IUpdateHelper
{
    public void performHelp();
}
public class UpdateHelperA implements IUpdateHelper {...}

Definitely replace those arrays with objects and add an interface.

Closing

In my experience it usually only takes the application of a few of these concepts on a regular basis to make a significant difference in code quality. If a package, class, method, conditional, etc. gets to be unruly, break it out into a smaller unit. Keep pushing the nitty-gritty functionality down into very small methods so that you can look at the code from a high level.

What are your .NET code-refactoring best practices?

I recommend actually reading the book. "Refactoring: Improving the Design of Existing Code", by Martin Fowler.


We are a student team maintaining a mobile app written mainly in javascript (also jquery) and PHP. We recognized that we have to improve the structure of our code and have to implement structures and workflows within the Team.

Status of the code:

By now the code is still very tangled, not loosely coupled.

Classnames with identical names in different folders.

Wide usage of == instead of ===.

Wide usage of global variables.

We reinvented the wheel for parts of the code. (where frameworks already existed for certain purposes)

Status Methods/Tools:

We are now implementing SCRUM

We use Redmine Backlogs which works good for us.

We implemented a usable branching model for git. (http://nvie.com/posts/a-successful-git-branching-model/)

We started to use jsdoc to do an inline documentation of the code.

We stumbled across SonarQube (a code quality checking tool) and think this might be helpful.

We are still new to TDD and skeptic about it. We do not have expertise in designpatterns but would like to apply them. Only few in our team of round about 6 people know the basic concepts of MVC, MVV, MVP.


We want to start with refactoring our code (to unobtrusive js?) around the first week of september.

Which steps would you suggest so that we don't stumble too much and that the process is kept transparent and motivation is high?

Would you start by fixing little issues like the == / === thing? (things SonarQube comes up with) Would you start with implementing a TDD framework but without having a testable MVC/MVV ? Would you start with separating js from html and css? Would you start with bugfixing of old, not yet resolved tickets to have absolutely clean branches?

Thank you very much for any suggestions/ideas/best practices

Marc

Well .. step nr.1: read this book. You have time enough for that till September.

Watch following lectures:

These materials should give you some sense of the subject.

As for real first steps, separating the HTML from JavaScript is a nice place to begin. If you know how to do event delegation in javascript - good. If not, look into it.

Then you can move on to fixing the part of page that spews out the HTML. Separate the SQL, add some abstraction, some OOP principles.

If you end up aiming at something MVC-shaped, then this list of links might help a bit.

Currently I'm trying to write a good commit comment for a code refactoring I've made. And I feel like I'm missing a word to sum up what I did instead of describing it.

Right now my description is: "Code refactor to improve decoupling inside the generator class."

But IMO, it's not really decoupling as it's only inside a class itself. It doesn't have any strong link to the code responsibility. It's more to improve testability of the class by having more smaller methods instead of few big one.

So that led me to a quite simple question:
What are the most common terms you use in your commit messages to describe a code refactoring ?

http://www.amazon.com/Refactoring-Improving-Design-Existing-Code/dp/0201485672

Is the defacto standard for basic refactoring vocabulary everywhere I've worked.

I am a newbie in software engineering and I want to specialize in C++. Now, I know C++ syntax very well and use Qt. But I don't have any knowledge about UML, design patterns or the Boost Library. Which library or topics do you recommend I learn first?

Learning syntax is like learning a grammar that is a stepping stone to become a poet. To become a poet you need to know how to form sentences that are easy and catchy.

Similarly for any language you should know how to design a system that is simple and maintainable. I suggest you to learn OOP Concepts, Design Patterns, and Refactoring

I am an intermediate javaScript programmer, and i am interested in expanding my knowledge in object oriented programming (especially object oriented JavaScript).

I would prefer a book over browsing scattered web resources, does anyone have an idea of which book will be best to get a head start with?

Thanks

Definitely worth reading: JavaScript: The Good Parts by Douglas Crockford.

The chapter 5, "Inheritance" covers different types of Object Orientation:

  • Pseudoclassical, simulating Class hierarchies by extending prototype objects with new methods
  • Prototypal, without classes, using prototype objects to create new instances with common functionalities, then attaching new functions to these new objects directly
  • Functional, using constructor functions to create a private scope and return a new object grouping a set of methods with privileged access to the private variables

Refactoring: Improving the Design of Existing Code by Martin Fowler while the examples are given in Java, the principles behind it are applicable to most OO languages including JavaScript.

I'd like to improve my coding skills and use the best programming approach to be a better programmer. Will someone recommend any website / platform for finding a code reviewer for checking and improving my code?

In addition, I would suggest that you read available good books that will help you write maintainable code. To start with, you may refer Clean Code, Refactoring for Software Desing Smells, and Refactoring: Improving the Design of Existing Code.

I have a method that is over 700+ lines long. In the beginning of the method, there are around 50 local variables declared. I decided to take the local variables out and put them into a separate class as properties so I could just declare the class in the method and use the properties through out it. Is this perfectly fine or does another data type fit in here such as a struct? This method was written during classic ASP times.

I would strongly recommend you take a read through this book and/or any number of web sites concerned with refactoring. http://www.amazon.com/Refactoring-Improving-Design-Existing-Code/dp/0201485672

Although you can't look at 700+ lines in a single method and automatically say that is bad, it does indicate a bad code smell. Methods should be small units of code with a single purpose. This makes it easier for you to maintain or those who come behind you. It can also help you to figure out improvements to your design and make altering your design in the future much easier.

Creating a class just to hold properties without looking at what the overall structure should be is just hiding a problem. That is not to say in this particular instance that is not a perfectly acceptable and correct solution, just that you should make sure you are taking the time to provide a properly thought out design where your classes have the properties, state, and functionality they deserve.

Hope this helps.

I have heard that Martin Fowler said in his book [Refactoring, Improving the design of existing code] (which i haven't read personally), that Enumerations are generally bad choice and they should be replaced by using Polymorphic classes, and that really confused me. aren't enumerations supposed to be the great departure from using old integer and bit constants. so my questions are:
1. When should we use enumerations ?
2. When should not ?
3. A good example of this conversion process ?

Enumerations are to be used in very simple cases, like data mapping, when you want the value to be defined only among a set of defined values.

Usually, people use enumerations to branch their logic based on different values. This is usually a poor design, as it would be better to put the different bits of logic into different classes (that may potentially extend the same superclass, or better, implement the same interface).

If you find yourself writing code that looks like this (pseudo-code)

if (myValue ==  MyEnum.VALUE1) {
    doThis;
else if (myValue == MyEnum.VALUE2) {    
    doThat;
}

Then a better pattern is to use some kind of Strategy design pattern, or in some cases a Visitor. This is more object-oriented.

I have several pages of code. It's pretty ugly because it's doing a lot of "calculation" etc. But it contains of several phases, like many algorthims, like that:

  1. calculate orders I want to leave
  2. kill orders I want to leave but I can't leave because of volume restrictions
  3. calculate orders I want to add
  4. kill other orders I want to leave but I can't because of new orders
  5. adjust new orders ammount to fit desired volume

Totally I have 5 pages of ugly code which I want to separate at least by stage. But I don't want to introduce separate method for each stage, because these stages make sense only together, stage itself is useless so I think it would be wrong to create separate method for each stage.

I think I should use c# #region for separation, what do you think, will you suggest something better?

Avoid #region directives for this purpose, they only sweep dirt under the carpet.

I second @RasmusFranke's advice, divide et impera: while separating functionalities into methods you may notice that a bunch of methods happen to represent a concept which is class-worthy, then you can move the methods in a new class. Reusability is not the only reason to create methods.

Refactor, refactor, refactor. Keep in mind principles like SOLID while using techniques from Refactoring and Working Effectively with Legacy Code.

Take it slow and use if you can tools like Resharper or Refactor! Pro which help to minimize mistakes that could occur while refactoring.

Use your tests to check if you broke anything, especially if you do not have access to the previously mentioned tools or if you are doing some major refactoring. If you don't have tests try to write some, even if it may be daunting to write tests for legacy code.

Last but not least, do not touch it if you don't need to. If it works but it is "ugly" and it is not a part of your code needing changes, let it be.

I have some legacy code that is all but clear, clean and concise. Much of that legacy logic lies in using arrays. Like this code here:

if (isset($statistics[$lowerInstance])) {
        $statistics[$lowerInstance][$size]['count'] +=     $items[$lowestType]['count'];
        $statistics[$lowerInstance][$size]['amount'] += $items[$lowestType]['amount'] + (($items[$highestType]['amount'] / $items[$highestType]['count']) * $items[$lowestType]['count']);
    } else {
        $statistics[$lowerInstance][$size]['count'] = $items[$lowestType]['count'];
        $statistics[$lowerInstance][$size]['amount'] = $items[$lowestType]['amount'] + (($items[$highestType]['amount'] / $items[$highestType]['count']) * $items[$lowestType]['count']);
    }
if (isset($statistics[$higherInstance])) {
        $statistics[$higherInstance][$size]['count'] += $items[$highestType]['count'] - $items[$lowestType]['count'];
        $statistics[$higherInstance][$size]['amount'] += $items[$highestType]['amount'] - (($items[$highestType]['amount'] / $items[$highestType]['count']) * $items[$lowestType]['count']);
    } else {
        $statistics[$higherInstance][$size]['count'] = $items[$highestType]['count'] - $items[$lowestType]['count'];
        $statistics[$higherInstance][$size]['amount'] = $items[$highestType]['amount'] - (($items[$highestType]['amount'] / $items[$highestType]['count']) * $items[$lowestType]['count']);
    }

This is only a fraction of this particular method, it goes on and on.

I'm having some difficulty how to make it clearer and easier to work with. It is not as easy (I think) to just move multidimensional array to an ArrayAccess type object although that might be a slight improvement.

Is there some generic way of refactoring multidimensional arrays, or some examples of how it might be done? Not just on this particular problem but a more generic way to handle PHP multidimensional array hell?

I took some help from Martin Fowler's book, Refactoring: Amazon.

Replace Array with Object

So I read what he wrote very carefully and decided to have a go at it. I started to create a unit test for the method. Just a simple one where I would pass a bunch of data, export the outcome and use that outcome in the assertions. I did this before refactoring so I could run the unit test after each iteration to know if I had broken anything.

First order of business according to Fowler is to create a class with a public array that will replace the offending array:

class Statistics
{
    public $data = [];
}

I created the variable:

$statistics = new Statistics();

Then I replaced all instances of the variable, $statistics became $statistics->data

So for example:

$statistics[$higherInstance][$size]['count'] += $items[$highestType]['count'] - $items[$lowestType]['count'];

would become:

$statistics->data[$higherInstance][$size]['count'] += $items[$highestType]['count'] - $items[$lowestType]['count'];

This was interesting because it opened up to some new possibilities. What I did next was to refactor out some of the code into their own methods:

protected function getInstanceData(Statistics $statistics, $higherInstance, $size, $highestType, $items) {
    $statistics->data[$higherInstance][$size]['count'] += $items[$highestType]['count'] - $items[$lowestType]['count'];
    ...
    return $statistics->data;

Then I would move that method into the Statistics class itself:

public function getInstanceData($higherInstance, $size, $highestType, $items) {
    $this->data[$higherInstance][$size]['count'] += $items[$highestType]['count'] - $items[$lowestType]['count'];
    ...
    return $this->data;

I did so with several chunks of the code. I didn't particularly like the parameters but for now, I think it was an improvement, the code was atleast a little bit less noisy.

I removed the return $this->data from all the methods as it was no longer necessary, the array was now contained within the Statistics class.

This opened up for further refactorings, breaking up the array further and delegating some of the responsibilities to new classes.

I think this is probably how I will deal with these type of arrays from now on, isolate them to classes and then break them up.

I have written a class that reads in data from external files, manipulates the data, and then does a calculation.

The class is very long, particularly the constructor. This is where I read in data from external files and manipulate it in preparation for the calculation. I use template functions to do this.

I know that it would be better style to split the class up, but I am using it in combination with commercial code that I can't change. Also, people who will need to use the code need it to be one class + auxiliary files, so I'm restricted to this one class. To make the code more readable, I would like to store some of the code from the constructor in helper .cpp/.h files called "auxiliary.h/.cpp" and access it through functions.

Here's the problem:

1) Passing the template functions (member functions of the class) as function arguments to the auxiliary files. I can't do this, although I have heard it might be possible through something called "disambiguation." It is no problem to pass non-template functions and vectors and things like that... The template functions are the problem.

2) Also, even if I could pass the template functions as arguments to the functions that access the auxiliary files, my argument lists end up very long.

What I would like to know is, is there some way to make the auxiliary files "see" the class? I tried passing an INSTANCE of the class as an argument/parameter in each function that does stuff in the auxiliary files. But this results in "instanceName not declared in this scope" and it does seem like a rather circular/convoluted approach.

Any advice would be very appreciated. Thanks.

I know that it would be better style to split the class up, but I am using it in combination with commercial code that I can't change...

I may be missing more background to your specific situation but regardless of what third-party code you are using, you should be able to do whatever you want with your code that you wrote and it sounds like you wrote a lot.

You recognized your class is too big so next step is to break it apart into separate, more cohesive classes. Attempting to create other files/functions that simply modify the internal state of your one class that's already big, will make the code even bigger and you will spaghetti it up even more than it is now.

If the third-party library needs to operate with a single class that you writing, take a look at Facade design pattern. You can easily have what "looks like" a complex class to the outside, but internally it simply delegates the work any number of more specialized classes.

If you want one class to have access to a third-party library, you can do that too. Take a look at something such as Adapter design pattern.

Another advice I can offer is pick up this book on refactoring. It'll show you how you can take any code that looks way too complex and methodically apply steps to make it much more readable and maintainable. For example take a look at your data members, do any of them look like they belong to one group (i.e. they are often accessed/modified together)? If yes, consider moving them into their own class which provides minimal interface for getting/setting only what your main class needs.

This post doesn't answer your specific question, but take a step back, maybe there's a much better solution than breaking OO rules and good design principles just because of some constraints which may not really be there.

I'm reading "Refactoring" by Martin Fowler.

There is a term "type code" that I've never seen that before.

What is a type code?

One context in which a type code can appear is in C with a union type:

typedef enum Type { T_INT, T_FLOAT, T_DOUBLE, T_LONG } Type;

typedef struct Datum
{
    Type type;
    union
    {
        int     i;
        float   f;
        long    l;
        double  d;
    } u;
} Datum;

This leads to code like:

Datum v;

switch (v.type)
{
case T_INT:
    int_processing(v.u.i);
    break;
case T_FLOAT:
    float_processing(v.u.f);
    break;
case T_DOUBLE:
    double_processing(v.u.d);
    break;
}

Now, was the omission of T_LONG from the switch deliberate or not? Was it recently added and this switch didn't get the necessary update?

When you get a lot of code like that, and you need to add T_UNSIGNED, you have to go and find a lot of places to correct. With C, you don't have such a simple solution as 'create a class to represent the type'. It can be done, but it takes (a lot) more effort than with more Object Oriented languages.

But, the term 'type code' refers to something like the Type type in the example.

I'm refactoring a C++ code doing kmean clustering. There are two version of the code:

  1. Normal kmean
  2. Feature aligned clustering: use slightly different distance metric and update rule.

How should I implement this effectively? Method overloading (this doesn't depend on input though)

OK so this is the pseudocode doing kmean clustering. I need to implement two versions of distance and update function.

int* kmean_clustering(vector<double[10]> data)
// each row of data hold one data point
// so we have M data points of 10 dimension
{
    // Split codebook
    // Assignment step
    Find the closest codebook based on distance(distance here can be Euclidean, Mahalanobis, .....)
    //Update step
}

A well written code is like reading a good novel. That's why Knuth calls it as an art of programming. This book might teach you bottom-up approach of refactoring.

I'm self taught. I'm currently doing most of my work in Rails. Sometimes I find it difficult to solve complex programming problems, as I'm sure a lot of us do. What would be a good subject or book to study to improve programming solving skills?

Is there a specific book about the matter? Maybe something like math, algebra, calculus? General computer science? A book like this http://pragprog.com/book/ahptl/pragmatic-thinking-and-learning ? General OOP?

I have over 20 years of programming experience, and in my experience some good ways to improve your programming skills are (not in any order of priority)

a) Solve complex programming problems

b) Revisit your solutions to see where improvements can be made (2-3 passes at least).A good book with tips to improve your programs is refactoring: http://www.amazon.com/Refactoring-Improving-Design-Existing-Code/dp/0201485672

c) Dr. Dobbs is an excellent site to get tips and insight: http://www.drdobbs.com/

e) Look at other people's code, eg. open source code so you don't develop a frog in the well mindset. This is a great way to learn good practices.

f) Learn to program in multiple languages (eg Java, PHP). This is also a great way to improve skills.

g) Try and always think 'best practices' when writing code. HTH.

I read that you can use labels to control which loops to break and continue if you have nested loops. I tried doing that in the code below, but when the outer loop is supposed to break when the cabinet is selected, it doesn't and just loops back the dining room loop, only breaking the kitchen loop.

//Same imports used from the original code as needed.
import java.net.MalformedURLException;
import java.util.Scanner;
import javax.swing.JOptionPane;
import java.net.URL;
import javax.swing.ImageIcon;

public class LoopyHauntedHouse {

private String name; //Player name.
private String location0; //First location selected from door.
private String toFrontDoor = "";
private String atFrontDoor;

//Method asking for user's name, and welcoming the user as well as asking where they'd like to go for the first time.
public void intro()throws MalformedURLException
{
    //URL to image initialized from the original code as needed.
    URL frontDoor = new URL("http://i.imgur.com/2m3giQk.png");

    //Scanner for user input.
    Scanner scnr = new Scanner(System.in);

    //Asking for user's name and welcoming the user.
    name = JOptionPane.showInputDialog(null, "What is your name?");
    JOptionPane.showMessageDialog(null, "Welcome " + name + " to the Haunted House!");

    //Shows starting location.
    JOptionPane.showMessageDialog(null, name + " you're at the front door of the haunted house.", "Title",
    JOptionPane.PLAIN_MESSAGE, new ImageIcon(frontDoor));

    //Asks for first choice of room to start at.
    location0 = JOptionPane.showInputDialog(null, name + " where to next? 'Living Room', 'Dining Room' or 'Stairs'?");
}

//Method for the rest of the program allowing users to walk through the house, backtrack, and interact with objects.
public void startWalking()throws MalformedURLException
{   
    //URLs to images are initialized from the original code as needed.
    URL stairs = new URL("http://i.imgur.com/WuddJUc.png");
    URL bedroom1 = new URL("http://i.imgur.com/HZ6OSyZ.png");
    URL bedroom2 = new URL("http://i.imgur.com/JZORNpd.png");
    URL bathroom = new URL("http://i.imgur.com/onSEc1J.png");
    URL masterBedroom = new URL("http://i.imgur.com/bf4L0sH.png");
    URL masterBathroom = new URL("http://i.imgur.com/yp87dTX.png");
    URL livingRoom = new URL("http://i.imgur.com/7XQD5Pt.png");
    URL bathRoom = new URL("http://i.imgur.com/G0CxjSy.png");
    URL diningRoom = new URL("http://i.imgur.com/gyU9mep.png");
    URL kitchen = new URL("http://i.imgur.com/tTMRCID.png");
    URL pantry = new URL("http://i.imgur.com/zBxPJCs.png");

    while(location0.equalsIgnoreCase("Living Room")||(toFrontDoor.equalsIgnoreCase("Living Room")))
    {
        JOptionPane.showMessageDialog(null, name + " you are now in the Living Room");
        String move1 = JOptionPane.showInputDialog(null, name + " would you like to explore the 'Chest' walk to the 'Bathroom' or 'Go back' and go to another room?");

            if(move1.equalsIgnoreCase("Chest"))
            {
                JOptionPane.showMessageDialog(null, name + " a ghost escapes and scares you to death!");
                JOptionPane.showMessageDialog(null, "Game Over! You've died.", "Try Again.", 
                JOptionPane.WARNING_MESSAGE, new ImageIcon(livingRoom));
                break;
            }

            else if(move1.equalsIgnoreCase("Bathroom"))
            {
                while(move1.equalsIgnoreCase("Bathroom"))
                {
                JOptionPane.showMessageDialog(null, name + " you are now in the bathroom.");
                String move2 = JOptionPane.showInputDialog(null, name + " would you like to explore the 'Mirror', 'Shower', or 'Go back'?");
                    if(move2.equalsIgnoreCase("Shower"))
                    {
                        JOptionPane.showMessageDialog(null, name + " the room suddenly steams up and you feel fingers touching the back of your neck...");
                    }
                    else if(move2.equalsIgnoreCase("Mirror"))
                    {
                        JOptionPane.showMessageDialog(null, name + "you see a bloody face looking back at you!");
                    }
                    else if(move2.equalsIgnoreCase("Go back"))
                    {
                        break;
                    }
                    else
                    {
                        JOptionPane.showMessageDialog(null, "Please enter a valid option.");
                    }
                }
            }
            else if(move1.equalsIgnoreCase("Go back"))
            {
                toFrontDoor = move1;
                break;
            }
            else
            {
                JOptionPane.showMessageDialog(null, "Please enter a valid option.");
            }
    }

    outerloop: while(location0.equalsIgnoreCase("Dining Room"))
    {
        JOptionPane.showMessageDialog(null, name + " you are now in the Dining Room");
        String move1 = JOptionPane.showInputDialog(null, name + " would you like to explore the 'Candelabra', walk to the 'Kitchen', or 'Go back'?");

        if(move1.equalsIgnoreCase("Candelabra"))
            {
                JOptionPane.showMessageDialog(null, "The candelabra light up by themselves and " + name + " sees a death shadow!");
                JOptionPane.showMessageDialog(null, "Game Over! You've died.", "Try Again.", 
                JOptionPane.WARNING_MESSAGE, new ImageIcon(diningRoom));
                break;
            }

        else if(move1.equalsIgnoreCase("Kitchen"))
        {
            while(move1.equalsIgnoreCase("Kitchen"))
            {
                JOptionPane.showMessageDialog(null, name + " you are now in the 'Kitchen'.");
                String move2 = JOptionPane.showInputDialog(null, name + " would you like to explore either the 'Refrigerator', the 'Cabinet', walk to the 'Pantry', or 'Go back'");
                    if(move2.equalsIgnoreCase("Refrigerator"))
                    {
                        JOptionPane.showMessageDialog(null, name + " opens the refrigerator and finds some delicious soul food.");
                    }
                    else if(move2.equalsIgnoreCase("Cabinet"))
                    {
                        JOptionPane.showMessageDialog(null, "The dished and glasses start flying at you as soon as you open the door. " + name + " gets hit in the head and feels themselves moving towards a light.");
                        JOptionPane.showMessageDialog(null, "Game Over! You've died.", "Try Again.", 
                        JOptionPane.WARNING_MESSAGE, new ImageIcon(kitchen));
                        break outerloop;
                    }
                    else if(move2.equalsIgnoreCase("Pantry"))
                    {
                        while(move2.equalsIgnoreCase("Pantry"))
                        {
                            JOptionPane.showMessageDialog(null, name + " you are now in the Pantry.");
                            String move3 = JOptionPane.showInputDialog(null, name + " would like to explore the 'Dusty Recipe Box', the 'Broom', or 'Go back'?");
                                if(move3.equalsIgnoreCase("Dusty Recipe Box"))
                                {
                                    JOptionPane.showMessageDialog(null, name + "opens it up and a recipe for chocolate devils food cake appears out of no where.");
                                }
                                else if(move3.equalsIgnoreCase("Broom"))
                                {
                                    JOptionPane.showMessageDialog(null, "As soon as " + name + " touches the broom, it flies up in the air!");
                                }
                                else if(move3.equalsIgnoreCase("Go back"))
                                {
                                    break;
                                }
                                else
                                {
                                    JOptionPane.showMessageDialog(null, "Please enter a valid option.");
                                }
                        }
                    }
                    else if(move2.equalsIgnoreCase("Go back"))
                    {
                        break;
                    }
                    else
                    {
                        JOptionPane.showMessageDialog(null, "Please enter a valid option.");
                    }
            }
        }
        else if(move1.equalsIgnoreCase("Go back"))
        {
            toFrontDoor = move1;
            break;
        }
        else
        {
            JOptionPane.showMessageDialog(null, "Please enter a valid option.");
        }
    }

    while(location0.equalsIgnoreCase("Stairs"))
    {
        JOptionPane.showMessageDialog(null, name + " you are now in font of the stairs.");
        String move1 = JOptionPane.showInputDialog(null, name + " would you like to walk to 'Bedroom 1', 'Bedroom 2', the 'Master Bedroom', or 'Go back'?");

        if(move1.equalsIgnoreCase("Master Bedroom"))
        {
            while(move1.equalsIgnoreCase("Master Bedroom"))
            {
                JOptionPane.showMessageDialog(null, name + " you are now in the Master Bedroom.");
                String move2 = JOptionPane.showInputDialog(null, name + " would you like to explore the 'Jewelry Box', walk into the 'Master Bathroom', or 'Go back'?");

                if(move2.equalsIgnoreCase("Jewelry Box"))
                {
                    JOptionPane.showMessageDialog(null, name + " you find the cursed Hope Diamond and feel your doom.");
                }
                else if(move2.equalsIgnoreCase("Go back"))
                {
                    break;
                }
                else if(move2.equalsIgnoreCase("Master Bathroom"))
                {
                    while(move2.equalsIgnoreCase("Master Bathroom"))
                    {
                        JOptionPane.showMessageDialog(null, name + " you are now in the Master Bathroom.");      
                        String move3 = JOptionPane.showInputDialog(null, name + " would you like to explore the 'Intricate Oil Lamp', the 'Shower', or 'Go back'?");

                        if(move3.equalsIgnoreCase("Intricate Oil Lamp"))
                        {
                            JOptionPane.showMessageDialog(null, name + " you rub the lamp and a genie pops out who says he'll grant you 3 wishes!");
                        }
                        else if(move3.equalsIgnoreCase("Shower"))
                        {
                            JOptionPane.showMessageDialog(null, name + " you suddenly hear singing in the shower but no one is there.");
                        }
                        else if(move3.equalsIgnoreCase("Go back"))
                        {
                            break;
                        }
                        else
                        {
                            JOptionPane.showMessageDialog(null, "Please enter a valid option.");
                        }
                    }
                }
                else
                {
                    JOptionPane.showMessageDialog(null, "Please enter a valid option.");
                }
            }
        }
        else if(move1.equalsIgnoreCase("Go back"))
        {
            toFrontDoor = move1;
            break;
        }
        else if(move1.equalsIgnoreCase("Bedroom 1"))
        {

        }
        else if(move1.equalsIgnoreCase("Bedroom 2"))
        {

        }
        else
        {
            JOptionPane.showMessageDialog(null, "Please enter a valid option.");
        }
    }
}   

public void toFrontDoor() throws MalformedURLException
{
    if(toFrontDoor.equalsIgnoreCase("Go back"))
    {
        atFrontDoor = JOptionPane.showInputDialog(null, name + " where to next? 'Living Room', 'Dining Room', 'Stairs', or 'Leave the house'?");
            if(atFrontDoor.equalsIgnoreCase("Leave the house"))
            {
                JOptionPane.showMessageDialog(null, "Game Over! Thanks for playing.");
            }
    }
    else
    {
        startWalking();
    }
}
}

Your understanding of labels is correct. The break outerloop statement should break the outer loop, as demonstrated by this example:

public class Example {

    public static void main(String[] args) {
        outerLoop: while (true) {
            int i = 0;
            while (true) {
                System.out.println(i++);
                if (i > 5) {
                    break outerLoop;
                }
                if (i > 10) {
                    break;
                }
            }
            System.out.println("Broken inner loop");
        }
        System.out.println("Broken outer loop");
    }
}

Which outputs:

0
1
2
3
4
5
Broken outer loop

I suspect that the break outerloop statement in your code isn't actually being run. Or there's another loop that isn't visible in the code you posted that causes the outerloop to be run again?

I'd suggest simplifying the code (e.g. by extracting some methods) and/or stepping through it with a debugger. At 142 lines your startWalking() method is starting to become unwieldy. If you keep your methods below 10 lines (the book Refactoring by Martin Fowler explains how) it makes things a lot easier to see the logic (and bugs in the logic).

I find writing a simpler example to demonstrate the problem is often a good way to solve these kinds of weird issues. If you find the simple example works, you can gradually add back in the complexity until it breaks again and then you know exactly what caused it.

I recently started learning test-driven development but I always see myself changing the design of my class so that higher level class can access some more properties. (After changing design, my test cases will have to be rewritten too)

For example, I am practicing this by writing a MineSweeper program. The MineFieldImp class has a width and a height property, but I didn't expose this in the MineField interface I created earlier. But later on I find out I need this features, so adding these properties requires me to add public methods to this class as well as the interface it's implementing.

I constantly see myself adding methods or fields to a class that I didn't think of earlier. What am I doing wrong? What should I do to improve?

Thanks, Yang

You aren't doing anything wrong. Class design is an constantly evolving process, its constantly changing. That's the beauty of TDD and unit testing. As your design changes you have your unit tests to assure you that your re-factoring hasn't broken anything.

I'd highly suggest reading Fowler's Refactoring book, there's lots of great information in there, including many useful refactoring patterns and guidance on how to refactor. It will definately help you be more productive and help you produce better object oriented designs.

So my class under test has code that looks braodly like this

public void doSomething(int param)
{
    Report report = new Report()
    ...do some calculations
    report.someMethod(someData)
}

my intention was to extract the construction of report into a protected method and override it to use a mock object that I could then test to ensure that someMethod had been called with the right data.

So far so good. But Report isnt under my control, and to mkae things worse it uses JNI to load a library at runtime.

If I do Report report = EasyMock.createMock(Report.class)

then EasyMock attempts to use reflection to find out the class members, but this causes an attempt to load the JNI library, which fails (the JNI libraries are only available on UNIX).

Im considering two things: a) Introduce a ReportWrapper interface with two implementations, one of which will delegate calls to an real Report (so basically a Proxy), and a second which will basically use a mock object. or b) instead of calling someMethod, call a protected method which will in turn call someMethod that I can override in a testing subclass.

Either way it seems nasty. Any better ways?

If there's no interface to Report class, then your suggested wrapper is the correct approach.
The book "Refactoring: Improving the Design of Existing Code" has a chapter about extracting interfaces from badly designed classes.

If you're using some DI framework (e.g. SpringFramework) you can easily replace this object with some ObjectFactory to create the correct implementation (mock vs. real one).

What’s the issue with the following, and how would I implement it better using OO principles?

My app contains a bunch of shape classes which all inherit from Shape - Circle, Rectangle, Triangle, etc. Some of these need to be displayed on the screen, in which case they need to leverage common screen logic, so there's a ScreenShape superclass which contains the common logic, and ScreenCircle, ScreenTriangle child classes.

About what you posted in response to @DOC:

@DOK the manager is not satisfied...although he gave me a go ahead with the design but still he said you should research on this.

I suppose the reason your manager is not satisfied with the current design is because it's suffering from a code smell called Parallel Inheritance Hierarchies. Basically, it describes what you're experiencing: every time a new subclass is created, you have to create its counterpart subclass in the parallel hierarchy. That would make your design more difficult to change, and thus, maintain.

Current Answers Review

I'd like to comment some things I didn't agree on the current answers. Basically you're suggested to either to:

  1. Inherit from a ScreenShape class
  2. Implement drawing logic in each subclass of Shape

I can see two issues going with option n° 1:

  • It defines Shape as an interface, so you'll have to re-implement it in each subclass (Rectangle, etc.)
  • To overcome the previous you could make ScreenShape implement it, because all kind of shapes will inherit from it. This is bad, and @Perception answer explains why.
  • Suppose you are asked now to export shapes to an XML File (this is another operation on shapes, just as is displaying them). Following this approach you'll need to implement that behavior on an XMLShape class and inherit from it, but you're already inheriting from ScreenShape. See the problem?

Option n° 2 forces you to bloat your domain model with presentation concerns, again just as @Perception said :)

Some Goals to Achieve

From the previous, we can see that:

  • You'll want your classes to change when your domain model changes, but not when the way you display shapes or export them to XML changes.
  • That leads you to the Single Responsibility Principle that states that a class should have only one reason to change.
  • We found that displaying, as well as exporting to XML (in the example I gave you) are operations you'll do on shapes, and you'll want to easily add new operations later without changing your shape classes.

Suggested Solution

First of all, get your Shape hierarchy cleaned out, and leave only things that belongs to your problem domain.

Then, you need different ways to display shapes without implementing that behavior in the Shape hierarchy. If you won't be dealing with nested shapes (shapes that contain other shapes), you can make use of a technique called Double Dispatch. It lets you dispatch a method call based on the receiver's type by encoding that information in the method name dispatched to the argument it receives like this:

public class Circle extends Shape {
    public void DisplayOn(IShapeDisplay aDisplay) {
        aDisplay.DisplayCircle(this);
    }
}

public class Rectangle extends Shape {
    public void DisplayOn(IShapeDisplay aDisplay) {
        aDisplay.DisplayRectangle(this);
    }
}

interface IShapeDisplay {
    void DisplayCircle(Circle aCircle);
    void DisplayRectangle(Rectangle aRectangle);
}

Classes implementing IShapeDisplay would be responsible to display each kind of shape. The nice thing about that is you managed to clean out those pesky details from the Shape hierarchy, encapsulating them in their own class. Because of that, now you're able to change that class without modifying Shape subclasses.

Final Comments

You can read more about code smells and Parallel Inheritance Hierarchies in Martin Fowler's book: Refactoring: Improving the Design of Existing Code.

Also, you'd like to check the Design Patterns: Elements of Reusable Object-Oriented Software book if you need dealing with shape nesting: the Composite and Visitor patterns.

Hope it was helpful to you!

I have a function and it works. Please check below:

public static function checkAccessLevel($user, $level) {

    public static function checkLevel($user, $level) {

    if ($level == "1") {
       if (!$user->usertype == 1) {
          return array(
           'error'  => 'Please login before', 'errortype' => 0
          );    
       }
    } else if ($level == "2") {
       if (!$user->usertype == 2) {
          return array(
            'error' => 'Please login before', 'errortype' => 0
          );    
       }                    
    } else if ($level == "1,2") {
       if (!$user->usertype == 1 || !$user->usertype == 2) {
           return array(
              'error'   => 'Please login before', 'errortype' => 0
           );   
       }                
    } else {
       return array(
          'error'   => 'Please login before', 'errortype' => 0
       );
    }
}

I'm looking the way for code optimization. Is it possible to rewrite this function?

First of all consider using switch, it should be more readable.

Then you may group your conditions, instead of:

 } else if ($level == "1,2") {
   if (!$user->usertype == 1 || !$user->usertype == 2) {

group them together:

(($level == "1,2") && (!$user->usertype == 1 || !$user->usertype == 2))

Then you may use temporary variable $result:

$result = array('Please login...

and return it at the end, instead of using return keyword in every step, it should help you in debugging.

Also I suggest to read Martin Fowlers book about refactoring which will give you many good advice.

So I hava Java this semester and I really want to ace it. Java feels much at home due to my little C# development background and I also started with Android. So I began learning Java and Android from :

  • Ivor Horton'S Beginning Java
  • Beginning Android 4 Games Development
  • Beginning Android 4 Application Development

and for OOP concepts:

  • Introduction to Object-Oriented Programming by Timothy Budd

I have already completed 30% of Ivor Horton and Budd and am loving it ! Adroid seems pretty catchable too.

So my query is, is there any other resource/book/site that I am missing to compliment my learning? If so what's it?

I would recommend Joshua Bloch's book "Effective Java".

It is a good reading for a novice (and experienced) java programmer.

Also study for design patterns in java

And take a look at Fowler's "Refactoring" book.

To understand how java works look at src.zip file, located in JDK installation folder. Unpack it and study source code of java standard classes. It will give you deep understanding how does java work.

my works like this i shot airplanes and if the shot toughed an airplane it change there positions(airplane and the shot) than it will delete them. if i toughed an airplane the airplane position gets changed. than it gets deleted than my health reduces. well that works well expect that it breaks when i have about 2-4 airplanes left and i toughed one of them and shot one of them my game breaks it pops a box says vector out of range... i have had something like this problem before i fixed it because i new where was the problem but the box doesn't give me any information where it is the problem. here my full code

Thanks in Advance

note: i don't think i can go any lowwer with my code i have put new one

Learn to 'refactor': for example, code like line 241 looks like it would be better as a subroutine.

Anyway, to find where this particular problem is, do "Debug/Break All" with the debugger while the error box is being displayed; and then look at the debugger's "Call stack" window, to see what code is being executed which triggers the popping of the box.