Anyway to make this code a little easier to read

Pho3nix · Dec 14, 2015

Hi,

Think the code below is a tad bloated.
Thoughts on making it a little more streamlined and easier to read?

PS. Don't rage about the variable names and the like,please. This is still in sublime and not in any solution

Enums are available for the riskCategory and QuestionTypes, just can't refer back to those properly in Sublime.

Code:

        foreach (var i in ruleScoringList)
            {
                if(i.riskCategory == "1")
                {
                    if(i.questionType == "1")
                    {
                        if(i.ruleValue == "Yes")
                        {
                            rule1 = false;
                        }
                    }
                }
                if(i.riskCategory == "7")
                {
                    if(i.questionType == "1")
                        {
                            if(i.ruleValue == "Yes")
                            {
                                rule1 = false;
                            }
                        }
                    if(i.questionType == "2")
                        {
                            switch(i.ruleValue)
                            {
                                case "None":
                                    valRule2 = valRule2 + 0;
                                    break;
                                case "Low"
                                    valRule2 = valRule2 + 1;
                                    break;
                                case "Medium"
                                    valRule2 = valRule2 +2;
                                    break;
                                case "High"
                                    valRule2 =  valRule2 +3;
                            }
                        }
                }
                if(i.riskCategory == "10")
                {
                    if(i.questionType == "1")
                        {
                            if(i.ruleValue == "Yes")
                            {
                                rule1 = false;
                            }
                        }
                    if(i.questionType == "2")
                        {
                            switch(i.ruleValue)
                            {
                                case "None":
                                    valRule3 = valRule3 + 0;
                                    break;
                                case "Low"
                                    valRule3 = valRule3 + 1;
                                    break;
                                case "Medium"
                                    valRule3 = valRule3 +2;
                                    break;
                                case "High"
                                    valRule3 =  valRule3 +3;
                            }
                        }
                }
                if(i.riskCategory == "11")
                {
                    if(i.questionType == "1")
                        {
                            if(i.ruleValue == "Yes")
                            {
                                rule1 = false;
                            }
                        }
                    if(i.questionType == "2")
                        {
                            switch(i.ruleValue)
                            {
                                case "None":
                                    valRule4 = valRule4 + 0;
                                    break;
                                case "Low"
                                    valRule4 = valRule4 + 1;
                                    break;
                                case "Medium"
                                    valRule4 = valRule4 +2;
                                    break;
                                case "High"
                                    valRule4 =  valRule4 +3;
                            }
                        }
                }

semaphore · Dec 14, 2015

Build a rule engine/parser. That is an utter abortion.

Pho3nix · Dec 14, 2015

semaphore said:
Build a rule engine/parser. That is an utter abortion.

Was [-]afraid[/-] hoping you were still around

Will look into it.

shauntir · Dec 14, 2015

OMG!

Yes, look into building some kind of rules engine.

From the looks of it, your problem could be a state machine. Have a look at this c# library that could potentially help. https://github.com/nblumhardt/stateless

MagicDude4Eva · Dec 14, 2015

In Java this is as simple as (you obviously would need to implement RiskRule)

RulesEngine rulesEngine = RulesEngine().build();

rulesEngine.registerRule(new RiskRule(myRiskQuestions));

rulesEngine.fireRules();

Look at EasyRules (https://github.com/EasyRules/easyrules) or something similar and do not let yourself be talked into buying some BPEL engine for this

me_ · Dec 14, 2015

It's difficult because I'm not 100% sure what you are trying to achieve, but you are repeating the same checks for every questiontype. Maybe consider moving that to your primary and then switch the category beneath:

Code:

        foreach (var i in ruleScoringList)
            {
				rule1 = !((i.questionType == "1") && (i.ruleValue == "Yes"));
				
				if(i.questionType == "2")
				{
					switch(i.riskCategory)
					{
						case 7:
							switch(i.ruleValue)
							{
								case "None":
									valRule2 = valRule2 + 0;
									break;
								case "Low"
									valRule2 = valRule2 + 1;
									break;
								case "Medium"
									valRule2 = valRule2 +2;
									break;
								case "High"
									valRule2 =  valRule2 +3;
							}
						break;
						case 10:
							switch(i.ruleValue)
                            {
                                case "None":
                                    valRule3 = valRule3 + 0;
                                    break;
                                case "Low"
                                    valRule3 = valRule3 + 1;
                                    break;
                                case "Medium"
                                    valRule3 = valRule3 +2;
                                    break;
                                case "High"
                                    valRule3 =  valRule3 +3;
                            }
							break;
						case 11:
							switch(i.ruleValue)
                            {
                                case "None":
                                    valRule4 = valRule4 + 0;
                                    break;
                                case "Low"
                                    valRule4 = valRule4 + 1;
                                    break;
                                case "Medium"
                                    valRule4 = valRule4 +2;
                                    break;
                                case "High"
                                    valRule4 =  valRule4 +3;
                            }
						default:
						break;
					
					}
				}
			}

Also, if RuleValue was an enum, it would have a text value and a numeric so you could parse the text value into the enum and then add the enum numeric. Something like:

Code:

case 7:
	ValueEnum val = Enum.Parse(ValueEnum,i.ruleValue);
	valRule2 += val;
	break;

Edit: This it would then look something like:

Code:

//Declare Enum Type
enum ValueEnum
{
	None = 0,
	Low = 1,
	Medium = 2,
	High = 3
}


//Updated check		
	foreach (var i in ruleScoringList)
            {
			//Not sure if this would work for your scenario, but rather than checking multiple values,
			//assign the output from the boolean check to the rule1 value.
			rule1 = !((i.questionType == "1") && (i.ruleValue == "Yes"));
				
			if(i.questionType == "2")
			{
				switch(i.riskCategory)
				{
					case 7:
						ValueEnum val = (ValueEnum)Enum.Parse(ValueEnum,i.ruleValue);
						valRule2 += val;
						break;
					case 10:
						ValueEnum val = (ValueEnum)Enum.Parse(ValueEnum,i.ruleValue);
						valRule3 += val;
						break;
					case 11:
						ValueEnum val = (ValueEnum)Enum.Parse(ValueEnum,i.ruleValue);
						valRule4 += val;
						break;
					default:
						break;
					
				}
			}
		}

Still not the prettiest, but should be more manageable.

Spacerat · Dec 14, 2015

maybe a visitor pattern is an easier & less complex solution

FarligOpptreden · Dec 14, 2015

MagicDude4Eva said:
Look at EasyRules (https://github.com/EasyRules/easyrules) or something similar and do not let yourself be talked into buying some BPEL engine for this

...and steer clear of the abortion often known as OpenESB if someone manages to persuade you to go the BPEL route.

Kilgore_Trout_Redux · Dec 14, 2015

Don't you want to start testing or become a business analyst or something?

etienne_marais · Dec 14, 2015

If you don't want to implement a rules engine you can make it a bit more readable as follow:

Code:

foreach (var i in ruleScoringList)
{
    if ( (i.riskCategory == "1") && (i.questionType == "1") && (i.ruleValue == "Yes") ) { rule1 =false;  }
    if ( (i.riskCategory == "7") && (i.questionType == "1") && (i.ruleValue == "Yes") ) { rule1 = false; }
    if ( (i.riskCategory == "7") && (i.questionType == "2") && (i.ruleValue == "None") ) ;
    if ( (i.riskCategory == "7") && (i.questionType == "2") && (i.ruleValue == "Low") ) { valRule2 += 1; }
    // ... etc ...
}

The lines should fit neatly under each other in the IDE and you can space the conditions to be evaluated so that same variables/members are beneath each other as well which is easier on the eye. I left out the 'else' (in 'else if) statements on purpose as the overhead reduction would be neglible to keep it easier on the eye as well.

HideInLight · Dec 14, 2015

My noob attempt at readability.

Code:

    foreach (var i in ruleScoringList)
    {

        switch(i.riskCategory)
        {
            /******************************************************/
            case "1":
                if(i.questionType == "1") && (i.ruleValue == "Yes")
                {
                        rule1 = false;
                }
                break;
            /******************************************************/

            case "7":
                if(i.questionType == "1")  && (i.ruleValue == "Yes")
                {
                        rule1 = false;
                }
                if(i.questionType == "2")
                {
                    switch(i.ruleValue)
                    {
                        case "None":
                            valRule2 = valRule2 + 0;
                            break;
                        case "Low"
                            valRule2 = valRule2 + 1;
                            break;
                        case "Medium"
                            valRule2 = valRule2 +2;
                            break;
                        case "High"
                            valRule2 =  valRule2 +3;
                    }
                }
                break;
            /******************************************************/
            case "10":
                if(i.questionType == "1") && (i.ruleValue == "Yes")
                {
                    rule1 = false;
                }
                if(i.questionType == "2")
                {
                    switch(i.ruleValue)
                    {
                        case "None":
                            valRule3 = valRule3 + 0;
                            break;
                        case "Low"
                            valRule3 = valRule3 + 1;
                            break;
                        case "Medium"
                            valRule3 = valRule3 +2;
                            break;
                        case "High"
                            valRule3 =  valRule3 +3;
                    }
                }
                break;
            /******************************************************/
            case "11":      
                if(i.questionType == "1") && (i.ruleValue == "Yes")
                {
                       rule1 = false;
                }
                if(i.questionType == "2")
                {
                    switch(i.ruleValue)
                    {
                        case "None":
                            valRule4 = valRule4 + 0;
                            break;
                        case "Low"
                            valRule4 = valRule4 + 1;
                            break;
                        case "Medium"
                            valRule4 = valRule4 +2;
                            break;
                        case "High"
                            valRule4 =  valRule4 +3;
                    }
                }
                 break;
            /******************************************************/
        }//switch(i.riskCategory)   
    }//foreach

willo · Dec 15, 2015

Read this. Breathe this. Become this.
http://martinfowler.com/articles/refactoring-adaptive-model.html

semaphore · Dec 15, 2015

etienne_marais said:
If you don't want to implement a rules engine you can make it a bit more readable as follow:

Code:

foreach (var i in ruleScoringList) { if ( (i.riskCategory == "1") && (i.questionType == "1") && (i.ruleValue == "Yes") ) { rule1 =false; } if ( (i.riskCategory == "7") && (i.questionType == "1") && (i.ruleValue == "Yes") ) { rule1 = false; } if ( (i.riskCategory == "7") && (i.questionType == "2") && (i.ruleValue == "None") ) ; if ( (i.riskCategory == "7") && (i.questionType == "2") && (i.ruleValue == "Low") ) { valRule2 += 1; } // ... etc ... }

The lines should fit neatly under each other in the IDE and you can space the conditions to be evaluated so that same variables/members are beneath each other as well which is easier on the eye. I left out the 'else' (in 'else if) statements on purpose as the overhead reduction would be neglible to keep it easier on the eye as well.

You call that readable? That is even worse than OP's.

Pho3nix · Dec 15, 2015

Kilgore_Trout_Redux said:
Don't you want to start testing or become a business analyst or something?

What gave you this idea?

Pho3nix · Dec 15, 2015

semaphore said:
You call that readable? That is even worse than OP's.

Thanks for the compliment?

Pho3nix · Dec 15, 2015

shauntir said:
OMG!

Yes, look into building some kind of rules engine.

From the looks of it, your problem could be a state machine. Have a look at this c# library that could potentially help. https://github.com/nblumhardt/stateless

MagicDude4Eva said:
In Java this is as simple as (you obviously would need to implement RiskRule)

Look at EasyRules (https://github.com/EasyRules/easyrules) or something similar and do not let yourself be talked into buying some BPEL engine for this

Doing some research and fixing all of this.

Sherbang · Dec 15, 2015

/off topic grammer nazi

Thread title should be Any way, two words, not Anyway

semaphore · Dec 15, 2015

Pho3nix said:
Thanks for the compliment?

My pleasure

Join the MyBroadband community

Get started

Anyway to make this code a little easier to read

Pho3nix

The Legend

semaphore

Honorary Master

Pho3nix

The Legend

shauntir

Well-Known Member

MagicDude4Eva

Banned

me_

Expert Member

Spacerat

Expert Member

FarligOpptreden

Executive Member

Kilgore_Trout_Redux

Executive Member

etienne_marais

Honorary Master

HideInLight

Expert Member

willo

Active Member

semaphore

Honorary Master

Pho3nix

The Legend

Pho3nix

The Legend

Pho3nix

The Legend

Sherbang

Executive Member

semaphore

Honorary Master