Indented as Intended or Intended as Indented?

[BernieTheBernie]

Indentation can be fun. What did Johny intend when he indented his code like that:

/// <summary>
/// create the user selected capture device.
/// </summary>
/// <param name="mon"></param>
void CreateCaptureDevice()
{
    object capObj = null;
    try
    {
        Guid gbf = typeof(IBaseFilter).GUID;
        ActDev.Mon.BindToObject(null, null, ref gbf, out capObj);
        capFilter = (IBaseFilter)capObj;
        capObj = null;
    }
    finally
    {
        if (capObj != null)
            Marshal.ReleaseComObject(capObj); capObj = null;
    }

}

Bernie still doesn't know what happens to that parameter mon.
But finally, he can be sure that capObj will be null eventually.

[TheCPUWizard]

@berniethebernie - Not spotting an issue with the indent, but the usage is not improper...

If work does not succeed in assigning capFilter, then capObj will be released if it was actually created....

[antiquarian]

@berniethebernie said in Indented as Intended or Intended as Indented?:

if (capObj != null)
            Marshal.ReleaseComObject(capObj); capObj = null;

I'm not normally in favor of capital punishment, but I think in this case I can make an exception.

[TwelveBaud]

@thecpuwizard said in Indented as Intended or Intended as Indented?:

If work does not succeed in assigning capFilter, then capObj will be released if it was actually created

But that use case doesn't make sense. You're saying someone would register a COM object under a moniker for IBaseFilter that could QI from IUnknown to IBaseFilter but not from IBaseFilter to itself?

[TheCPUWizard]

@twelvebaud said in Indented as Intended or Intended as Indented?:

But that use case doesn't make sense. You're saying someone would register a COM object under a moniker for IBaseFilter that could QI from IUnknown to IBaseFilter but not from IBaseFilter to itself?

No I am saying that it is POSSIBLE to do so, and that the code presented would be a means of protecting against it. Now this occurring should be rare, so I would not expect such code to be the default....But I can imagine it occurring because at one time someone did exactly that.

[accalia]

@antiquarian said in Indented as Intended or Intended as Indented?:

@berniethebernie said in Indented as Intended or Intended as Indented?:

if (capObj != null)
            Marshal.ReleaseComObject(capObj); capObj = null;

I'm not normally in favor of capital punishment, but I think in this case I can make an exception.

it took me a long time to see it but when i did i facepunched.

[BernieTheBernie]

@antiquarian Gratulation! At least one reader of the daily WTF to spot it. Yes, it is hidden, and the hint with indentation didn't help with most readers...
By the way, why set that local variable capObj to null in the finally block? It runs out of scope when the function is left, and will be cleaned up by GC anyway.

[TheCPUWizard]

@berniethebernie said in Indented as Intended or Intended as Indented?:

It runs out of scope when the function is left, and will be cleaned up by GC anyway.

Yes capObj goes out of scope, but NOTHING is related to GC since the code only exists if the object has been successfully assigned to capFilter which servers as a live reference.

So, the null assignment does nothing in the current code, but it does provide a safety against future changes. If the code is modified so that there is an attempt to reference capObj while still in CreateCaptureDevice after the try/finally, compare the difference in behavior (and the effort of trouble shooting required...

[BernieTheBernie]

@thecpuwizard said in Indented as Intended or Intended as Indented?:

safety against future changes

Oh I see: I should take an even closer look. There is an empty line between the closing } of the finally block and the closing } of the function. Some invisible code could be hidden there. I'll investigate.

[TheCPUWizard]

@berniethebernie said in Indented as Intended or Intended as Indented?:

Oh I see: I should take an even closer look. There is an empty line between the closing } of the finally block and the closing } of the function. Some invisible code could be hidden there. I'll investigate.

Are you 100% confident (say to the point of making a bet equal to your annual pay) that nobody will even edit a method and add code at the bottom without considering the ramifications of the existing code????

If you are, then please give me access to some of your code for 5 minutes so I can collect on the bet. :)

[Tsaukpaetra]

@thecpuwizard said in Indented as Intended or Intended as Indented?:

If you are, then please give me access to some of your code for 5 minutes

Sure. Make this even more broken for me please:

        // POST: CloudVM_Builds/Edit/5
        // To protect from overposting attacks, please enable the specific properties you want to bind to, for 
        // more details see http://go.microsoft.com/fwlink/?LinkId=317598.
        [HttpPost]
        [ValidateAntiForgeryToken]
        public ActionResult Edit([Bind(Include = "ID,Name,MinOnDeck,LevelWeightCapacity,Properties")] buildData buildDat, int id)
        {
            //Find the build 
            ComputeCloudBuild build = db.ComputeCloudBuilds.Include("ComputeCloudBuildProperties").Where( x => x.ID == id).FirstOrDefault();
            if (build == null || buildDat.ID != id)
                return HttpNotFound();

            if (ModelState.IsValid)
            {
                //Main editable things
                build.Name = buildDat.Name;
                build.LevelWeightCapacity = buildDat.LevelWeightCapacity;
                build.MinOnDeck = buildDat.MinOnDeck;
                
                //Build properties
                foreach (var item in buildDat.Properties)
                {
                    var prop = build.ComputeCloudBuildProperties.Where(x => x.PropertyID == item.Key).FirstOrDefault();
                    if(prop != null)
                    {
                        prop.Value = item.Value;
                    }
                }


                db.SaveChanges();
                return RedirectToAction("Index");
            }
            ViewBag.CloudType = new SelectList(db.ComputeCloudTypes, "ID", "CloudName", buildDat.CloudType);
            return View(build);
        }

