Nov 10, 2023 14:02
I wonder how many times one can use the word “guise” before wondering “are we the baddies?”
 
Oct 8, 2023 14:49
but this is making prod code simpler (one if statement less), and it's moving the "log level" responsibility to the logger where it better belong
Oct 8, 2023 14:47
Like if it were just making prod code more complex for the sake of testing, I get it
Oct 8, 2023 14:26
Let me ask you this: Suppose you get a good idea for an API design, but you discover it via the TDD process. Is that something you have to hold back from using, because it violate some divine "thou shall not change prod for tests" law?
Oct 8, 2023 14:25
Perhaps we just disagree there, and that's fine, but I really don't think that's the case
Oct 8, 2023 14:24
@Tibrogargan I'm not missing that, though!
Oct 8, 2023 14:24
@Tibrogargan "you're missing though is that in this case it would literally be adding production code that is only there to resolve a test coverage issue"
Oct 8, 2023 01:50
And this isn't just changing production code for the sake of meeting tests. It does happen to help with that, but it's also just a better design for the production code. I would suggest writing that regardless of whether you had the issue with test coverage or not.
Oct 8, 2023 01:49
In other words, the suggestion made by SonarQube is just bad. It's bad code, that adds a branch that you have to find coverage for. Adding coverage isn't the solution, it's removing the need for the branch.
Oct 8, 2023 01:47
It's the logger's job to be worried about the log level. It can easily have a test that ensures "log.info does nothing if the info level isn't enabled" and "the message supplier isn't called unless the message will actually be used"
Oct 8, 2023 01:46
It's not the job for some random code (which just so happens to log stuff, in addition to its main responsibility) to be concerned about what the log level is. Put another way: it would be inappropriate for the tests to cover this, because this isn't a relevant responsibility of the unit
Oct 8, 2023 01:45
Not only does this solve your test issue, but it's a better design in general
Oct 8, 2023 01:45
I offer an alternative solution: Rather than checking `log.isInfoEnabled()` in your calling code, modify the logger to accept a lambda, which only generates the messages and logs if the particular log level is enabled. This solves your test coverage problem:

When the logger is responsible for checking the log-level, then your caller code doesn't need to. What's better than test coverage for a code branch? Not needing a code branch at all.
Oct 8, 2023 01:44
Your initial concern is about how to add test coverage for the case when `log.isInfoEnabled()` is false.
Oct 8, 2023 01:44
@Tibrogargan I think we're talking past one another here. From what I understand:
Oct 7, 2023 01:28
Even in its strictest interpretation, nothing about TDD suggests that you should code at 88 mph or the ~bus~ program explodes, barrelling through implementing tests and prod code in the first shape that comes to mind, with no forethought about design or grander structure.

You can take detours to implement the abstractions that you discover that you need
Oct 7, 2023 01:27
> the coverage analysis suite suggested adding code that caused more coverage issues.

Yeah? There's a piece of your system that you need that doesn't exist yet. That's totally fine. You just go write the new tests and the new code, then resume off from where you were
Oct 7, 2023 01:26
Heya @Tibrogargan

I know the the idiom, but it's not clear to me which part is "insult" and what part is the "injury".
Oct 7, 2023 01:24
Hey @Tibrogargan "but that would be adding insult to injury" Sorry, I don't think I follow, what's the insult, and what's the injury? As far as I see it, using a lambda both solves your testing issue (it removes the if statement, so there's no extra coverage needed, just assert that the info was called on the provided logger), while also being a more robust design for prod.
Oct 7, 2023 01:24
Actually, nothing I've said is really specific to testing. It's just a better design from a prod perspective, as well. E.g. it makes it impossible to forget the isInfoEnabled() check (which would cause you to emit an info msg even though it was supposed to be disabled). Have a look at Log4j's API for reference. It offers an overload of .info that takes a String (for when the msg construction is cheaper than a lambda), and one that takes a Supplier (for when the msg is expensive).
Oct 7, 2023 01:24
The usual approach for this kinda thing, if feasible, is to use a lambda which lazily produces the message, only when needed. Would that work for your case?
 
