4 ms·

This is an odd mix of common sense, random cosmetic advice, subjective bits and points that are plain wrong or contradictory to other points. For example:

This is an odd mix of common sense, random cosmetic advice, subjective bits and points that are plain wrong or contradictory to other points. For example: Bad: $ymdstr = $moment->format('y-m-d'); Good: $currentDate = $moment->format('y-m-d'); Both are inferior to "$ymd". Use searchable names addExpireAt(DateGlobal::SECONDS_IN_A_DAY); Who would ever need to search for that? You'd search for "Expire". This doesn't preclude from using named constants, but it has nothing to do with search-ability per se. Don't use flags as function parameters Good: function createFile($name) { touch($name); } function createTempFile($name) { touch('./temp/'.$name); } Now, this is how you create the good old copypasta. What you need instead is a single universal function that does in fact accept a boatload of parameters, but that is never meant to be called directly. You then supplement it with a host of simpler one-liners that fill in most of the parameters with defaults and forward just the select few. Avoid negative conditionals should be "Avoid double-negative conditionals". Avoid conditionals this is just overly broad to the point of being completely pointless. What the example proposes is to use abstraction and inheritance instead. In some cases it helps, but it many it leads to needlessly obscure code that's impossible to read off the page. Use method chaining Good: $car = (new Car()) ->setColor('pink') ->setMake('Ford') ->setModel('F-150') ->dump(); Except at some point you will need setModel() to fail for some arguments, e.g. setModel('Trabant'), and you will end up with some methods returning $this and some not. This is not going to be a "clean code" by any measure. (edited for clarity a bit)

userbinator 9y ago
What the example proposes is to use abstraction and inheritance instead. In some cases it helps, but it many it leads to needlessly obscure code that's impossible to read off the page. Reminds me of https://cirillocompany.de/pages/anti-if-campaign https://cirillocompany.de/pages/anti-if-campaign --- not sure if satire.
Sujan 9y ago
So you created PRs for these with fixed examples at the repo. Right?
- coldtea 9y ago
  Why, did they say they are interested in volunteering?
- huhtenberg 9y ago
  You almost seem to imply that I supposed to feel bad or ashamed that I didn't.
zimpenfish 9y ago
> Both are inferior to "$ymd". I half-prefer `$currentDate` but then it doesn't really contain the essential "it's a YMD string" information. `$currentYMD` would be better than all of the above, IMO. All of which assumes you religiously stick to "`...YMD` means a 'Y-M-D' formatted string" and "`...Date` means a date object", mind, otherwise you might as well pick random strings.
- coldtea 9y ago
  >I half-prefer `$currentDate` but then it doesn't really contain the essential "it's a YMD string" information That information is not essential. If the date is later changed to also take hour/minute etc, it will still be currentDate but not YMD. Unless you need to work with the structure of the date, and not just pass it along, currentDate is better.
  - zimpenfish 9y ago
    > That information is not essential. It is if you're passing it to something which expects to receive a YMD date. Also a `$currentDate` which also contains the time is borderline acceptable if specifically called out in comments / docs but I would still ding it in a review.
    - coldtea 9y ago
      >It is if you're passing it to something which expects to receive a YMD date. You don't want to expose what kind of date string that something depends upon in your variable name. This way you you can change the internal implementation (what you pass and the function that received it) and not 1000 places where it's called.
      - zimpenfish 9y ago
        > You don't want to expose what kind of date string that something depends upon in your variable name. Sure you do if you're a subscriber to Hungarian Notation. And if you're working in a loosely-typed language, it helps a lot to be able to track types through variable names.
