Csdrhrt

Yes, it's an acceptable byproduct, and the justification is that it is now structured such that you don't have to read most of the code most of the time. Instead of reading a 30-line function every single time you are making a change, you are reading a 5-line function to get the overall flow, and maybe a couple of the helper functions if your change touches that area. If your new "extra" class is called EmailValidator and you know your problem isn't with email validation, you can skip reading it altogether.

It's also easier to reuse smaller pieces, which tends to reduce your line count for your overall program. An EmailValidator can be used all over the place. Some lines of code that do email validation but are scrunched together with database access code can't be reused.

And then consider what needs to be done if the email validation rules ever need to be changed—which would you rather: one known location; or many locations, possibly missing a few?

Bill Gates was famously attributed to saying, "Measuring programming progress by lines of code is like measuring aircraft building progress by weight."

I humbly agree with this sentiment. This is to not say that a program should strive for more or less lines of code, but that this isn't ultimately what counts to create a functioning and working program. It helps to remember that ultimately the reason behind adding extra lines of code is that it is theoretically more readable that way.

Disagreements can be had on whether a specific change is more or less readable, but I don't think you'd be wrong to make a change to your program because you think by doing so you're making it more readable. For instance making an isEmailValid could be thought to be superfluous and unnecessary, especially if it is being called exactly once by the class which defines it. However I would much rather see an isEmailValid in a condition than a string of ANDed conditions whereby I must determine what each individual condition checks and why it is being checked.

Where you get into trouble is when you create a isEmailValid method which has side effects or checks things other than the e-mail, because this is worse than simply writing it all out. It is worse because it is misleading and I may miss a bug because of it.

Though clearly you're not doing that in this case, so I would encourage you to continue as you're doing. You should always ask yourself if by making the change, it is easier to read, and if that is your case, then do it!

so some developers/managers see value in writing less code to get things done so that we have less code to maintain

This is a matter of losing sight on the actual goal.

What matters is lowering hours spent on development. That is measured in time (or equivalent effort), not in lines of code.

This is like saying that car manufacturers should build their cars with less screws, because it takes a non-zero amount of time to put each screw in. While that is pedantically correct, a car's market value is not defined by how many screws it does or doesn't have. Above all else, a car needs to be performant, safe, and easy to maintain.

The rest of the answer are examples of how clean code can lead to time gains.

Logging

Take an application (A) which has no logging. Now create application B, which is the same application A but with logging. B will always have more lines of code, and thus you need to write more code.

But a lot of time will sink into investigating issues and bugs, and figuring out what went wrong.

For application A, developers will be stuck reading the code, and having to continually reproduce the problem and step through the code to find the source of the issue. This means that the developer has to test from the beginning of the execution to the end, in every used layer, and needs to observe every used piece of logic.

Maybe he is lucky to find it immediately, but maybe the answer is going to be in the last place he thinks of looking.

For application B, assuming perfect logging, a developer observes the logs, can immediately identify the faulty component, and now knows where to look.

This can be a matter of minutes, hours or days saved; depending on the size and complexity of the codebase.

Regressions

Take application A, which is not DRY-friendly at all.

Take application B, which is DRY, but ended up needing more lines because of the additional abstractions.

A change request is filed, which requires a change to the logic.

For application B, the developer changes the (unique, shared) logic according to the change request.

For application A, the developer has to change all instances of this logic where he remembers it being used.

• If he manages to remember all instances, he'll still have to implement the same change several times.


• If he does not manage to remember all instances, you're now dealing with an inconsistent codebase that contradicts itself. If the developer forgot a rarely used piece of code, this bug may not become apparent to the end users until well into the future. At that time, are the end users going to identify what the source of the issue is? Even if so, the developer may not remember what the change entailed, and will have to figure out how to change this forgotten piece of logic. Maybe the developer doesn't even work at the company by then, and then someone else now has to figure it all out from scratch.

This can lead to enormous time wastage. Not just in development, but in hunting and finding the bug. The application can start behaving erratically in a way that developers cannot easily comprehend. And that will lead to lengthy debugging sessions.

Developer interchangeability

Developer A created application A. The code is not clean nor readable, but it works like a charm and has been running in production. Unsurprisingly, there is no documentation either.