[TheCPUWizard]

@tsaukpaetra said in Indented as Intended or Intended as Indented?:

Sure. Make this even more broken for me please:

That one is easy. Look at the last two lines, they only get executed if the model is invalid (and the initial validation on the returned initial build passed).

One can overlook the return statement inside of the if statement.

Thus code can be added here and not necessarily have the desired effect.

Do I think the code is "broken"? NO (At least at the level I looked while having my first morning coffee).
Do I think that the code is as robust as possible? NO
Do I have a clue, if for your environment that is "good" or "bad"? NO

[dkf]

@berniethebernie said in Indented as Intended or Intended as Indented?:

Bernie still doesn't know what happens to that parameter mon.

What should he have been using instead?

[TheCPUWizard]

@berniethebernie said in Indented as Intended or Intended as Indented?:

Bernie still doesn't know what happens to that parameter mon.

Almost certainly the developer is lazy/sloppy and ignoring warning during compiles.... Why, Oh Why is "Treat Warnings as Errors" the default????

[Tsaukpaetra]

@thecpuwizard said in Indented as Intended or Intended as Indented?:

NO

Yeah, it's slightly modified from the template MVC gives you when you scaffold from Entity Framework in an Asp.Net project.

[TheCPUWizard]

@tsaukpaetra said in Indented as Intended or Intended as Indented?:

@thecpuwizard said in Indented as Intended or Intended as Indented?:

NO

Yeah, it's slightly modified from the template MVC gives you when you scaffold from Entity Framework in an Asp.Net project.

Not sure which of the 3 "NO" occurrences you are quoting....

That being said, the templates from MVC are in a condition where I almost never use them [sometimes I do not have a choice] preferring instead other templates that produce code which is warning free [even in Code Analysis or Equiv for a pretty tight rule set] and are also structured to provide SOLID basis for usage.

[Tsaukpaetra]

@thecpuwizard said in Indented as Intended or Intended as Indented?:

@tsaukpaetra said in Indented as Intended or Intended as Indented?:

@thecpuwizard said in Indented as Intended or Intended as Indented?:

NO

Yeah, it's slightly modified from the template MVC gives you when you scaffold from Entity Framework in an Asp.Net project.

Not sure which of the 3 "NO" occurrences you are quoting....

That being said, the templates from MVC are in a condition where I almost never use them [sometimes I do not have a choice] preferring instead other templates that produce code which is warning free [even in Code Analysis or Equiv for a pretty tight rule set] and are also structured to provide SOLID basis for usage.

Agreed, but in this specific case I just needed something really quick and dirty to edit a table, which also happened to also be modifying a sub table that gets some of its values from another table, which table can optionally get allowed values from a sub table again.

Will post pics on request.

[TheCPUWizard]

@tsaukpaetra said in Indented as Intended or Intended as Indented?:

Will post pics on request.

No need. My prior posts were all about risk and risk tolerance level. Risk can never be eliminated, but the cost of reducing tends to increase as the easy ones to mitigate are addressed and you are left with the hard ones. Thus understanding how much risk and what type you are willing to accept is key.

[Tsaukpaetra]

@thecpuwizard said in Indented as Intended or Intended as Indented?:

Thus understanding how much risk and what type you are willing to accept is key.

Yeah. Technically speaking, any user with a login can (currently, will be locked down later) access this page and screw up our Azure build configurations for our Master Server.

But the risk is fairly low because you'd need to know the controller existed, need to know how to log into the site (cookie sniffing would be an excellent vector, assuming you're MITM-ing well enough), know what all the parameters mean, and then change them. The risk that there are unauthorized people wanting to do that is... minimal.

Is it acceptable? For now as it's in development (despite that this site is actually currently public on the web), sure. Once it goes to prod though, things will have been done a bit better (for instance, HTTPS is required for everything but the landing page).

I'd be pretty stoked though if a user figured out that they were getting auto-gen accounts and technically could access the site if they were so inclined, there's quite a bit of non-VR stuff they can do there.

[TheCPUWizard]

@tsaukpaetra said in Indented as Intended or Intended as Indented?:

(currently, will be locked down later)