Jul 31, 2023 04:24
Every single hand in a game of poker is rarer than a royal flush (4 times rarer, obviously, because there's 1 of every possible hand, but 4 hands that we chose to label "royal flush"). It's just that nobody is noticing/measuring/impressed-by getting 5H, 2S, 6H, KD, 8D or some other hand we'd call "junk"
Jul 31, 2023 04:22
Only because no one is paying attention to sequences without satisfying patterns
Jul 31, 2023 04:22
> The probability of getting 100 straight heads is the same as any other sequence, yet the other sequences aren’t seen as improbable.
 
Jul 12, 2023 00:00
It's like a marker protocol, when you see implements IImmutableDictionary instead of IReadOnlyDictionary`, that's a pinky-promise that the class' instances won't be mutable (which isn't enforced by the compiler)
Jul 11, 2023 23:59
@AndrewWilliamson Indeed! And in fact, IImmutableDictionary could be a subtype of IReadOnlyDictionary, and it can even be empty! IReadOnlyDictionary already has the required methods.
Jul 11, 2023 23:58
@AndrewWilliamson My ordering was intentional, as an indication of causality. I believe the goal/motivation of LSP is to build systems where polymorphism can work correctly (without special-casing NetworkStream, in your example)
Jul 11, 2023 21:32
Adhering correctly to LSP is about not screwing up polymorphism
Jul 11, 2023 21:31
I think you have it backwards, conceptually
Jul 11, 2023 20:44
Not _exactly_ the same, no, but it must preserve any invariants that have been guaranteed by "the contract" (whether it be a superclass or interface). Method signatures are just one such thing to be preserved (and the most obvious/explicit), but invariants guaranteed by the methods' contracts must also be preserved
Jul 11, 2023 20:44
> no OO law that says each implementation must be semantically the same
Jul 11, 2023 20:42
From my fair reading of the entire article, I conclude that inheritance really is a very minute. This is a general principle that could be applied even to any kind of modular thing, where once can be replaced with another
Jul 11, 2023 20:37
In object-oriented programming, behavioral subtyping is the principle that subclasses should satisfy the expectations of clients accessing subclass objects through references of superclass type, not just as regards syntactic safety (such as the absence of "method-not-found" errors) but also as regards behavioral correctness. Specifically, properties that clients can prove using the specification of an object's presumed type should hold even though the object is actually a member of a subtype of that type. For example, consider a type Stack and a type Queue, which both have a put method to add an...
Jul 11, 2023 20:37
> In object-oriented programming, behavioral subtyping is the principle that subclasses should satisfy the expectations of clients accessing subclass objects through references of superclass type, not just as regards syntactic safety (such as the absence of "method-not-found" errors) but also as regards behavioral correctness.
Jul 11, 2023 20:37
> The Liskov substitution principle (LSP) is a particular definition of a subtyping relation, called strong behavioral subtyping, that was initially introduced by Barbara Liskov in ...
Jul 11, 2023 20:36
Inheritance and subclassing has a very minimal mention on the Wikipedia article, and there's a consistent emphasis on the more general term "subtyping"
Jul 11, 2023 20:36
@MartinMaat I certianly could be wrong, but I don't agree, and I think the quote I shared spells it out pretty directly
Jul 11, 2023 19:23
Looks like you can :)
Jul 11, 2023 19:22
Hmmm?
Jul 11, 2023 15:25
> semantic interoperability
Jul 11, 2023 15:25
> It is a semantic rather than merely syntactic relation, because it intends to guarantee semantic interoperability of types in a hierarchy, object types in particular
Jul 11, 2023 15:24
There's no dispute over the original definition, I just liked that layman wording. If we're going to focus on the original definition, it explicitly talks about "subtypes" en.wikipedia.org/wiki/Liskov_substitution_principle
Jul 11, 2023 15:24
@MartinMaat Ignoring implementation details for a moment, I would summarize LSP as "can be correctly used polymorphically". For example, It's not sufficient that two objects implement IComparable, with a CompareTo() method that return an Int, if the semantics of their int results isn't consistent. Despite being able to make the call, I won't be able to use these objects polymorhpically. Likewise with a hypothetical IImmutableDictionary. I like jgauffin's wording: "LSP applies to the contract. The contract may be a class or an interface.".
Jul 11, 2023 15:24
@MartinMaat "an interface typically only defines a subset of its implementer's behavior and properties" That's fine, but interfaces are not just dumb bags of method requirements. That's how they manifest in source code, but interfaces can also have meaning and invariants which aren't surfaced explicitly in source, but are nonetheless important. For example, IComparable requires you to return an integer, with specific meanings for negative, positive and zero. You can implement that interface and not respect those meanings, but it would be incorrect to do so.
Jul 11, 2023 15:24
@MartinMaat Hmmm I don't quite understand your perspective. LSP isn't narrowly about inheritance, it generalizes to "subtyping" of any form (including implementing interfaces). However, I did learn that my answer is based on an incorrect premise. "IReadOnlyDictionary" means "a dictionary that guarantees at least the ability to be read" not "an immutable dictionary that can only be read". Still, if the interface was IImmutableDictionary, all my remarks about LSP would be correct, I believe.
 
Sep 24, 2022 21:49
@TheBluffs If you did that, I would just scroll it slowly while watching a video or doing something worthwhile with my time. If the training is useless (and it's sounding like it is, if the complaint is about the training metrics being low but not the output KPIs), then can it. If it's not, then find an engaging way to do it.
 
Aug 26, 2022 04:29
@Shum You're looking at trees and missing the forest. The optional you're unwrapping is the last element of Array(0...num), which is always num (or nil when num is 0, as I explained earlier). You never actually use that array, this is just a really contrived way to check guard num != 0 else { return 0 }, which can be combined with the earlier if num < 0 { return 0 } into just a simple if num <= 0 { return 0 }
Aug 26, 2022 04:29
Further points to look into before I write up a full answer: arrayOfTen is pretty much never used. The guard clause will always pass, except for n is 0, but you can just roll that into the first line (if num <= 0 { return 0 }). Further, you recalculate sum needlessly on every loop iteration, but you only ever use the last value of it
Aug 26, 2022 04:29
"Note: If the number is a multiple of both 3 and 5, only count it once." Your code counts it twice. You should add some test cases with known inputs/outputs to test your code, and help others who are interested in messaging it into better shape
 

 The 2nd Monitor

General discussion about codereview.stackexchange.com - Welcom...
Aug 16, 2021 19:32
> Your example isn't an example what people actually comment.

@Peilonrayz It's what I would have commented, so I"m not really sure what you're trying to say