"if" condition structure

(2) Share

Report

Posted on by DELDYN

556

Hi,

I want to know which code looks better and if both have the same effect

private boolean canGenerateValue(Table1 _table1)
{
if(_table1 && _table1.Field1 == NoYes::No &&
(_table1.Type == Type::A
&& Parameters::find().FieldX == NoYes::Yes
&& Table2::find(_Table1.Id).FieldA== NoYes::Yes
)
||
(
_table1.Type == Type::B
)
)
{
return true;
}
else
{
return false;
}

}

OR

private boolean canGenerateValue(Table1 _table1)
{
if(_table1&& _table1.Field1 == NoYes::No)

{
if (_table1.Type == Type::A)
{

if(Parameters::find().FieldX == NoYes::Yes && Table2::find(_Table1.Id).FieldA== NoYes::Yes)

{
return true;

}

else

{
return false;

}

}
else if(_table1.Type == Type::B)

{
return true;

}

else

{
return false;

}

private boolean canGenerateValue(Table1 _table1)
{
if(_table1&& _table1.Field1 == NoYes::No)

{
if (_table1.Type == Type::A && Parameters::find().FieldX == NoYes::Yes && Table2::find(_Table1.Id).FieldA== NoYes::Yes)
{
return true;

}
else if(_table1.ype == Type::B)

{
return true;

}

else

{
return false;

}

-- and for the first option, since I have mix of && and || --- what is the best way to place them, sometimes i see the && on the same line and the || on a new line alone and sometimes the opposite

I have the same question (0)

All responses (14)

Answers (0)

Sort by

Martin Dráb 240,061 Most Valuable Professional on at

Like
a
(1)

Report
Copy link

Link copied!
My attempt: :-)
private boolean canGenerateValue(Table1 _table1) { if (!_table1.RecId || _table1.Field1 == NoYes::Yes) { return false; } switch (_table1.Type) { case Type::A: return Parameters::find().FieldX && Table2::find(_table1.Id).FieldA; case Type::B: return true; } return false; }

Was this reply helpful? Yes No
DELDYN 556 on at

Like
a
(0)

Report
Copy link

Link copied!

.

Was this reply helpful? Yes No
DELDYN 556 on at

Like
a
(0)

Report
Copy link

Link copied!
Hi Martin, I can't edit my last comment somehow so i will add a new message

If the full code is as you can see below:

I have two questions:

1. As you can see in the "canGenerateValue()" method, i had this condition "!Table3::find(table1.CustAccount).FieldD" and I had to select Table3 again inside "generateValue()" method to get FieldD by doing this:
Table3 table3 = Table3::find(_table1.CustAccount);
            if(table3.FieldD)
How can i avoid selecting table3 twice and still making the code clean and readable?

2. In method postPosting, is it ok that first I had to do this  Table1 table1 = _table1
then later do this
table1 = Table1::find(_table1.Id, true);
??
[ExtensionOf(classStr(Class1_TypeA))] final class Class1_TypeA_Extension { void postPosting(Table1 _table1) { Table1 table1 = _table1; if (this.canGenerateValue(table1)) { str value = this.generateValue(table1); if(value) { ttsbegin; table1 = Table1::find(_table1.Id, true); table1.Value = value; table1.update(); ttscommit; } } next postPosting(table1); } }

[ExtensionOf(classStr(Class1_TypeB))] final class Class1_TypeB_Extension { void postPosting(Table1 _table1) { Table1 table1 = _table1; if (this.canGenerateValue(table1)) { str value = this.generateValue(table1); if(value) { ttsbegin; table1 = Table1::find(_table1.Id, true); table1.Value = value; table1.update(); ttscommit; } } next postPosting(table1); } }

[ExtensionOf(classStr(Class1))] final class Class1_Extension { public boolean canGenerateValue(Table1 _table1) { if (!_table1 || _table1.Field1 == NoYes::Yes || !Table3::find(_table1.CustAccount).FieldD) { return false; } switch (_table1.Type) { case Type::A: return Parameters::find().FieldX && Table2::find(_table1.Id).FieldA; case Type::B: return true }    return false; } public void generateValue(Table1 _table1) { str value; //logic Table3 table3 = Table3::find(_table1.CustAccount); if(table3.FieldD) { Table1 table1Count; select count(RecId) from table1Count where table1Count.Id2 == _table1.Id2; if(table1Count.RecId == 1) {    //logic if(//logic) { //logic value = //logic } else { //logic value = table3.FieldD + todayDate + //logic; } }    } return value; } }

Was this reply helpful? Yes No
Martin Dráb 240,061 Most Valuable Professional on at

Like
a
(0)

Report
Copy link

Link copied!
1. You can store the record in a variable.

2. Using the same variable for two different thing isn't a good approach. Use should two variables instead and give them names describing their purpose. But in your particular case, I don't think you need to the first usage at all. I would do this instead:

void postPosting(Table1 _table1) { if (this.canGenerateValue(_table1)) { str value = this.generateValue(_table1); if (value) { ttsbegin; Table1 table1Update = Table1::find(_table1.Id, true); table1Update.Value = value; table1Update.update(); ttscommit; } } next postPosting(table1); }

Was this reply helpful? Yes No
DELDYN 556 on at

Like
a
(0)

Report
Copy link

Link copied!
Hi Martin,

For point 2, I'm already using two variables (table1 and _table1), i think the difference then, between my approach and yours, is that you defined the 2nd variable inside the ttsbegin and that you called it table1Update instead of table1.
but u passed _table1 in next postPosting(_table1) instead of table1Update and I want to pass table1Update. what do you think?

For point1, you mean to define FieldD in class1 as a global variable? and then fill it's value in method "canGenerateValue" and at the end call fieldD directly inside "generateValue()" method? like the code below?
but now if someone doesn't want to call canGenerateValue, when they call postProcessing, fieldD will always be empty, so i wanted to force filling the varaible somehow, that's why i called Table3::find twice in the previous reply... what do you think?
[ExtensionOf(classStr(Class1))] final class Class1_Extension { public str fieldD; public boolean canGenerateValue(Table1 _table1) { fieldD = Table3::find(_table1.CustAccount).FieldD; if (!_table1 || _table1.Field1 == NoYes::Yes || !fieldD) { return false; } switch (_table1.Type) { case Type::A: return Parameters::find().FieldX && Table2::find(_table1.Id).FieldA; case Type::B: return true } return false; } public void generateValue(Table1 _table1) { str value; //logic Table1 table1Count; select count(RecId) from table1Count where table1Count.Id2 == _table1.Id2; if(table1Count.RecId == 1) { //logic if(//logic) { //logic value = //logic } else { //logic value = fieldD + todayDate + //logic; } } return value; } }

Was this reply helpful? Yes No
Martin Dráb 240,061 Most Valuable Professional on at

Like
a
(0)

Report
Copy link

Link copied!

2. I know you're using table1 and _table1, my point is that you reuse table1. You assign a value there twice.

1. What you mean is an instance variable, not a global variable. You can store there either FieldD or Table3. If you're saying that your design populating the value in canGenerateValue() is wrong, then fix it. How to do it depends on your requirements. For example, you can do it when creating an instance, use lazy initialization (when you need the value, you'll check if it's populated and call find() if not), maybe calling postProcessing() without canGenerateValue() would be a bug that you need to prevent...

Declaring the variable as public is a bad idea, make it either private or protected (if you want to make it accessible from child classes).

But we're off-topic here, because this has nothing to do with "if" condition structure.

Was this reply helpful? Yes No
DELDYN 556 on at

Like
a
(0)

Report
Copy link

Link copied!
Hi Martin,

2. But in your solution you are passing table1 to nextPosting(table1) instead of table1Update and i won't be able to pass table1Update if you define it inside the ttsbegin. That's why I had to do this

Table1 table1 = _table1;

1. we can't declare protected variables in extensions. And sorry i meant if we call generateValue without canGenerateValue, fieldD will always be empty -- So i think i can do sth like this (check green changes please):

[ExtensionOf(classStr(Class1))]
final class Class1_Extension
{
private str fieldD;

public boolean canGenerateValue(Table1 _table1)
{

if (!_table1 || _table1.Field1 == NoYes::Yes)
{
return false;
}

fieldD = Table3::find(_table1.CustAccount).FieldD; // i think it's better to not include this in the first or condition in order to not select this table if the first condition will fail what do you think?
if(!fieldD)
{
return false;
}

switch (_table1.Type)
{
case Type::A:
return Parameters::find().FieldX && Table2::find(_table1.Id).FieldA;
case Type::B:
return true
}

return false;
}

public str generateValue(Table1 _table1)
{
str value;

//logic

    if(!fieldD)
{
return '';
}
Table1 table1Count;

select count(RecId) from table1Count
where table1Count.Id2 == _table1.Id2;
if(table1Count.RecId == 1)
{
//logic
if(//logic)
{
//logic
value = //logic
}
else
{
//logic
value = fieldD + todayDate + //logic;
}
}


return value;
}
}

Was this reply helpful? Yes No
Martin Dráb 240,061 Most Valuable Professional on at

Like
a
(0)

Report
Copy link

Link copied!

2. I don't understand what you're trying to tell me. I don't pass table1Update to nextPosting() because it's impossible and it would be logically wrong.

1. I was talking about all classes, not just final ones.

Was this reply helpful? Yes No
DELDYN 556 on at

Like
a
(0)

Report
Copy link

Link copied!

Hi Martin,

2. I mean, that i want to pass the updated value of Table1, that's why I had to do this

1. I don't get what u mean, what do u want me to do in the other two classes: Class1_TypeA_Extension, Class1_TypeB_Extension?
So is what i did in the last reply not what you wanted?

Was this reply helpful? Yes No
Martin Dráb 240,061 Most Valuable Professional on at

Like
a
(0)

Report
Copy link

Link copied!

2. Aha, I see it now.

1. There is nothing to do with Class1_TypeA_Extension and Class1_TypeB_Extension, because Table3 is not used there. Let me remind you that your question was "How can i avoid selecting table3 twice".

Was this reply helpful? Yes No