5 ms·

Or maybe it’s worth taking a couple seconds to understand the problem domain before judging someone else’s code.

berkes 5y ago
Regardless of the domain: classes with And or Or in them are at the very least a codesmell. Because the name even implies it does more than one thing (SRP)
- singlow 5y ago
  Or it represents one real thing that has multiple purposes outside the code and so the code can't fix that.
  - berkes 5y ago
    True. But I've honestly never encountered this. Not in a valid domain concept. Where "valid" means it is truly embedded in the domain: and not some historical artifact that has been carried along for far too long¹ I'd love to see what valid real-world use-cases there are for an "FooOrBar" or "BarAndFoo" domain concept. -- ¹ Which is a conundrum, I know. If some legacy system or even legacy software was badly designed (or not designed at all, but just evolved over time) carries along its "design flaws", after so many generations or iterations, it becomes the domain concept itself.
    - Anon1096 5y ago
      > But I've honestly never encountered this Boy do I have the perfect example for you then: https://developer.apple.com/documentation/contacts/cnlabelcontactrelationyoungercousinmotherssiblingsdaughterorfatherssistersdaughter https://developer.apple.com/documentation/contacts/cnlabelco...
      - berkes 5y ago
        and, as many people pointed out, the actual domain concept is in Mandarin, and a single word. So a perfect example of the word "Or" that does not denote a logical disjunction but a translation-artifact. I thought it was obvious that it is not literally implied the letters O and R are never allowed after each-other in code. I mean: obviously having a class named OrGate in an electronics simulation is fine. As is OrParty in a system modeling Israelian politics. This is about the word Or denoting that your class, value, type, variable, model etc, is One Or The Other.
        marcellus23 5y ago
        Then I'm not really sure what your point is. People were talking about the specific domain mentioned, then you said "or" is code smell regardless of domain, then people pointed out why it's _not_ code smell in this domain, then you come back and say "of course it's not code smell in this domain, I was talking about something else".
        berkes 5y ago
        My point is that if you have a class, or method, or variable that represents "one thing, or another thing" this is a poor abstraction. And that you can "reek" this smell quite clearly if your names contain "one Or the Other" or "One And The Other" the words "Or" or "And". The example you gave is an artifact of a translation. It is not an indication that this class holds either a "YoungerCousinMothersSiblingsDaughter" class/type/value Or a "FathersSistersDaughter". First, because AFAICS these separate things don't exist. And second because of the first part of that name: `LabelContactRelation`, it being a label; not a return value. You did pay attention to the part where I explained that this, naturally, is not literally so, did you?
        marcellus23 5y ago
        Right, I think you missed my point. If the example being discussed by everyone in this thread does not apply to your point, why did you bring it up in the first place and cause this confusion? And why did you say "Regardless of domain" if that's obviously not true?
    - munchbunny 5y ago
      You've caveated the "valid" answers out of existence. But here are some examples of or-ed concepts: * parent or legal guardian (shows up on legal forms all the time) * state or province (an English language political construct) * DNS name or IP address You could argue that "parent of legal guardian" is a legacy concept since it really just means "legal guardian", but it's literally written on legal forms, so it's a domain concept, except if you believe it shouldn't count because it's an anachronism. You could argue that we don't need "state or province" since we can just say "region", but "region" doesn't mean "state or province" colloquially, and we have to be careful not to refer to Ontario as a state, so the distinction is important. Ironically, that distinction does not exist in Chinese, but it does in English. "DNS name or IP address" absolutely are two different concepts, it just happens that URL's were specced to accept both as the "host" segment. So whether or not it counts as an example depends on whether you believe that the act of coining the term is also the act of erasing its status as an "or" relation. Alternatively, if you're implementing a web browser, that distinction absolutely does exist because you need to know not to try to resolve an IP address as a DNS name.
      - berkes 5y ago
        I agree about the first two. The legal one being the most strong. WRT "state or province": I've modelled quite some GIS data and tend to follow abstractions laid by OSM here. Where "state" is very different from "province" because in many locales that is the case; But maybe your model is purely Chinese and the OSM model -albeit being global- has a poor local fit. The latter: nah. It's the one I know about most, and I'd say, depending on your use-case, "address", "host", "location", URI, URL, etc are a better name for what you are modeling than "DnsOrIpAddress". If only for the fact that this can be read as "DNS Address Or IP address" as well as "DNS, or IP-Address". It is truly a bad name in many situations.
        munchbunny 5y ago
        Fair enough, but you've surely encountered those first two before.