Developer A is absent for a month due to holidays. An emergency change request is filed. It can't wait another three weeks for Dev A to return.

Developer B has to execute this change. He now needs to read the entire codebase, understand how everything works, why it works, and what it tries to accomplish. This takes ages, but let's say he can do it in three weeks' time.

At the same time, application B (which dev B created) has an emergency. Dev B is occupied, but Dev C is available, even though he doesn't know the codebase. What do we do?

• If we keep B working on A, and put C to work on B, then we have two developers who don't know what they're doing, and the work is bering performed suboptimally.


• If we pull B away from A and have him do B, and we now put C on A, then all of developer B's work (or a significant portion of it) may end up being discarded. This is potentially days/weeks of effort wasted.

Dev A comes back from his holiday, and sees that B did not understand the code, and thus implemented it badly. It's not B's fault, because he used all available resources, the source code just wasn't adequately readable. Does A now have to spend time fixing the readability of the code?

All of these problems, and many more, end up wasting time. Yes, in the short term, clean code requires more effort now, but it will end up paying dividends in the future when inevitable bugs/changes need to be addressed.

Management needs to understand that a short task now will save you several long tasks in the future. Failing to plan is planning to fail.

If so, what are some arguments I can use to justify the fact that more LOC have been written?

My goto explanation is asking management what they would prefer: an application with a 100KLOC codebase that can be developed in three months, or a 50KLOC codebase that can be developed in six months.

They will obviously pick the shorter development time, because management doesn't care about KLOC. Managers who focus on KLOC are micromanaging while being uninformed about what they're trying to manage.

I think you should be very careful about applying "clean code" practices in case they lead to more overall complexity. Premature refactoring is the root of many bad things.

Extracting a conditional to a function leads to simpler code at the point where the conditional was extracted from, but leads to more overall complexity because you now have a function which is visible from more points in the program. You add a slight complexity burden to all other functions where this new function is now visible.

I'm not saying you shouldn't extract the conditional, just that you should consider carefully if you need to.

• If you want to specifically test the e-mail validation logic. Then you need to extract that logic to a separate function - probably even class.


• If the same logic is used from multiple places in the code then you obviously have to extract it to a single function. Don't repeat yourself!


• If the logic is obviously a separate responsibility, e.g. the email validation happen in the middle of a sorting algorithm. The email validation will change independent of the sorting algorithm, so they should be in separate classes.

In all of the above the is a reason for the extraction beyond it just being "clean code". Furthermore, you probably wouldn't even be in doubt if it was the right thing to do.

I'd say, if in doubt, always choose the simplest and most straightforward code.

I'd point out there is nothing inherently wrong with this:

