92

Feature envy is a term used to describe a situation in which one object gets at the fields of another object in order to perform some sort of computation or make a decision, rather than asking the object to do the computation itself. As a trivial example, consider a class representing a rectangle. The user of the rectangle may need to know its area. The ...


74

When building real-world programs, there is often a trade-off between staying pragmatic on one hand, and staying 100% clean on the other. If staying clean prohibits you to ship your product in time, then you are better off with a little bit of duct-tape to get the d***d thing out of the door. Said that, your description sounds different - it sounds you are ...


67

Instead of initializing the reader from your method, move this line { this.reader = new ReaderImplementation(this.location) } Into the default parameterless constructor. public ReaderConsumer() { this.reader = new ReaderImplementation(this.location) } public ReaderConsumer(IReader reader) { this.reader = reader } There is no such thing as a ...


54

You only need the single constructor: public class ReaderConsumer() { private IReader reader = null public string location // constructor public ReaderConsumer(IReader reader) { this.reader = reader; } in your production code: var rc = new ReaderConsumer(new ReaderImplementation(0)); in your test: var rc = new ...


45

The code can be simplified like this: boolean condA = ( fooSuccess && !barSuccess && mooSuccess ) boolean condB = ( fooSuccess && !barSuccess && !mooSuccess ) boolean condC = (!fooSuccess && barSuccess && mooSuccess ) boolean condD = (!fooSuccess && barSuccess && !mooSuccess && ...


42

The issue I have seen when maintaining code that makes use of flags is that the number of states grows quickly, and there are almost always unhandled states. One example from my own experience: I was working on some code that had these three flags bool capturing, processing, sending; These three created eight states (actually, there were two other flags as ...


40

Yes, it's a code smell. A code smell isn't something that necessarily always needs to get removed. It's something that makes you take a second look. Here you have an object in two fundamentally different states: pre-init and post-init. Those states have different responsibilities, different methods that are allowed to be called, and different behavior. ...


38

Here's an example when flags are useful. I have a piece of code which generates passwords (using a cryptographically secure pseudorandom number generator). The caller of the method chooses whether or not the password should contain capital letters, small letters, digits, basic symbols, extended symbols, Greek symbols, Cyrillic ones and unicode. With flags, ...


34

No - not in terms of lines of code. The driver should be logical grouping. There certainly shouldn't be multiple classes in one large file for example If you had a class that legitmately had a few hundred methods (not impossible in say 3D modelling) it would be a lot less convenient to split that into arbitrary files. We used to have to do this when memory ...


31

Loggers are what we call a "cross-cutting concern." They yield to techniques such as Aspect-Oriented Programming; if you have a way to decorate your classes with an attribute or perform some code weaving, then that is a good way to get logging capabilities while keeping your objects and parameter lists "pure." The only reason you might want to pass in a ...


31

There is absolutely nothing wrong with having pure data objects. The author of the piece quite frankly doesn't know what he's talking about. Such thinking stems from an old, failed, idea that "true OO" is the best way to program and that "true OO" is all about "rich data models" where one mixes data and functionality. Reality has shown us that actually the ...


30

Code organization is all about displaying enough information to convey a single idea. The sweet spot is getting your code pared down enough that a single idea can fit in a single unit of code. Your unit of code can be a function, a class, etc. These are merely tools of organization. As with any tool, it can be over used or used incorrectly. Having a one ...


29

As an ideal model I use the following criteria (with a similar rationale to what Martin Beckett suggested, i.e. to think in terms of logical structure and not in terms of lines of code): Rule 1 One class per file (in C++: one class -> one header and one implementation file). Rule 2 Seven is considered the number of items that our brain can observe at the ...


26

Those assertions are really useful for testing your assumptions, but they also serve another really important purpose: documentation. Any reader of a public method can read the asserts to quickly determine the pre and post conditions, without having to look at the test suite. For this reason, I recommend you keep those asserts for documentation reasons, ...


26

Modern OO design accepts that not everything is an object. Some things are behaviors, or formulae, and some of those don't have state. It's good to model these things as pure functions to get the benefit of that design. Java and C# (and others) require you make a util class and jump through that hoop to do it. Annoying, but not the end of the world; and not ...


26

Let's say you were using a C-style language with no && and needed to do the equivalent code as in your question. Your code would be: if(smartphone != null) { if(smartphone.GetSignal() > 50) { // Do stuff } } This pattern would turn up a lot. Now imagine version 2.0 of our hypothetical language introduces &&. Think how cool ...


25

Data classes are valid in some cases. DTO's are one good example mentioned by Anna Lear. In general though, you should regard them as the seed of a class whose methods haven't yet sprouted. And if you are running into a lot of them in old code, treat them as a strong code smell. They are often used by old C/C++ programmers who have never made the ...


25

It's fine. It's fine in C++ as well, for reference. Keeping tightly-coupled things together is sensible practice. Avoiding inappropriate coupling is also good practice. Striking the right balance isn't a matter of strict rules, but of, well, striking a balance between different concerns. Some rules of thumb: Size Excessively large files can be ugly, but ...


25

Interesting question. I am a bit biased though due to my previous experiences, which prompts me to answer with No. Short answer: We never stop learning. When you hit a wall like that, it is a chance to improve your architectural/design skills, not an excuse to add code smells. The longer version is that I have been asked similar questions a lot of times in ...


23

Here are some reasons why someone might use parrel arrays: In a language that does not support classes or structs To avoid thread locking when individual threads are only modifying one of the columns When the persistence method forces these things to be stored separately and you are reconstituting them. They can consume less memory if the structures are ...


23

Dictionaries (C# or otherwise) are simply a container where you look up a value based on a key. In many languages it's more correctly identified as a Map with the most common implementation being a HashMap. The problem to consider is what happens when a key does not exist. Some languages behave by returning null or nil or some other equivalent value. ...


22

There's nothing wrong with tweaking a UI based on which features are enabled for a customer, or which deployment types they have chosen, but the change should depend on meaningful flags, e.g. "HAVE_EXPORT" to enable/disable an exporting option, not on weird date comparisons. The UI has no business knowing the business rule about what was published when, it ...


22

The code is bad not only because the magic numbers, but because it coalesces several meanings in the return code, hiding inside of its meaning an error, a warning, a permission to create a session or a combination of the three, which makes it a bad input for decision making. I would suggest the following refactoring: returning an enum with the possible ...


22

Some good answers here on the general principles of hashtables/dictionaries. But I thought I'd touch on your code example, int x; if (dict.TryGetValue("key", out x)) { DoSomethingWith(x); } As of C# 7 (which I think is around two years old), that can be simplified to: if (dict.TryGetValue("key", out var x)) { DoSomethingWith(x); } And of course ...


21

I've seen an increasing trend in the programming world saying that it is good practice to separate code blocks into their own functions. I wouldn't have called this an "increasing trend". I was taught that splitting overly large methods into smaller methods improved readability ... ummm ... nearly 40 years ago. And I was taught the design-time equivalent ...


18

Since you are not a professional programmer, I would recommend sticking to simplicity. It will be a LOT easier for programmer to take your modularized, procedural code and make it OO later, than it will be for them to fix a badly written OO program. If you are not experienced, it is possible to create OO programs that can turn into an unholy mess which will ...


16

Yes, it's OK, and fairly common. It can be non-obvious though, as you've discovered. In general, I tend to have persistence-type methods return the updated instance of the object. That is: Report InsertReport(Report report) { db.Reports.InsertOnSubmit(report); db.SubmitChanges(); return report; } Yes, you're returning the same object ...


16

Never say "Never" I don't think it's necessarily bad, it's only bad if you do it badly and abuse it. We All Need Tools and Utilities For starters, we all use some libraries that are sometimes deemed as almost ubiquitous and must-haves. For instance, in the Java world, Google Guava or some of Apache Commons (Apache Commons Lang, Apache Commons Collections, ...


15

It sounds like you are trying to do Design-by-Contract by hand. Doing DbC is a good idea, but you should at least consider switching to a language which does support it natively (such as Eiffel) or at least use a contract framework for your platform (e.g. Microsoft Code Contracts for .NET is pretty nice, arguably the most sophisticated contract framework ...


Only top voted, non community-wiki answers of a minimum length are eligible