- LandR 5y ago
  What if it's an Either type / some kind of discriminated union? e.g. type PersonOrError = P of Person | E of Error It can be either or a Person or an Error
  - berkes 5y ago
    If it is an argument, it is certainly a design issue: `response_serializer(PersonOrError: person_or_error)` is confusing, impossible to reason about easily, hard to test etc. It signals a way too tightly coupled API, for one. And it breaks the SRP, secondly. Why not `response_serializer(Person: person, Error: error)` or `try_error(person).then(response_serializer(person))` or many other designs that do the same, but make it explicit where, when and why the distinction is made. It is something you often see in e.g. Rails, though. And I consider it really poor and confusing API: an example of that dreaded "Rails Black Magick" where a method like `form_for()` can get literally hundreds of combinations of arguments and behaves entirely different according to what is being passed. In runtime. In production. With user-generated content and dynamic typing and all that. If it is a return-type, there too: why not make it a tuple, or do like Rust: make it an `Option<Some, None>: user` or a `Result<Ok, Err>: user`, the typing now contains this "either/or" not the variable names; And if it is a class, or even method: it is violating SRP. Neither of the reasons are absolute. But they all signal that a better solution is possible (but maybe not practical given constraints). Which is fine: but acknowledging this design is lacking (and choosing to still use it) is IMO the correct path; telling yourself that the design is fine, is not.
    - LandR 5y ago
      I agree about passing Either types as arguments, I wouldn't do this. Pattern matching solves this problem. > If it is a return-type As for making them return types, it forces you to match over them, you can have the compiler do exhaustive matching over the types it can be. If you have a tuple of (int, string), you could store an int and a string. With Either types, it has to be one OR the other, it can never be both. Like I said, you can pattern match over this depending on what the type contains. It also allows nice binding and failing out the chain early. e.g. something like in a C style syntax: public Either<Error, int> M() { "In M".Print(); return 6; } public Either<Error, string> M2(int y) { $"In M2 {y}".Print(); return $"{++y}"; } public Either<Error, string> M3(string y) { $"In M3 {y}".Print(); return $"Result {y}"; } public void ShowResult(string y) => y.Print(); public void DisplayError(Error error) => error.Message.Print(); M().Then(M2) .Then(M3) .Match(left: DisplayError, right: ShowResult); So, the functions that operate on the Either types only ever take the right type of the Either. If say M2 returned an Error, M3 would never be called and you shortcut to the error handler. The match forces you to explicitly handle if it's an error or not. I like this pattern, it can be concise and expressive when used in a context where it's applicable. > And if it is a class, or even method: it is violating SRP. I disagree, the Person class is just a Person (and nothing else), the Error class is an Error and nothing else. The Either class is generic and is a way to hold Either of multiple types (but only one at any time). > make it an `Option<Some, None>: I like Rusts option type, but the problem is the None state doesn't collect information, but I think it's certainly a nicer alternative to null. Again you can compile time enforce pattern matching to ensure you handle both states. > Result<Ok, Err>: user` Assuming Result can only ever be OK or Err, this is basically an Either type!
- glxxyz 5y ago
  It's not a class, it's a variable. The Or is there because the same Chinese word refers to two possible relationships within a family tree. It doesn't do more than one thing, it represents more than one relationship. If the word 'Sibling' didn't exist, we could use 'BrotherOrSister' in its place.
- goto11 5y ago
  All code does more than one thing, and this is not what the SRP (Single Responsibility Principle) is about. The SRP says a module should not implement multiple independent business concerns.

