Selecting Employee details from a complicated schema

I have a "litte" problem with my stored procedure because I need some values selected at a point in time.

I need to do many select and group by's in my Left outer join which looks and feels like I am doing something really wrong. It would be very nice if some one could take a look at it.

In this Select there are at least 13 Tables involved:

```
-- Add the parameters for the stored procedure here
@Wann AS datetime2(7) = null,
@EinrichtungId AS int = null,
@MitarbeiterId AS int = null

AS

-- if there was no submitted datetime set it
SET @Wann = ISNULL(@Wann,GETDATE())

-- declare the previous month parameter
DECLARE @oldDate AS datetime2(7)= null;

-- set the previous month
SET @oldDate= DATEADD(month, -1, @Wann);

SELECT Mitarbeiter.MitarbeiterId,
Mitarbeiter.Personalnummer,
Mitarbeiter.Vorname,
Mitarbeiter.Nachname,
Mitarbeiter.IsAktiv,
Mitarbeiter.freierMa,
Mitarbeiter.IsFuehrungskraft,

Mitarbeiter.Eintrittsdatum,
Mitarbeiter.Austrittsdatum,
Mitarbeiter.Eintrittsurlaub,
Mitarbeiter.Austrittsurlaub,

Mitarbeiter.RefQualifikationId,

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,

Nachtrag.NachtragStunden,

-- tries to find the last PlanKrank
ISNULL(PKTable.PlanKrank,0) AS PlanKrank,

-- tries to find the last AusbezahltMonat
ISNULL(PL.AktuellAusbezahltMonat,OldPlan.OldAusbezahltMonat) AusbezahltVormonat,

CASE WHEN Pl.CurrentStundenKonto IS NOT NULL THEN Pl.CurrentStundenKonto +ISNULL(PL.AktuellAusbezahltMonat,0)-1 + ISNULL(Nachtrag.NachtragStunden,0)-1
WHEN OldPlan.OldCurrentStundenKonto IS NOT NULL T

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,

ISNULL()

I noticed a lot of ISNULL() checks that could likely be eliminated. In some cases, it doesn't seem to even make sense with what your comments say your code is doing.

-- tries to find the last PlanKrank
    ISNULL(PKTable.PlanKrank,0) AS PlanKrank,

What this actually does is replace any NULL value with 0. This would make sense if you are outputting to a process that cannot take NULL as an input, or if a NULL value would mess up calculations. I would try to eliminate some of those that are not needed.

Calculations using CASE WHEN

This operation could use some clarification. Think if a new DBA came in and had to maintain your code. Would that leave them scratching their head?

CASE WHEN Pl.CurrentStundenKonto IS NOT NULL         THEN Pl.CurrentStundenKonto +ISNULL(PL.AktuellAusbezahltMonat,0)*-1 + ISNULL(Nachtrag.NachtragStunden,0)*-1 
         WHEN OldPlan.OldCurrentStundenKonto IS NOT NULL THEN OldPlan.OldCurrentStundenKonto+ ISNULL(PL.PlanStunden,0)
         WHEN PL.PlanStunden IS NOT NULL                 THEN  Mitarbeiter.StundenKonto + PL.PlanStunden
         ELSE Mitarbeiter.StundenKonto
    END AS StundenKontoVormonat,

What it appears to do is:

-
If CurrentStundenKonto ("Current hours account"), in other word if there are no hours, you set it to 0.

-
If AktuellAusbezahltMonat ("Currently Paid out month") is null you also set it to 0, then multiply by -1 which gives you either 0 or a negative value.

-
If NachtragStunden ("Additional Hours" guessing it is the same as "Overtime") is null you also set it to 0, and multiply by -1 again so you have either 0 or a positive value.

Else...

• If Old current hours are not null, you add those up to PlanStunden ("Schedule hours") or 0, which then gives you either 0 or a positive value.

Else...

• If Schedule hours are not null, then you add those up or Current hours, which gives you a positive value.

Finally...

• If none of the above are true, then you use the value in the Mitarbeiter table.

And all that gives you the hours account for each employee in the last 30 days. Think there is a way to simplify this logic? Let's see. In all cases, but one, you are adding either 0 or positive values. I don't know enough about your schema to offer an actual rewritten code, but hopefully you can work it out. Sometimes breaking things down to plain English (or German) can help you see flaws / potential simplification. Remember, KISS!

The elephant in the room

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" s

StackExchange Code Review Q#72031, answer score: 2