if(contact.email != null && contact.email.contains('@')

At least assuming it's used this one time.

I could have problems with this very easily:

private Boolean isEmailValid(String email){

   return contact.email != null && contact.email.contains('@');

}

A few things I'd watch for:

1. Why is it private? It looks like a potentially useful stub. Is it useful enough to be a private method and no chance of it being used more widely?


2. I wouldn't name the method IsValidEmail personally, possibly ContainsAtSign or LooksVaguelyLikeEmailAddress because it does almost no real validation, which is maybe good, maybe not what is exected.


3. Is it being used more than once?

If it is being used once, is simple to parse, and takes less than one line I would second guess the decision. It probably isn't something I'd call out if it wasn't a particular problem from a team.

On the other hand I have seen methods do something like this:

if (contact.email != null && contact.email.contains('@')) { ... }

else if (contact.email != null && contact.email.contains('@') && contact.email.contains("@mydomain.com")) { //headquarters email }

else if (contact.email != null && contact.email.contains('@') && (contact.email.contains("@news.mydomain.com") || contact.email.contains("@design.mydomain.com") ) { //internal contract teams }

That example is obviously not DRY.

Or even just that last statement can give another example:

if (contact.email != null && contact.email.contains('@') && (contact.email.contains("@news.mydomain.com") || contact.email.contains("@design.mydomain.com") )

The goal should be to make the code more readable:

if (LooksSortaLikeAnEmail(contact.Email)) { ... }

else if (LooksLikeFromHeadquarters(contact.Email)) { ... }

else if (LooksLikeInternalEmail(contact.Email)) { ... }

Another scenario:

You might have a method like:

public void SaveContact(Contact contact){

   if (contact.email != null && contact.email.contains('@'))

   {

       contacts.Add(contact);

       contacts.Save();

   }

}

If this fits your business logic and isn't reused there is not a problem here.

But when someone asks "Why is '@' saved, because that's not right!" and you decide to add actual validation of some sort, then extract it!

You'll be glad you did when you also need to account for the presidents second email account Pr3$sid3nt@h0m3!@mydomain.com and decide to just go all out and try and support RFC 2822.

On readability:

// If there is an email property and it contains an @ sign then process

if (contact.email != null && contact.email.contains('@'))

If your code is this clear, you don't need comments here. In fact, you don't need comments to say what the code is doing most the time, but rather why it's doing it:

// The UI passes '@' by default, the DBA's made this column non-nullable but 

// marketing is currently more concerned with other fields and '@' default is OK

if (contact.email != null && contact.email.contains('@'))

Whether the comments above an if statement or inside a tiny method is to me, pedantic. I might even argue the opposite of useful with good comments inside another method because now you would have to navigate to another method to see how and why it does what it does.

In summary: Don't measure these things; Focus on the principles that the text was built from (DRY, SOLID, KISS).

// A valid class that does nothing

public class Nothing 

{



}

Clean Code is an excellent book, and well worth reading, but it is not the final authority on such matters.

Breaking code down into logical functions is usually a good idea, but few programmers do it to the extent that Martin does - at some point you get diminishing returns from turning everything into functions and it can get hard to follow when all the code is in tiny pieces.

One option when it's not worth creating a whole new function is to simply use an intermediate variable:

boolean isEmailValid = (contact.email != null && contact.emails.contains('@');



if (isEmailValid) {

...

This helps keep the code easy to follow without having to jump around the file a lot.

Another issue is that Clean Code is getting quite old as a book now. A lot of software engineering has moved in the direction of functional programming, whereas Martin goes out of his way to add state to things and create objects. I suspect he would have written quite a different book if he'd written it today.

Considering the fact that the "is email valid" condition you currently have would accept the very much invalid email address "@", I think you have every reason to abstract out an EmailValidator class. Even better, use a good, well-tested library to validate email addresses.

Lines of code as a metric is meaningless. The important questions in software engineering are not:

• Do you have too much code?


• Do you have too little code?

The important questions are:

• Is the application as a whole designed correctly?


• Is the code implemented correctly?


• Is the code maintainable?


• Is the code testable?


• Is the code adequately tested?

I've never given a thought to LoC when writing code for any purpose but Code Golf. I have asked myself "Could I write this more succinctly?", but for purposes of readability, maintainability, and efficiency, not simply length.

Sure, maybe I could use a long chain of boolean operations instead of a utility method, but should I?

Your question actually makes me think back on some long chains of booleans I've written and realize I probably should have written one or more utility method(s) instead.

On one level, they are right - less code is better.
Another answer quoted Gate, I prefer:

“If debugging is the process of removing software bugs, then programming must be the process of putting them in.”
– Edsger Dijkstra

“When debugging, novices insert corrective code; experts remove defective code.”
– Richard Pattis

The cheapest, fastest, and most reliable components are those that aren’t there. - Gordon Bell

In short, the less code you have, the less can go wrong. If something isn't necessary, then cut it.
If there is over-complicated code, then simplify it until the actual functional elements are all that remain.

What is important here, is that these all refer to functionality, and only having the minimum required to do it. It doesn't say anything about how that is expressed.

What what you are doing by attempting to have clean code isn't against the above. You are adding to your LOC but not adding unused functionality.

The end goal is to have readable code but no superfluous extras. The two principles should not act against each other.

A metaphor would be building a car. The functional part of the code is the chassis, engine, wheels... what makes the car run. How you break that up is more like the suspension, power steering and so on, it makes it easier to handle.
You want your mechanics as simple as possible while still performing their job, to minimise the chance of things going wrong, but that doesn't prevent you from having nice seats.

There's lots of wisdom in the existing answers, but I'd like to add in one more factor: the language.

Some languages take more code than others to get the same effect.  In particular, while Java (which I suspect is the language in the question) is extremely well-known and generally very solid and clear and straightforward, some more modern languages are much more concise and expressive.

For example, in Java it could easily take 50 lines to write a new class with three properties, each with a getter and setter, and one or more constructors — while you can accomplish exactly the same in a single line of Kotlin* or Scala.  (Even greater saving if you also wanted suitable equals(), hashCode(), and toString() methods.)

The result is that in Java, the extra work means you're more likely to reuse a general object that doesn't really fit, to squeeze properties into existing objects, or to pass a bunch of ‘bare’ properties around individually; while in a concise, expressive language, you're more likely to write better code.

(This highlights the difference between the ‘surface’ complexity of the code, and the complexity of the ideas/models/processing it implements.  Lines of code isn't a bad measure of the first, but has much less to do with the second.)

So the ‘cost’ of doing things right depends on the language.  Perhaps one sign of a good language is one that doesn't make you choose between doing things well, and doing them simply!

(* This isn't really the place for a plug, but Kotlin is well worth a look IMHO.)

Let's assume that you are working with class Contact currently. The fact that you are writing another method for validation of email address is evidence of the fact that the class Contact is not handling a single responsibility.

It is also handling some email responsibility, which ideally, should be its own class.

Further proof that your code is a fusion of Contact and Email class is that you will not be able to test email validation code easily. It will require a lot of maneuverings to reach email validation code in a big method with the right values. See the method viz below.

private void LargeMethod() {

    //A lot of code which modifies a lot of values. You do all sorts of tricks here.

    //Code.

    //Code..

    //Code...



    //Email validation code becoming very difficult to test as it will be difficult to ensure 

    //that you have the right data till you reach here in the method

    ValidateEmail();



    //Another whole lot of code that modifies all sorts of values.

    //Extra work to preserve the result of ValidateEmail() for your asserts later.

}

On the other hand, if you had a separate Email class with a method for email validation, then to unit test your validation code you would just make one simple call to Email.Validation() with your test data.

_{Bonus Content: MFeather's talk about the deep synergy between testability and good design.}

Reduction in LOC has been found to be correlated with reduced defects, nothing else. Assuming then that anytime you reduce LOC, you have reduced the chance of defects is essentially falling into the trap of believing that correlation equals causation. The reduced LOC is a result of good development practices and not what makes code good.

In my experience, people who can solve a problem with less code (at a macro level) tend to be more skilled than those who write more code to do the same thing. What these skilled developers do to reduce the lines of code is use/create abstractions and reusable solutions to solve common problems. They don't spend time counting lines of code and agonizing over whether they can cut a line here or there. Often the code they do write is more verbose than is necessary, they just write less of it.

Let me give you an example. I've had to deal with logic around time periods and how they overlap, whether they are adjacent, and what gaps exist between them. When I first started working on these problems, I would have blocks of code doing the calculations everywhere. Eventually, I built classes to represent the time periods and operations that calculated overlaps, complements etc. This immediately removed big swaths of code and turned them into a few method calls. But those classes themselves were not written tersely at all.

Plainly stated: if you are trying to reduce LOC by trying to cut a line of code here or there with more terse, you are doing it wrong. It's like trying to lose weight by reducing the amount of vegetables you eat. Write code that is easy to understand, maintain, and debug and reduce LOC through reuse and abstraction.

Let's start with the example at hand.

Why should your class - in general - even validate the Email or bother to know what an email is? This looks like a perfect use case for a Validator interface, extended by an EmailValidator class.

Or, a EmailValidationRule that you can use to build a validator for the data of some other concrete entity (a form?).

so, in general ...

Which brings to the two points raised:

1. your original class will be shorter and will satisfy the SRP (Single Responsibility Principle)



2. 
   

   yes, you'll have more classe, but they will be:

   
   
   
   
   

   • implementation agnostic (validator does not care where it is used)

   
   

   • reusable (more classes will be able to validate an email)

   
   
   
   

Caveats:

Firstly: yes clean code is sometimes bigger and yes bigger is generally worse, but the implicit there will be more is probably not right.

It is quite common for people to see the extra work from a change more than the lost work. If you take a finished (dirty) product, and 'make it nice' and you've added a lot of extra code, the likely hood is you've not done well.

It may well be that you are not mis-evaluating the situation, or doing it wrong but that you are running into the initial overhead, and will see the payoff later.

The real answer:

The problem of size is not on of lines or characters, those are just proxies for the complexity:

if (cond())

  func();

Has exactly the same complexity as:

if (SensibleNameForConditional())

{

  SensibleNameForFunction();

}

Despite the fact its twice as many lines and characters.
There are not twice as many opportunities mistakes, the extra characters could be seen as error checking. If you wrote:

if (SensibleNameForConditional())

{

  sensible_name_for_function();

}

by accident the compiler would pick it up straight away, and maybe even prompt a did you mean...

If you contract that with:

if(cond_a()==val||flags_b&flags_c)

  impl_->do_thing()

There are fewer lines and characters here but there are 6 tokens not 2, and mistakes are hard to determine: did you mean flags_b&&flags_c, what about precidence...
There will be no clear way for the reader or the compiler to check what you meant was what you said etc.

Not all lines are created equal, 3 concisely named variables are not less complex than one 1 verbosely named one.

Conclusion:

Lines of code and visual size of code (character count) are only used as measures of how 'big' code is because they are unambitious, easily measured and for a given developer/style: an okay measure of amount of complexity. However when comparing between styles, and for a fixed project size: it means sweet FA, and souldn't need justifying unless you are a really slow typist...

You should write the code like how the code is designed.

Everybody say goto is bad. But in some specific cases, people design a subsystem using flow charts, that contain some logic basically equivalent to goto. Everyone implement, review and test the system using the flow charts as reference. In this case, rewriting it to not use goto only because goto is bad, isn't likely a great improvement. It may only add one more step that someone may make mistakes.

That doesn't mean avoiding goto is wrong. It may reflect a deeper problem, such as you should not design it using flow charts in the first place. Or you find a very strong reason that it's undoubtedly better to use flow charts in the original design, because it is just that special. That reason should also convince you goto isn't too bad in this very specific case. But in either case, you really should not avoid goto more than how it is designed. You may avoid it anyway for many other practical reasons, but sure you should not learn from such practical cases. The reason it is good or bad will be different from the general cases.

The "clean code" rule is especially easy to have such problems. It may be not easy to notice if you blindly follow it. I notice two problems in your case:

In many cases, you simply cannot describe a method accurately. People may have different expectations about things with the same name. For example, you are using a quite naive way of verifying email addresses. Sometimes you'll want to test whether there are at least one word before and after the @ symbol. Sometimes you may even want to test whether the domain exists.

If you have a design document, that explicitly say what is an email address, good for you all. But if not, someone who need a stronger rule (think about a new UI that has the @ symbol prefilled) may be misled by the method name. If they are not misled, you may also end up with incomprehensible method names such as isEmailStrictlyValid or even isEmailValid2. This design will end up counterproductive.

I feel it much more problematic if the methods are about a group of related things, having an arbitrary name of the group in the name of the method, but people have different ideas about how things are grouped together.

The less clear the design is, the less the programmers should trust the code, and the more details should be exposed. I'm not saying you must have that document to specify it exactly, but one could make a clear educated guess for something critical to the core logic or user experience. But how you verify an email address hardly affect anything else in your system. And when it does affect something, you may find some unexpected problems.

TL;DR: The method name should describe itself, but it actually didn't.

The other problem is, this code is hardly near ideal. Without more context, the method isEmailValid clearly should belong to contact.email, not a class that contains the contact.

Adding more context, we could find it not so easy. contact.email is a string. We may have to move it to contact.email.text and change all the relevant code. How about two fields whose validity are related? We may write reference of the related fields to the affected field, or even use "two-way binding". contact is something directly from the database, and it's a bit awkward to contain this kind of logic, and wastes memory. We'll have to split it, like using models and views, to make the model part usable in smaller utility programs. It contains copied code now. We may implement a framework that creates the models automatically if nothing notable has changed. Hmm...

That creates much more code than just writing it "cleanly". This may seem absurd, but not unimaginable. Many GUI frameworks already implemented it this way. Actually the code is really much cleaner if you can add any number of fields, only overriding some onValidate method or event to control how it is validated, and all of them are called programmatically. You don't even need to invent the method names for each specific field.

By simply using separate private methods, you may have some new problems, such as forgetting to call a validation method, or mixing two field of the same type but verified differently. All these are gone if you switch to the more complicated way. And you will feel relieved by removing such noises from your mind. Do you have to create them in the first place?

I'd say the body of the method has a strong relevance to contact.email, a weaker relevance to the method calling all the validations, and an indirect (thus the weakest) relevance to the class containing that method. By doing this, you did remove the weaker relevance, which could sure be perceived as imperfect in some standard.

Not using the better way, is usually due to the limitations of the language. Implementing them anyway may make others more difficult to learn, and different things implemented in different way less likely to be compatible. It's advisable to only make the kind of complicated design in environments that it is not ready to use, if either you clearly need it, so that it's not only a matter of style, or your product is clearly labeled as a framework for other programmers.

TL;DR: You should use a separate method in some appropriate place. But you have put it in the wrong place.

I consider these rules are advices. Even between the advices, they are the easiest things to follow, and probably the easiest thing to change if you feel you need them. But they are not the most important factors deciding the code quality.

If other big parts are clearly imperfect, the small style changes may arbitrarily profit or backfire. It's worse to backfire than not profit, because others would wonder why you did it this way, and would be reluctant to optimize it. One should firstly evaluate the tradeoff about the big parts. If you intend to keep the imperfect way for the big parts, you may realize it is impossible to follow the small advices in the perfect ways. If it is not perfect, you may risk adding more noise to it by arbitrarily adapting to what is possible.

In general, they are good. Writing more code isn't that much of a problem. (Languages may improve by removing some boilerplates, but they are not logically that much longer, effectively just adding a name and the folding and listing functionality to a code block.) But if something else cause them to be bad, they are bad, unjustifiably. Their easiness automatically makes them one of the last things to consider, when you won't refactor too much other code just for them, unless you have extreme OCD. In a personal project, it's justifiable to not use them for reasons such as you simply don't feel like them applied in this case.

Personally, if the exact behavior isn't specified in any documents, I won't follow this rule in the first try. But I may change when I could estimate how big the project will be and how likely I will reuse them. If instead, there is a high quality (not only for bureaucracy) and complete (I won't need more experiments and am sure everything is correct) design document, I would probably simply blindly follow the document. The clean code rule itself won't be relevant. Emphasize on this because people could easily think it has the benefit of this clean code rule, while the code alone is not clean enough without the document, and this rule is obviously supposed to be the most useful when there isn't too much document.

I would say that the point of Clean Code practices is ultimately to make your work more readable and more reliable.

A few seconds over on the Code-Golf stack-exchange will show you exactly where "least code" can lead. Utter opaque unreadability is not good coding practice, even if you achieve in a few dozen characters what would ordinarily take 1000 lines.

If you get hit by a bus and the well-qualified replacement they hire to do your job has no idea what your code is doing, your code is bad.

With Enterprise code; be Readable first, Efficient second.

The rule I like to work with is that if I can describe the complete functionality of a block of code with a single sentence, it should be wrapped up as a self-contained Method.

If there's repetition in my code, make a method.

If something is an clearly defined Task in the application, it's a method.

Ultimately, my code becomes a tree of complexity that starts with Main()

Every part is individually easily followed, but put together they produce something very complex.

At each stage, I describe what I want to do with any wrapping conditionals.
For example

if(ValidateEmail(Profile)){

  SendEmail(Profile)

}

Would be a perfectly normal piece of code in my work. It might hide a couple hundred lines of validation and ajax and so on, but none of that is important because it's known to work consistently and doesn't affect anything outside of those two method calls.

In an otherwise busy section of code, writing a few method calls keeps things tidy.

There's no real penalty for bulky code as long as it's encapsulated in Methods.

So write chunky easy-to-read code, it'll be more robust and your colleagues will thank you for it.