Skip to main content
Combined both answers into one
Source Link
Phrancis
  • 20.5k
  • 6
  • 70
  • 155

Please keep in mind that I do not speak German, so I used Google Translate. Excuse me if some of the terms are unclear or mistranslated.

#Comments

As written, your English comments are not helpful.

-- Add the parameters for the stored procedure here 

-- declare my intern parameters 

-- set the previous month 

Those are all perfectly obvious from looking at the code. What would be more useful would be if you had comments that explained why you are doing something. Your German comments appear more useful, as you use them to label sections of code, mostly.

#Consistency

While we're on the topic of language, why do you mix English and German naming and comments? I would say pick one and stick to it. For example, @oldDate would instead be something like @altDatum. For comments, you could also make them bilingual, e.g.:

-- Mitarbeiter_Einrichtung (Employee Revenues) 

Your table aliases, while consistent, are not helpful I find. Look at this from the end of your query, for instance:

MSK.Buchung, MSK.Buchungsdatum, MSK.IstStartBuchung, MU.Jahresurlaub, MU.UrlaubGültigAb, MS.Stunden, MS.StundenGültigAb, ME.RefEinrichtungID, ME.EinrichtungGültigAb, MT.RefTarifvertragId, MT.TarifvertragGültigAb, 

One cannot tell just by looking at it what all those aliases mean. I have to go back through the whole code to find that MSK means MitarbeiterStundenkonto. That would be a nightmare for a new person to maintain if the code based is all like that.

This is not only inconsistently space, but it is also very cryptic as to not only what you are doing, but also why.

 ISNULL(JBU.JahresBeginUrlaub, ISNULL(MBU.JahresBeginUrlaub,0)) -- wenn kein Plan abgeschlossen wurde wird versucht der JahresBeginUrlaub des Vorjahres zu addieren + (case when OldPlan.OldCurrentUrlaubskonto IS NULL THEN ISNULL(OJBU.JahresBeginUrlaub,0) else OldPlan.OldCurrentUrlaubskonto END) - ISNULL(PL.PlanUrlaub,0) - ISNULL(Nachtrag.NachtragUrlaub,0) AS Urlaubskonto, 

Please keep in mind that I do not speak German, so I used Google Translate. Excuse me if some of the terms are unclear or mistranslated.

#Comments

As written, your English comments are not helpful.

-- Add the parameters for the stored procedure here 

-- declare my intern parameters 

-- set the previous month 

Those are all perfectly obvious from looking at the code. What would be more useful would be if you had comments that explained why you are doing something. Your German comments appear more useful, as you use them to label sections of code, mostly.

#Consistency

While we're on the topic of language, why do you mix English and German naming and comments? I would say pick one and stick to it. For example, @oldDate would instead be something like @altDatum. For comments, you could also make them bilingual, e.g.:

-- Mitarbeiter_Einrichtung (Employee Revenues) 

Your table aliases, while consistent, are not helpful I find. Look at this from the end of your query, for instance:

MSK.Buchung, MSK.Buchungsdatum, MSK.IstStartBuchung, MU.Jahresurlaub, MU.UrlaubGültigAb, MS.Stunden, MS.StundenGültigAb, ME.RefEinrichtungID, ME.EinrichtungGültigAb, MT.RefTarifvertragId, MT.TarifvertragGültigAb, 

One cannot tell just by looking at it what all those aliases mean. I have to go back through the whole code to find that MSK means MitarbeiterStundenkonto. That would be a nightmare for a new person to maintain if the code based is all like that.

This is not only inconsistently space, but it is also very cryptic as to not only what you are doing, but also why.

 ISNULL(JBU.JahresBeginUrlaub, ISNULL(MBU.JahresBeginUrlaub,0)) -- wenn kein Plan abgeschlossen wurde wird versucht der JahresBeginUrlaub des Vorjahres zu addieren + (case when OldPlan.OldCurrentUrlaubskonto IS NULL THEN ISNULL(OJBU.JahresBeginUrlaub,0) else OldPlan.OldCurrentUrlaubskonto END) - ISNULL(PL.PlanUrlaub,0) - ISNULL(Nachtrag.NachtragUrlaub,0) AS Urlaubskonto, 
added 1 character in body
Source Link
Phrancis
  • 20.5k
  • 6
  • 70
  • 155

Remember what I said about descriptive aliases in my other answer? I havehad to scroll down to line # 397 to find out what the heck PL meant, when I was looking at the above CASE statement.

This is probably a big part of where your query is slowing down. As @200_success pointed out, you should use CTEs or Common Table Expressions. It's a bit difficult to work your way out of subqueries into CTEs, you need to work your way backwards from the "deepest" subquery up to the "shallowest" or earliest query code.

I'm not sure how familiar you are with CTEs, but they help readability a ton, by declaring your subqueries right at the beginning, and you can set meaningful names to reference them later in the script. Extremely handy and much easier to maintain. Now, working your way back from subqueries to CTEs can be challenging, but not impossible. A good approach is to work your way backwards, as I said.

WHERE RefMitarbeiterId = INNERPLAN.RefMitarbeiterId AND GültigAb IN cte_MaxGültigAb 