Regardless of the domain: classes with And or Or in them are at the very least a codesmell. Because the name even implies it does more than one thing (SRP)

singlow 5y ago
Or it represents one real thing that has multiple purposes outside the code and so the code can't fix that.
- berkes 5y ago
  True. But I've honestly never encountered this. Not in a valid domain concept. Where "valid" means it is truly embedded in the domain: and not some historical artifact that has been carried along for far too long¹ I'd love to see what valid real-world use-cases there are for an "FooOrBar" or "BarAndFoo" domain concept. -- ¹ Which is a conundrum, I know. If some legacy system or even legacy software was badly designed (or not designed at all, but just evolved over time) carries along its "design flaws", after so many generations or iterations, it becomes the domain concept itself.
  - Anon1096 5y ago
    > But I've honestly never encountered this Boy do I have the perfect example for you then: https://developer.apple.com/documentation/contacts/cnlabelcontactrelationyoungercousinmotherssiblingsdaughterorfatherssistersdaughter https://developer.apple.com/documentation/contacts/cnlabelco...
    - berkes 5y ago
      and, as many people pointed out, the actual domain concept is in Mandarin, and a single word. So a perfect example of the word "Or" that does not denote a logical disjunction but a translation-artifact. I thought it was obvious that it is not literally implied the letters O and R are never allowed after each-other in code. I mean: obviously having a class named OrGate in an electronics simulation is fine. As is OrParty in a system modeling Israelian politics. This is about the word Or denoting that your class, value, type, variable, model etc, is One Or The Other.
      - marcellus23 5y ago
        Then I'm not really sure what your point is. People were talking about the specific domain mentioned, then you said "or" is code smell regardless of domain, then people pointed out why it's _not_ code smell in this domain, then you come back and say "of course it's not code smell in this domain, I was talking about something else".
        berkes 5y ago
        My point is that if you have a class, or method, or variable that represents "one thing, or another thing" this is a poor abstraction. And that you can "reek" this smell quite clearly if your names contain "one Or the Other" or "One And The Other" the words "Or" or "And". The example you gave is an artifact of a translation. It is not an indication that this class holds either a "YoungerCousinMothersSiblingsDaughter" class/type/value Or a "FathersSistersDaughter". First, because AFAICS these separate things don't exist. And second because of the first part of that name: `LabelContactRelation`, it being a label; not a return value. You did pay attention to the part where I explained that this, naturally, is not literally so, did you?
        marcellus23 5y ago
        Right, I think you missed my point. If the example being discussed by everyone in this thread does not apply to your point, why did you bring it up in the first place and cause this confusion? And why did you say "Regardless of domain" if that's obviously not true?
  - munchbunny 5y ago
    You've caveated the "valid" answers out of existence. But here are some examples of or-ed concepts: * parent or legal guardian (shows up on legal forms all the time) * state or province (an English language political construct) * DNS name or IP address You could argue that "parent of legal guardian" is a legacy concept since it really just means "legal guardian", but it's literally written on legal forms, so it's a domain concept, except if you believe it shouldn't count because it's an anachronism. You could argue that we don't need "state or province" since we can just say "region", but "region" doesn't mean "state or province" colloquially, and we have to be careful not to refer to Ontario as a state, so the distinction is important. Ironically, that distinction does not exist in Chinese, but it does in English. "DNS name or IP address" absolutely are two different concepts, it just happens that URL's were specced to accept both as the "host" segment. So whether or not it counts as an example depends on whether you believe that the act of coining the term is also the act of erasing its status as an "or" relation. Alternatively, if you're implementing a web browser, that distinction absolutely does exist because you need to know not to try to resolve an IP address as a DNS name.
    - berkes 5y ago
      I agree about the first two. The legal one being the most strong. WRT "state or province": I've modelled quite some GIS data and tend to follow abstractions laid by OSM here. Where "state" is very different from "province" because in many locales that is the case; But maybe your model is purely Chinese and the OSM model -albeit being global- has a poor local fit. The latter: nah. It's the one I know about most, and I'd say, depending on your use-case, "address", "host", "location", URI, URL, etc are a better name for what you are modeling than "DnsOrIpAddress". If only for the fact that this can be read as "DNS Address Or IP address" as well as "DNS, or IP-Address". It is truly a bad name in many situations.
      - munchbunny 5y ago
        Fair enough, but you've surely encountered those first two before.
