Is this too big for a struct ctor?

[dusky pier]

I just want to know if this is too much code to be put on a ctor?

constexpr FFloat160::FFloat160(const int64 HiVal, const int64 LoVal, const int32 ExVal) 
    : Hi{HiVal}, Lo{LoVal}, Ex{ExVal}
{
    // Lo can only be 0, if Hi is 0
    if (Lo == 0LL && Hi != 0LL)
    {
        Lo = Hi;
        Hi = 0LL;
        Ex += 18;
    }

    // Make sure Hi and Lo sign is same
    if ((Hi > 0LL && Lo < 0LL) || (Hi < 0LL && Lo > 0LL))
        Lo *= -1LL;

    // Make sure Lo don't have Trailing Zero
    const int32 TrailingZeroNum = TaMath::CountTrailingZero(Lo);
    if (TrailingZeroNum != 0)
    {
        const int64 ExpoDivisor = static_cast<int64>(FMath::Pow(10.0, TrailingZeroNum));

        Lo /= ExpoDivisor;
        if (Hi != 0LL)
        {
            Lo += (Hi % ExpoDivisor) * static_cast<int64>(FMath::Pow(10.0, 18 - TrailingZeroNum));
            Hi /= ExpoDivisor;
        }
        Ex += TrailingZeroNum;
    }

    // if Hi have more than 18 digits
    if (FMath::Abs(Hi) > 999'999'999'999'999'999LL)
    {
        Lo /= 10LL;
        Lo += (Hi % 10LL) * 100'000'000'000'000'000LL;
        Hi /= 10LL;
        Ex += 1;
    }

    // if Lo have more than 18 digits
    if (FMath::Abs(Lo) > 999'999'999'999'999'999LL)
    {
        Hi = Lo / 1000'000'000'000'000'000LL;
        Lo %= 1000'000'000'000'000'000LL;
    }
}

I don't think you need to understand what the code or what my struct do... but... the struct is more or less like this

struct FFloat160
{
private:
    // Hi and Lo together made up Mantissa
    // 
    // Rule:
    // 1. Lo can only be 0, if Hi is 0
    //    if Hi not 0, Lo should not be 0
    // 2. Hi and Lo should have same sign
    //    If sign not same, follow Hi sign
    // 3. Lo should not have Trailing Zero
    // 4. Hi or Lo should not have more than 18 digits
    //
    int64 Hi{0}; // 10^18
    int64 Lo{0};
    int32 Ex{0}; // Exponent
}

[digital wharf]

personally I would likely make this a function rather than a constructor, but thats just my style (then make the constructor maybe private/protected)

[dusky pier]

I see...

[digital wharf]

but thats more of a modern style

I think this is perfectly reasonable

[digital wharf]

the only reason i said this is because you can get some large benefits from having trivial types

[dusky pier]

what is the most common benefit example?

[digital wharf]

well theyre pretty good for codegen/performance, But potentially the largest benefit is old/new constexpr things

a trivial type can be a template parameter for example

although I think the restrictions have reduced recently

[dusky pier]

I see... thx, I will think about that

[digital wharf]

but yeah they allow the compiler to enable some nice optimizations

but then you lose out on some level of enforcing invariants

so its a tradeof

also from a readability perspective

potentially something like FFloat160 f(a, c, d); looks less likely to be doing complex calculations than
FFloat160 f = cannonicalize_ffloat160(a, b, c);

its also more clear what the constructor is doing if you have a named function, and you can potentially do better errorhandlging (e.g. returning an optional/expected if the function might error so you don't need to use exceptions or out-parameters)

in this case since the members are values its not so bad, but if you had non-assignable members (like references or const members) you would definitly need a function to prepare the values ahead of time

[dusky pier]

I see... thx

[pseudo kindle]

Depends on the exact context - the constructor is not doing too much, but there is too much code in it

You should split that into functions

[digital wharf]

no this is fine

if you split this uo it will be unreadable

and why would you split it unless other code needs it

[digital wharf]

also i saw somehwere else you had a similar constructor which just did the same things but with some defaults

its good to know you can do delegating constructors

[pseudo kindle]

For readability sake

[digital wharf]

I disagree that splitting up ~30 lines of code is required

if you need to split up something that small you'll spend your entire time splitting up code

[pseudo kindle]

True, but in this case, the code is not readable

[digital wharf]

it is readable

how is it unreadable

[pseudo kindle]

You cannot tell what the code does unless you read the comments

[digital wharf]

thats what comments are for

actually no you are incorrect

you can tell what the code does

you just cant tell why it does it

which is what the comments are for

the code is not doing anything complex

at most

        const int64 ExpoDivisor = static_cast<int64>(FMath::Pow(10.0, TrailingZeroNum));

        Lo /= ExpoDivisor;
        if (Hi != 0LL)
        {
            Lo += (Hi % ExpoDivisor) * static_cast<int64>(FMath::Pow(10.0, 18 - TrailingZeroNum));
            Hi /= ExpoDivisor;
        }
        Ex += TrailingZeroNum;
```could be factored out

the rest is fine

[pseudo kindle]

Maybe it's my personal preference then. But I find this code not readable enaugh

Maybe if the namings were different, it would be more clear for me

[digital wharf]

I think we're saying the same thing tbh

you can tell what it does

just not why

like each step is not hard to read

the comments could be a little better

but I dont see a reason to factor the code out unless you're going to re-use it somewhere

otherwise thats just functions for the sake of comments, but we already have comments theyre called comments

[pseudo kindle]

Maybe. I got slightly detracted from comments on behalf of naming things better by Uncle Bob

[digital wharf]

Uncle Bob is way over the top with his "functions should do 1 thing"

if you write functions which do 1 thing your code becomes impossible to reason about because every time you go into a function there is just another function

[pseudo kindle]

Gotta say ... you have a point

I usually try to tone down a bit on this, into making a function do one logical thing, and trying to keep it not longer than 60-70 lines

So it's managable to work with them, and they don't hurt the eyes that much

[digital wharf]

yeah thats reasonable I think

good benchmark

[pseudo kindle]

Thanks

[digital wharf]

I say that ... I have a function which is probably a couple thousand lines long

but its really only that big because its 1 big switch case, so each case sort of acts like its own function

[pseudo kindle]

For instance, in the OP case I would extract the processing of hi and processing of lo to separate functions. If placed in anonymous namespace within .cpp file, they would not hurt anyone, and would make reasoning about what happens easier in my opinion

[pseudo kindle]

And in this case, I would just extract the bodies of each case to a separate function, and just keep the switch, as sort of a dispatcher function

[digital wharf]

you say that but code doesnt evolve so smoothly. And actually having the implementations of multiple similar cases next to each other is useful

[pseudo kindle]

If those cases are similar, I would consider having a class that provides them in a per-function basis

Considering they share at least a significant part of parameters

Regarding the code evolution, here TDD kinda comes into help. I'm personally not a fan, however I usually write implementation first, then make tests, and from time to time take a moment to refactor what I already have

I find refactoring one in an often while pretty helpful

[surreal pelican]

Putting all these details in a constructor is perfectly fine though me personally I’d break things up into functions and call them from the constructor if everything has to happen during construction.