Unconditionally conditional query composition

amyb

This is really elementary stuff, but I acually said "WTF" (in longhand... voice?) when I saw it during a routine peer code review, so naturally, I had to immediately post it here.

            if ($this->hasSomeCondition){
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            } else {
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";

            }

And a bonus- found within the same class, different method. (it went to production like this)...

$oldFileName = $olddir .'/'.$oldfile.'.'.$ext;
$newFileName = '***';

[snip...]

$dl = $downloads -> publishFile($oldFileName, $newFileName);

omgLOL

Hmm i don't see the bonus WTF, the *** parameter could be a signal for the publishFile method to just keep the old name? Maybe it would be better to make a separate parameter for that, but not really a big WTF don't you think? Or am i missing something?

@omgLOL said:

the *** parameter could be a signal for the publishFile method to just keep the old name

Because magic strings are a good idea.

toon

@omgLOL said:

Hmm i don't see the bonus WTF, the *** parameter could be a signal for the publishFile method to just keep the old name? Maybe it would be better to make a separate parameter for that, but not really a big WTF don't you think? Or am i missing something?

Yes, I'd say you're missing something. I'd go for a construction like this:

 $dl = $downloads -> publishFile($oldFileName, DownloadsClass::KEEP_ORIGINAL_NAME);

or maybe this is even better (but of course, I'm just a developer, not a software architect):

 $dl = $downloads -> publishFile($oldFileName, $oldFileName);

EDIT: maybe a default argument value is even better yet:

$dl = $downloads -> publishFile($oldFileName);

presidentsdaughter

I'm starting to see this pattern almost every day.

From the code I am now responsible for:

  //snip... int objectType = getObjectTypeForItem(item);

 if(objectType >= 0) {
    result =  updateTable(item, objectType);
 } else if( objecType == Const.OBJECT_TYPE_NA) { //some negative constant meaning not applicable
    result = updateTable(item, objectType);
 }
 return result;
}

Before you ask, no, there aren't any other possible values for objectType.

LoremIpsum

And don't forget the potential for SQL injection for itemId (unless the rest of the source code proves otherwise)!

TGV

@edgsousa said:

if(objectType >= 0) {

It's rare that someone manages to make 0 a magic number.

And how you have a namespace Const. I can see this becoming the new Hungarian Notation!

namespace Const

namespace Const.Int

namespace Const.String

namespace GlobalVar

namespace GlobalVar.Int

...

RaceProUK

@TGV said:

@edgsousa said:
if(objectType >= 0) {
It's rare that someone manages to make 0 a magic number.

Not really: C devs are used to #define NULL 0.

Windows devs are also used to #define S_OK 0.

PJH

@RaceProUK said:

C devs are used to #define NULL 0.

That's C++. C is #define NULL ((void*)0)

Evo

@PJH said:

@RaceProUK said:
C devs are used to #define NULL 0.
That's C++. C is #define NULL ((void*)0)

Completely off topic, though way more interesting than staying on topic.
I love the signature even more than a few months ago. Everybody did stop talking about the mayans, except for your signature.

amyb

@omgLOL said:

Hmm i don't see the bonus WTF, the *** parameter could be a signal for the publishFile method to just keep the old name? Maybe it would be better to make a separate parameter for that, but not really a big WTF don't you think? Or am i missing something?

In my haste to share, I forgot to mention that the tempfile resides on a linux filesystem, and the publishFile() method handling its creation/removal does so via system()...

toon

@amyb said:

he tempfile resides on a linux filesystem, and the publishFile() method handling its creation/removal does so via system()...

Reminds me of this horror story.

amyb

@toon said:

@amyb said:
he tempfile resides on a linux filesystem, and the publishFile() method handling its creation/removal does so via system()...

Reminds me of this horror story.

Yeah... I actually did that as a brand-new Linux N00b (13-14 years ago now...) Thankfully, it was in an educational environment. No actual production systems were harmed in the learning of that lesson ;)

C_Octothorpe

@amyb said:

@toon said:
@amyb said:
he tempfile resides on a linux filesystem, and the publishFile() method handling its creation/removal does so via system()...

Reminds me of this horror story.

Yeah... I actually did that as a brand-new Linux N00b (13-14 years ago now...) Thankfully, it was in an educational environment. No actual production systems were harmed in the learning of that lesson ;)

I too did that about 15 or so years ago on a Win95 machine. I was running out of HD space, and I noticed that this pesky System32 folder had lots of excess files that were quite large. SHIFT+DELETE...

Evo

@C-Octothorpe said:

@amyb said:
@toon said:
@amyb said:
he tempfile resides on a linux filesystem, and the publishFile() method handling its creation/removal does so via system()...

Reminds me of this horror story.

Yeah... I actually did that as a brand-new Linux N00b (13-14 years ago now...) Thankfully, it was in an educational environment. No actual production systems were harmed in the learning of that lesson ;)
I too did that about 15 or so years ago on a Win95 machine. I was running out of HD space, and I noticed that this pesky System32 folder had lots of excess files that were quite large. SHIFT+DELETE...

When I was young (also 15 or so, probably Windows XP) and I'd never used Linux before I got pissed off by Windows. I wanted to delete the temporary internet files as it was apparently quite big, but I couldn't delete it, Windows wouldn't let me. Out of frustration I opened the registry editor, searched for "Temporary Internet Files", deleted all the keys I could find (until I was fed up with pressing I believe F3, del, enter shitloads of times), and rebooted the computer.

I managed to log in and everything looked fine (I can't remember if I could actually delete the folder then, but probably not). After rebooting again and logging in again, Windows crashed and booted again. Logged into another profile: worked fine. After a reboot logged into the same profile: crash!

Every profile worked exactly once after that.

I didn't care one bit though. I just started setting up Gentoo. Yup, as my first distro. Took me days to get working, but it tought my heaps about Linux.

amyb

@omgLOL said:

Hmm i don't see the bonus WTF, the *** parameter could be a signal for the publishFile method to just keep the old name? Maybe it would be better to make a separate parameter for that, but not really a big WTF don't you think? Or am i missing something?

That is literally the name of the file: ***

as seen here, in the PUBLICLY ACCESSIBLE web root at that. note the conspicuous absence of any other regular files. That is because the process first created a file named '***', did system("rm -f '***'"), then before the thread ended, RE-created the file... why? I have no idea...

drwxrwsr-x 3 someuser nobody 4096 Nov 8 14:30 webService
-rw-r--r-- 1 nobody nobody 474340 Jan 23 10:20 ***
drwxrwsr-x 3 someuser nobody 4096 Jan 23 10:24 downloadfiles
drwxrwsr-x 5 someuser nobody 4096 Jan 25 15:51 css

Soviut

@Evo said:

@C-Octothorpe said:
@amyb said:
@toon said:
@amyb said:
he tempfile resides on a linux filesystem, and the publishFile() method handling its creation/removal does so via system()...

Reminds me of this horror story.

Yeah... I actually did that as a brand-new Linux N00b (13-14 years ago now...) Thankfully, it was in an educational environment. No actual production systems were harmed in the learning of that lesson ;)
I too did that about 15 or so years ago on a Win95 machine. I was running out of HD space, and I noticed that this pesky System32 folder had lots of excess files that were quite large. SHIFT+DELETE...

When I was young (also 15 or so, probably Windows XP) and I'd never used Linux before I got pissed off by Windows. I wanted to delete the temporary internet files as it was apparently quite big, but I couldn't delete it, Windows wouldn't let me. Out of frustration I opened the registry editor, searched for "Temporary Internet Files", deleted all the keys I could find (until I was fed up with pressing I believe F3, del, enter shitloads of times), and rebooted the computer.

I managed to log in and everything looked fine (I can't remember if I could actually delete the folder then, but probably not). After rebooting again and logging in again, Windows crashed and booted again. Logged into another profile: worked fine. After a reboot logged into the same profile: crash!

Every profile worked exactly once after that.

I didn't care one bit though. I just started setting up Gentoo. Yup, as my first distro. Took me days to get working, but it tought my heaps about Linux.

You couldn't handle the relative simplicity of Windows, so you decided to install Linux instead?

Cassidy

@amyb said:

Yeah... I actually did that as a brand-new Linux N00b ... No actual production systems were harmed in the learning of that lesson ;)

Of course not - because Linux isn't used in production systems, duh.

Mcoder

@Soviut said:

You couldn't handle the relative simplicity of Windows, so you decided to install Linux instead?

That's the problem, Windows has a kind of "simplicity" that is anything, but simple.

blakeyrat

@Mcoder said:

That's the problem, Windows has a kind of "simplicity" that is anything, but simple.

Oh man I want to make fun of that comma splice so badly...

Anyway, I agree with you: both Windows and Linux are shit for this. The OS consists of 30,000 random tiny files, scattered all over, with no organization, and no way of finding out any given file's actual purpose.

The Mac Classic way was far superior. All the required system software was in one single file, named "System". Later they added a second file with hardware-specific drivers, but your computer would still boot without it. Finder was a third file. Guess what? All you needed to boot was System and Finder. Boom. Done.