If I had $200 for every time I heard that - and it did NOT happen, I would be able to retire on those funds alone. I know (first hand) that at least 2 of the top 15 "information thefts" in the last 5 years occurred for that specific reason [the exploit ws known to exist, and there were documents how to fix prepared, but the decision to implement was deferred to "later"

From my perspective [this is subjective] this is a vary high risk, one that is difficult (technically and business) to properly fix afterwards.

There is a well known saying of "you can add on quality, it has to be designed in".

[Tsaukpaetra]

@thecpuwizard said in Indented as Intended or Intended as Indented?:

every time I heard that - and it did NOT happen

Well, for the most part at the class level it will be just decorating everything with all the fluff, which is a fairly boring and mechanical operation that (in theory) could be done automagically, but at the moment isn't and resources won't be spent to figure out how to so do.

So, nearing the completion of this feature I'll be spending a day (or two) just decorating everything, publishing it up on dev, and then doing basic checks that all my fluff was worded correctly (an annoying aspect for something with dynamically defined roles, but not dynamically assigned permissions for roles ).

Having such a small team (of one) means it's all up to me, and I try to stick to the plan when possible...

[sloosecannon]

@antiquarian said in Indented as Intended or Intended as Indented?:

I can make an exception.

In the finally?

Surely that's not advised...

[M_Adams]

@thecpuwizard said in Indented as Intended or Intended as Indented?:

There is a well known saying of "you can add on quality, it has to be designed in".

Also:

There's never time to do it right, but always time to do it over.

[Tsaukpaetra]

@m_adams said in Indented as Intended or Intended as Indented?:

@thecpuwizard said in Indented as Intended or Intended as Indented?:

There is a well known saying of "you can add on quality, it has to be designed in".

Also:

There's never time to do it right, but always time to do it over.

That... almost sounds backwards...

[M_Adams]

@tsaukpaetra said in Indented as Intended or Intended as Indented?:

That... almost sounds backwards...

Yes. Yes it is. It's also heard in the form of "Just get it in the can. We can fix it in the post." ... Then the priest's "boner" shocks all the little girls watching Little Mermaid....

[Tsaukpaetra]

@m_adams said in Indented as Intended or Intended as Indented?:

@tsaukpaetra said in Indented as Intended or Intended as Indented?:

That... almost sounds backwards...

Yes. Yes it is. It's also heard in the form of "Just get it in the can. We can fix it in the post." ... Then the priest's "boner" shocks all the little girls watching Little Mermaid....

And turns them into scalies, right?

[M_Adams]

@tsaukpaetra said in Indented as Intended or Intended as Indented?:

scalies

[Tsaukpaetra]

@m_adams said in Indented as Intended or Intended as Indented?:

@tsaukpaetra said in Indented as Intended or Intended as Indented?:

scalies

Furries, but for the scaled folk (i.e. fishes, lizards, the like).

[M_Adams]

@tsaukpaetra I think you may need to check some of your cognitive circuits...
That train of thought sounds like one my husband would be boarding... :)

[Tsaukpaetra]

@m_adams said in Indented as Intended or Intended as Indented?:

I think you may need to check some of your cognitive circuits...

Error while running integrity check on High Level systems: A Diagnostics session cannot be started while it is in progress!
Notice: Current SysDiag Session ID 19subL is currently InProgress, estimated 1 of 4 tasks completed.
Notice: Proper Planning Prevents Peruvian Prostitution Privacy Problems.
Error: Cyclic reference dereferring log output.

I'm not sure. Here's what found on the web:

Scalie

Would you like to open it?

[Onyx]

@tsaukpaetra said in Indented as Intended or Intended as Indented?:

I'm not sure. Here's what found on the web:

Scalie - WikiFur, the furry encyclopedia

Would you like to open it?

Oh good, he rebooted in personal assistant mode... Who was messing with @Tsaukpaetra's boot time parameters? 'Fess up!

[BernieTheBernie]

Beyond the indentation issue which suggests that capObj = null in the finally block will be executed only when the condition is true (but it will be executed anyway), there are some more issues. Issues with readability of that convolutionary code.

Have you ever looked at the "happy path" scenario, i.e. when no exception happened? Then the condition in the finally block is always false because capObj is set to null before the finally block is entered. That means the value of that variable is abused as a signal.

A minor issue from an OO point of view, but major from a functional programming point of view is the void return value of the function. It prefers to change the state of the instance (by setting a member variable) instead of returning the created item.

Also, the variables could have better naming. And the function itself: it creates a Filter, not a CaptureDevice. But it's copy-paste code, used in several places inside that class.

I guess the code becomes clearer after some refactoring (perhaps even more refactoring is possible):

private IBaseFilter CreateFilter(IMoniker moniker)
{
    object tmp = null;
    try
    {
        Guid filterId = typeof(IBaseFilter).GUID;
        moniker.BindToObject(null, null, ref filterId, out tmp);
        IBaseFilter filter = (IBaseFilter)tmp;
        return filter;
    }
    catch
    {
        if (tmp != null)
        {
            Marshal.ReleaseComObject(tmp);
        }
        throw;
    }
}

Now the function does not depend on the current state of the object anymore, its purpose is clearly indicated, and the actions for an exceptional situation are placed where such actions are expected.

Object Orientation is hard for low-level-coders.

[accalia]

@onyx said in Indented as Intended or Intended as Indented?:

Who was messing with @Tsaukpaetra's boot time parameters? 'Fess up!

Kyon?