LandR 5y ago
What if it's an Either type / some kind of discriminated union? e.g. type PersonOrError = P of Person | E of Error It can be either or a Person or an Error
- berkes 5y ago
  If it is an argument, it is certainly a design issue: `response_serializer(PersonOrError: person_or_error)` is confusing, impossible to reason about easily, hard to test etc. It signals a way too tightly coupled API, for one. And it breaks the SRP, secondly. Why not `response_serializer(Person: person, Error: error)` or `try_error(person).then(response_serializer(person))` or many other designs that do the same, but make it explicit where, when and why the distinction is made. It is something you often see in e.g. Rails, though. And I consider it really poor and confusing API: an example of that dreaded "Rails Black Magick" where a method like `form_for()` can get literally hundreds of combinations of arguments and behaves entirely different according to what is being passed. In runtime. In production. With user-generated content and dynamic typing and all that. If it is a return-type, there too: why not make it a tuple, or do like Rust: make it an `Option<Some, None>: user` or a `Result<Ok, Err>: user`, the typing now contains this "either/or" not the variable names; And if it is a class, or even method: it is violating SRP. Neither of the reasons are absolute. But they all signal that a better solution is possible (but maybe not practical given constraints). Which is fine: but acknowledging this design is lacking (and choosing to still use it) is IMO the correct path; telling yourself that the design is fine, is not.
  - LandR 5y ago
    I agree about passing Either types as arguments, I wouldn't do this. Pattern matching solves this problem. > If it is a return-type As for making them return types, it forces you to match over them, you can have the compiler do exhaustive matching over the types it can be. If you have a tuple of (int, string), you could store an int and a string. With Either types, it has to be one OR the other, it can never be both. Like I said, you can pattern match over this depending on what the type contains. It also allows nice binding and failing out the chain early. e.g. something like in a C style syntax: public Either<Error, int> M() { "In M".Print(); return 6; } public Either<Error, string> M2(int y) { $"In M2 {y}".Print(); return $"{++y}"; } public Either<Error, string> M3(string y) { $"In M3 {y}".Print(); return $"Result {y}"; } public void ShowResult(string y) => y.Print(); public void DisplayError(Error error) => error.Message.Print(); M().Then(M2) .Then(M3) .Match(left: DisplayError, right: ShowResult); So, the functions that operate on the Either types only ever take the right type of the Either. If say M2 returned an Error, M3 would never be called and you shortcut to the error handler. The match forces you to explicitly handle if it's an error or not. I like this pattern, it can be concise and expressive when used in a context where it's applicable. > And if it is a class, or even method: it is violating SRP. I disagree, the Person class is just a Person (and nothing else), the Error class is an Error and nothing else. The Either class is generic and is a way to hold Either of multiple types (but only one at any time). > make it an `Option<Some, None>: I like Rusts option type, but the problem is the None state doesn't collect information, but I think it's certainly a nicer alternative to null. Again you can compile time enforce pattern matching to ensure you handle both states. > Result<Ok, Err>: user` Assuming Result can only ever be OK or Err, this is basically an Either type!
glxxyz 5y ago
It's not a class, it's a variable. The Or is there because the same Chinese word refers to two possible relationships within a family tree. It doesn't do more than one thing, it represents more than one relationship. If the word 'Sibling' didn't exist, we could use 'BrotherOrSister' in its place.
goto11 5y ago
All code does more than one thing, and this is not what the SRP (Single Responsibility Principle) is about. The SRP says a module should not implement multiple independent business concerns.