rypskar 9y ago
> Both are inferior to "$ymd" I think this is wrong, in my opinion the correct is $formattedDate = $moment->format(translation('date-format')); since how a date is formatted will depend on the current language, you don't need to format it if it is for internal use
arethuza 9y ago
"What you need instead is a single universal function that does in fact accept a boatload of parameters, but that is never meant to be called directly. You then supplement it with a host of simpler one-liners that fill in most of the parameters with defaults and forward just the select few." Or if you have a language that can have supply defaults for parameters set them to the default case?
- huhtenberg 9y ago
  I am not aware of any language that supports overriding _some_ of the defaults from the function declaration, e.g. foo(1,,,'abc',,) Meaning that if you need to override a default for, say, 2nd argument, you will also need to explicitly provide all arguments that follow.
  - lccarrasco 9y ago
    Python lets you override only those that you want using Named Parameters as explained in this answer: https://stackoverflow.com/questions/25374814/multiple-default-values-for-function-arguments-python https://stackoverflow.com/questions/25374814/multiple-defaul...
  - conradfr 9y ago
    As an alternative to named parameters I've always wanted a "default" keyword for function calling. function bar($word1, $word2='loves', $word3='Elixir') { return sprintf('%s %s %s', $word1, $word2, $word3); } $foo = bar('Hacker News', default, 'PHP'); But I guess functions with too many arguments are frowned upon and it would encourage it.
  - wolco 9y ago
    If you want to exclude the parameters after 'abc' you can. Better yet in php 7 using the splat operator ... foo(...$args) Allows you to send in any number of args.
  - arethuza 9y ago
    I was thinking of languages (e.g. python and c#) where you can specify default values for named parameters.
Matt3o12_ 9y ago
> Except at some point you will need setModel() to fail for some arguments, e.g. setModel('Trabant'), and you will end up with some methods returning $this and some not. This is not going to be a "clean code" by any measure. Thankfully PHP now supports exceptions, which all chain method APIs have used exclusively used. It would be hell if they a mixed value consisting of true in case everything is ok or any other type (including strings, ints and custom objects) depending on the return code. If you used such an API, you would have spaghetti code because every other line would be if ($err !== true) { handleErr } It is just much easier to have a try and catch block and use multiple catch and try blocks if you need more fine grand controller where the error occoried and the exception does not contain that detail. So in a way, chained method should always garantuee to return itself or throw an appropriate error.
- megous 9y ago
  "Now" is like for the last 12 years. With a few lines you could convert errors to exceptions globally using 1 callback.
coldtea 9y ago
>Both are inferior to "$ymd" Nope. $ymd encodes information about the formatting of the date that could just be an implementation detail (and might change, e.g. need to have the hour/minute too later, or have the month first, or be encoded in a different timezone etc). Whereas that it's the "current date" is a business predicate, and more relevant. >addExpireAt(DateGlobal::SECONDS_IN_A_DAY); Who would ever need to search for that? You'd search for "Expire" I might search for all kinds of things. A searchable name is better. It's also a good way to write the constant, whether you search or not. >Now, this is how you create the good old copypasta. Depends on how many options there are and how crucial is what they do. Two clearly defined functions can be better than an "abstract" function and two instances. >should be "Avoid double-negative conditionals" No, it's negative conditionals alright. The example to be avoided is: "isDOMNodeNotPresent" -- where's the double-negative? >Except at some point you will need setModel() to fail for some arguments, e.g. setModel('Trabant'), and you will end up with some methods returning $this and some not. This is not going to be a "clean code" by any measure. You can trivially use an Exception to signal that. And in other languages, an Optional/Maybe etc type.
- huhtenberg 9y ago
  > Nope. Yep. The original "bad" case was meant to capture the fact that the formatting is YMD. The "good" example however is stuffing a string with an YMD formatted date in a generically-named variable. > A searchable name is better. Nobody argues with that. Point being that search'ability of named constants is not the principal reason for using them. > Two clearly defined functions can be better. Unless they substantially overlap in their code, in which case it's not better. > No, it's negative conditionals alright. "isDOMNodeNotPresent" is not a _conditional_. > You can trivially use an Exception Certainly. But for every dev who thinks that exception-based error handling is a good idea there will be two who don't. And that's even without getting into whether it will be a clean code.
  - coldtea 9y ago
    >Unless they substantially overlap in their code, in which case it's not better. Depends. DRY is not an absolute rule. Sometimes the added abstraction to make something DRY makes it messier. >"isDOMNodeNotPresent" is not a _conditional_. Well, the if is assumed, no? What the author suggests is to avoid having what you check being negative -- so not only "if (!$isNotFoo)" but also "if ($isNotFoo)" is bad as well "if (!isFoo)" is clearer imo. >But for every dev who thinks that exception-based error handling is a good idea there will be two who don't. Some people don't like checked exceptions, but I don't think many have beef with exception based error handling.
lucideer 9y ago
Need to chime and echo coldtea's comment: all of the advice you cite seems quite good, your counter examples don't seem compelling.

userbinator 9y ago

What the example proposes is to use abstraction and inheritance instead. In some cases it helps, but it many it leads to needlessly obscure code that's impossible to read off the page. Reminds me of https://cirillocompany.de/pages/anti-if-campaign https://cirillocompany.de/pages/anti-if-campaign --- not sure if satire.

Sujan 9y ago

So you created PRs for these with fixed examples at the repo. Right?

coldtea 9y ago
Why, did they say they are interested in volunteering?
huhtenberg 9y ago
You almost seem to imply that I supposed to feel bad or ashamed that I didn't.

zimpenfish 9y ago

> Both are inferior to "$ymd". I half-prefer `$currentDate` but then it doesn't really contain the essential "it's a YMD string" information. `$currentYMD` would be better than all of the above, IMO. All of which assumes you religiously stick to "`...YMD` means a 'Y-M-D' formatted string" and "`...Date` means a date object", mind, otherwise you might as well pick random strings.

coldtea 9y ago
>I half-prefer `$currentDate` but then it doesn't really contain the essential "it's a YMD string" information That information is not essential. If the date is later changed to also take hour/minute etc, it will still be currentDate but not YMD. Unless you need to work with the structure of the date, and not just pass it along, currentDate is better.
- zimpenfish 9y ago
  > That information is not essential. It is if you're passing it to something which expects to receive a YMD date. Also a `$currentDate` which also contains the time is borderline acceptable if specifically called out in comments / docs but I would still ding it in a review.
  - coldtea 9y ago
    >It is if you're passing it to something which expects to receive a YMD date. You don't want to expose what kind of date string that something depends upon in your variable name. This way you you can change the internal implementation (what you pass and the function that received it) and not 1000 places where it's called.
    - zimpenfish 9y ago
      > You don't want to expose what kind of date string that something depends upon in your variable name. Sure you do if you're a subscriber to Hungarian Notation. And if you're working in a loosely-typed language, it helps a lot to be able to track types through variable names.

rypskar 9y ago

> Both are inferior to "$ymd" I think this is wrong, in my opinion the correct is $formattedDate = $moment->format(translation('date-format')); since how a date is formatted will depend on the current language, you don't need to format it if it is for internal use

arethuza 9y ago

"What you need instead is a single universal function that does in fact accept a boatload of parameters, but that is never meant to be called directly. You then supplement it with a host of simpler one-liners that fill in most of the parameters with defaults and forward just the select few." Or if you have a language that can have supply defaults for parameters set them to the default case?

huhtenberg 9y ago
I am not aware of any language that supports overriding _some_ of the defaults from the function declaration, e.g. foo(1,,,'abc',,) Meaning that if you need to override a default for, say, 2nd argument, you will also need to explicitly provide all arguments that follow.
- lccarrasco 9y ago
  Python lets you override only those that you want using Named Parameters as explained in this answer: https://stackoverflow.com/questions/25374814/multiple-default-values-for-function-arguments-python https://stackoverflow.com/questions/25374814/multiple-defaul...
- conradfr 9y ago
  As an alternative to named parameters I've always wanted a "default" keyword for function calling. function bar($word1, $word2='loves', $word3='Elixir') { return sprintf('%s %s %s', $word1, $word2, $word3); } $foo = bar('Hacker News', default, 'PHP'); But I guess functions with too many arguments are frowned upon and it would encourage it.
- wolco 9y ago
  If you want to exclude the parameters after 'abc' you can. Better yet in php 7 using the splat operator ... foo(...$args) Allows you to send in any number of args.
- arethuza 9y ago
  I was thinking of languages (e.g. python and c#) where you can specify default values for named parameters.

Matt3o12_ 9y ago

> Except at some point you will need setModel() to fail for some arguments, e.g. setModel('Trabant'), and you will end up with some methods returning $this and some not. This is not going to be a "clean code" by any measure. Thankfully PHP now supports exceptions, which all chain method APIs have used exclusively used. It would be hell if they a mixed value consisting of true in case everything is ok or any other type (including strings, ints and custom objects) depending on the return code. If you used such an API, you would have spaghetti code because every other line would be if ($err !== true) { handleErr } It is just much easier to have a try and catch block and use multiple catch and try blocks if you need more fine grand controller where the error occoried and the exception does not contain that detail. So in a way, chained method should always garantuee to return itself or throw an appropriate error.

megous 9y ago
"Now" is like for the last 12 years. With a few lines you could convert errors to exceptions globally using 1 callback.

coldtea 9y ago

>Both are inferior to "$ymd" Nope. $ymd encodes information about the formatting of the date that could just be an implementation detail (and might change, e.g. need to have the hour/minute too later, or have the month first, or be encoded in a different timezone etc). Whereas that it's the "current date" is a business predicate, and more relevant. >addExpireAt(DateGlobal::SECONDS_IN_A_DAY); Who would ever need to search for that? You'd search for "Expire" I might search for all kinds of things. A searchable name is better. It's also a good way to write the constant, whether you search or not. >Now, this is how you create the good old copypasta. Depends on how many options there are and how crucial is what they do. Two clearly defined functions can be better than an "abstract" function and two instances. >should be "Avoid double-negative conditionals" No, it's negative conditionals alright. The example to be avoided is: "isDOMNodeNotPresent" -- where's the double-negative? >Except at some point you will need setModel() to fail for some arguments, e.g. setModel('Trabant'), and you will end up with some methods returning $this and some not. This is not going to be a "clean code" by any measure. You can trivially use an Exception to signal that. And in other languages, an Optional/Maybe etc type.

huhtenberg 9y ago
> Nope. Yep. The original "bad" case was meant to capture the fact that the formatting is YMD. The "good" example however is stuffing a string with an YMD formatted date in a generically-named variable. > A searchable name is better. Nobody argues with that. Point being that search'ability of named constants is not the principal reason for using them. > Two clearly defined functions can be better. Unless they substantially overlap in their code, in which case it's not better. > No, it's negative conditionals alright. "isDOMNodeNotPresent" is not a _conditional_. > You can trivially use an Exception Certainly. But for every dev who thinks that exception-based error handling is a good idea there will be two who don't. And that's even without getting into whether it will be a clean code.
- coldtea 9y ago
  >Unless they substantially overlap in their code, in which case it's not better. Depends. DRY is not an absolute rule. Sometimes the added abstraction to make something DRY makes it messier. >"isDOMNodeNotPresent" is not a _conditional_. Well, the if is assumed, no? What the author suggests is to avoid having what you check being negative -- so not only "if (!$isNotFoo)" but also "if ($isNotFoo)" is bad as well "if (!isFoo)" is clearer imo. >But for every dev who thinks that exception-based error handling is a good idea there will be two who don't. Some people don't like checked exceptions, but I don't think many have beef with exception based error handling.

lucideer 9y ago

Need to chime and echo coldtea's comment: all of the advice you cite seems quite good, your counter examples don't seem compelling.