OS X could do that using bundles if they gave a shit, but it's run by Unix people now so of course they don't. Windows... eh, Windows tries to solve the problem by restricting access to the folders the necessary files are in, but it still has rare times when you *need* to dive into that folder so they can't lock it away completely. Anyway.

FragFrog1

@toon said:

EDIT: maybe a default argument value is even better yet:

$dl = $downloads -> publishFile($oldFileName);

Assuming this is PHP, method overloading is not really a viable option. It can be done, but the hoops you have to spring through don't exactly make it the simpler solution.

@amyb said:

            if ($this->hasSomeCondition){
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            } else {
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            }

Ah, I see the WTF. This should of course be

            if ($this->hasSomeCondition){
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            } else if (!($this->hasSomeCondition)){
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            } else {
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            }

toon

@FragFrog said:

@toon said:
EDIT: maybe a default argument value is even better yet:

$dl = $downloads -> publishFile($oldFileName);
Assuming this is PHP, method overloading is not really a viable option. It can be done, but the hoops you have to spring through don't exactly make it the simpler solution.

Who's talking about method overloading? AFAIK you can't have multiple methods of the same name in a class in PHP, if that's what you mean. I just meant that the second parameter gets a default value, say: false. So if the new name isn't specified, you mean that the old file should be published, but under the same name. Makes for more readable code.

toon

@Anonymouse said:

@amyb said:
            if ($this->hasSomeCondition){
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            } else {
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            }

Ah, I see the WTF. This should of course be
            if ($this->hasSomeCondition){
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            } else if (!($this->hasSomeCondition)){
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            } else {
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            }

	if ($this->hasSomeCondition){
		$query = "SELECT * from myTable WHERE itemId = '$itemId'";
	} else if (!($this->hasSomeCondition)){
		$query = "SELECT * from myTable WHERE itemId = '$itemId'";
	} else if ($this->hasSomeCondition == FILE_NOT_FOUND){
		$query = "SELECT * from myTable WHERE itemId = '$itemId'";
	} else {
		$query = "SELECT * from myTable WHERE itemId = '$itemId'";
	}

FragFrog1

@toon said:

Who's talking about method overloading? AFAIK you can't have multiple methods of the same name in a class in PHP, if that's what you mean.

You can, of sorts; using the magic __call method and checking the number and or type of arguments passed to it, it is possible to emulate overloading - but as you can imagine, that ain't pretty. I see what you meant with simply using a default value instead now though, that would indeed be a neat solution here :)

error

@Anonymouse said:

@amyb said:
            if ($this->hasSomeCondition){
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            } else {
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            }

Ah, I see the WTF. This should of course be
            if ($this->hasSomeCondition){
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            } else if (!($this->hasSomeCondition)){
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            } else {
                $query = "SELECT * from myTable WHERE itemId = '$itemId'";
            }

You of course have to prepare for:

public bool hasSomeCondition {
    get { return new Random().Next( 0, 2 ) == 1; }
}

amyb

Yeah... The monolith in 2010 was "simple" too :D

amyb

Oh, that's no problem either...

Just rename one of the methods...

doSomething() becomes _doSomething()

Surely, that won't cause problems... ;)

morbiuswilters

@LoremIpsumDolorSitAmet said:

And don't forget the potential for SQL injection for itemId (unless the rest of the source code proves otherwise)!

Even if itemId is escaped, it's asinine not to be using prepared statements in 2013.

EDIT: Goddammit, it was a rezzed thread.

locallunatic

@morbiuswilters said:

Even if itemId is escaped, it's asinine not to be using prepared statements in 2013.

EDIT: Goddammit, it was a rezzed thread.

But it was at least from 2013 rather than one of the real old ones so your statement would still apply.

morbiuswilters

@locallunatic said:

@morbiuswilters said:
Even if itemId is escaped, it's asinine not to be using prepared statements in 2013.

EDIT: Goddammit, it was a rezzed thread.

But it was at least from 2013 rather than one of the real old ones so your statement would still apply.

True, although my statement would apply from about 2000 BC on.

Ben L.

@morbiuswilters said:

@locallunatic said:
@morbiuswilters said:
Even if itemId is escaped, it's asinine not to be using prepared statements in 2013.

EDIT: Goddammit, it was a rezzed thread.

But it was at least from 2013 rather than one of the real old ones so your statement would still apply.

True, although my statement would apply from about 2000 BC on.

I call bullshit.

SQL wasn't invented until 1992 BC