NOTE: This will likely not improve performance much, but it certainly will improve readability, which in turn makes it more clear where performance issues are.

Remember what I said about descriptive aliases in my other answer? I have to scroll down to line # 397 to find out what the heck PL meant, when I was looking at the above CASE statement.

This is probably a big part of where your query is slowing down. As @200_success pointed out, you should use CTEs or Common Table Expressions. It's a bit difficult to work your way out of subqueries into CTEs, you need to work your way backwards from the "deepest" subquery up to the "shallowest" or earliest query code.

I'm not sure how familiar you are with CTEs, but they help readability a ton, by declaring your subqueries right at the beginning, and you can set meaningful names to reference later in the script. Extremely handy and much easier to maintain. Now, working your way back from subqueries to CTEs can be challenging, but not impossible. A good approach is to work your way backwards, as I said.

AND GültigAb IN cte_MaxGültigAb 

NOTE: This will likely not improve performance, but it certainly will improve readability, which in turn makes it more clear where performance issues are.

Remember what I said about descriptive aliases in my other answer? I had to scroll down to line # 397 to find out what the heck PL meant, when I was looking at the above CASE statement.

As @200_success pointed out, you should use CTEs or Common Table Expressions. It's a bit difficult to work your way out of subqueries into CTEs, you need to work your way backwards from the "deepest" subquery up to the "shallowest" or earliest query code.

I'm not sure how familiar you are with CTEs, but they help readability a ton, by declaring your subqueries right at the beginning, and you can set meaningful names to reference them later in the script. Extremely handy and much easier to maintain. Now, working your way back from subqueries to CTEs can be challenging, but not impossible. A good approach is to work your way backwards, as I said.

WHERE RefMitarbeiterId = INNERPLAN.RefMitarbeiterId AND GültigAb IN cte_MaxGültigAb 

NOTE: This will likely not improve performance much, but it certainly will improve readability, which in turn makes it more clear where performance issues are.

Made a few edits
Source Link
Phrancis
  • 20.5k
  • 6
  • 70
  • 155

Remember what I said about descriptive aliases in my other answer? I have to scroll down to line # 397 to find out what the heck PL meant, when I was looking at the above CASE statement. 

This is probably a big part of where your query is slowing down. As @200_success pointed out, you should use CTEs or Common Table Expressions. It's a bit difficult to work your way out of subqueries into CTEs, you need to work your way backwards from the "deepest" subquery up to the "shallowest" or earliest query code.

 AND GültigAb =( -- SELECT MAX(GültigAb) FROM Mitarbeiter_Einrichtung WHERE RefMitarbeiterId = INNERPLAN.RefMitarbeiterId AND ( ( YEAR(GültigAb) = YEAR(@oldDate) AND MONTH(GültigAb) <= MONTH(@oldDate) ) OR YEAR(GültigAb) < YEAR(@oldDate) ) ) 

I prefer the cte_prefixcte prefix personally, but it really can be named anything, including no prefix. Then just work your way up to the next subquery, and combine subqueriesCTEs together.

Remember what I said about descriptive aliases in my other answer? I have to scroll down to line # 397 to find out what the heck PL meant, when I was looking at the above CASE statement. This is probably a big part of where your query is slowing down. As @200_success pointed out, you should use CTEs or Common Table Expressions. It's a bit difficult to work your way out of subqueries into CTEs, you need to work your way backwards from the "deepest" subquery up to the "shallowest" or earliest query code.

 AND GültigAb =( -- SELECT MAX(GültigAb) FROM Mitarbeiter_Einrichtung WHERE RefMitarbeiterId = INNERPLAN.RefMitarbeiterId AND ( ( YEAR(GültigAb) = YEAR(@oldDate) AND MONTH(GültigAb) <= MONTH(@oldDate) ) OR YEAR(GültigAb) < YEAR(@oldDate) ) ) 

I prefer the cte_prefix personally, but it really can be named anything, including no prefix. Then just work your way up to the next subquery, and combine subqueries.

Remember what I said about descriptive aliases in my other answer? I have to scroll down to line # 397 to find out what the heck PL meant, when I was looking at the above CASE statement. 

This is probably a big part of where your query is slowing down. As @200_success pointed out, you should use CTEs or Common Table Expressions. It's a bit difficult to work your way out of subqueries into CTEs, you need to work your way backwards from the "deepest" subquery up to the "shallowest" or earliest query code.

 AND GültigAb =( SELECT MAX(GültigAb) FROM Mitarbeiter_Einrichtung WHERE RefMitarbeiterId = INNERPLAN.RefMitarbeiterId AND ( ( YEAR(GültigAb) = YEAR(@oldDate) AND MONTH(GültigAb) <= MONTH(@oldDate) ) OR YEAR(GültigAb) < YEAR(@oldDate) ) ) 

I prefer the cte prefix personally, but it really can be named anything, including no prefix. Then just work your way up to the next subquery, and combine CTEs together.

Source Link
Phrancis
  • 20.5k
  • 6
  • 70
  • 155
Loading