Ekultek / Dagon

Advanced Hash Manipulation
172 stars 54 forks source link

Remove unnecessary second regex check #23

Closed delirious-lettuce closed 7 years ago

delirious-lettuce commented 7 years ago

In some cases, regex.match(hash_to_verify) is being run twice unnecessarily.

This PR fixes that issue.

Ekultek commented 7 years ago

Let me push to the repo before merging it

Ekultek commented 7 years ago

Fix the conflicts for the new verbose flag and I'll merge it

delirious-lettuce commented 7 years ago

@Ekultek ,

Ok, let me take a quick look.

Ekultek commented 7 years ago

Sounds good

On Jul 2, 2017, at 5:11 PM, delirious-lettuce notifications@github.com wrote:

@Ekultek ,

Ok, let me take a quick look.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

delirious-lettuce commented 7 years ago

@Ekultek ,

I'm not sure about your commit.

    for regex in HASH_TYPE_REGEX:
        if verbose is True:
            LOGGER.debug("Testing: {}".format(HASH_TYPE_REGEX[regex]))
        if regex.match(hash_to_verify) and least_likely:
            if verbose is True:
                LOGGER.debug("Testing: {}".format(HASH_TYPE_REGEX[regex]))
            return HASH_TYPE_REGEX[regex]
        elif regex.match(hash_to_verify) and not least_likely:
            if verbose is True:
                LOGGER.debug("Testing: {}".format(HASH_TYPE_REGEX[regex]))
            return HASH_TYPE_REGEX[regex][0]

The if verbose is True part is repeated three times with the same message. I'm not sure why the second and third message (inside if there is a regex match) are even needed...? Shouldn't the first one above the regex check be enough? Or should it only log if there is a regex match?

Also, I was reading PEP8 again yesterday and noticed that it is frowned upon to use is True for a boolean.

Yes: if greeting:

No: if greeting == True:

Worse: if greeting is True:

https://pep8.org/#programming-recommendations

delirious-lettuce commented 7 years ago

I could write it like this:

for regex, hash_types in HASH_TYPE_REGEX.iteritems():
    if verbose:
        LOGGER.debug("Testing: {}".format(hash_types))
    if regex.match(hash_to_verify):
        return hash_types if least_likely else hash_types[0]

or like this:

for regex, hash_types in HASH_TYPE_REGEX.iteritems():
    if regex.match(hash_to_verify):
        if verbose:
            LOGGER.debug("Testing: {}".format(hash_types))
        return hash_types if least_likely else hash_types[0]
Ekultek commented 7 years ago

Oops, forgot to remove that, knew I had some commits to push and forgot to check all of them lol my bad, do what you need to do

On Jul 2, 2017, at 5:27 PM, delirious-lettuce notifications@github.com wrote:

I could write it like this:

for regex, hash_types in HASH_TYPE_REGEX.iteritems(): if verbose: LOGGER.debug("Testing: {}".format(hash_types)) if regex.match(hash_to_verify): return hash_types if least_likely else hash_types[0] or like this:

for regex, hash_types in HASH_TYPE_REGEX.iteritems(): if regex.match(hash_to_verify): if verbose is True: LOGGER.debug("Testing: {}".format(hash_types)) return hash_types if least_likely else hash_types[0] — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

delirious-lettuce commented 7 years ago

@Ekultek ,

I was asking because I don't know which way you want it to be. If you want the log to show every different hash that's being tested, it seems like having the message above the regex check if preferable. But if you want only the matches shown, having the message underneath the regex check if preferable.

Just guessing, but the message seems to imply you want them all shown, not just the matches. In other words, remove the log message from inside of the regex check and just leave the one above the regex check...?

Ekultek commented 7 years ago

As I said, you've pushed enough to this project for me to trust what you think is best. You can make the choice on this one man, completely up to you. I would prefer it to show all hashes, however, that would slow things down a bit and I don't want that. So whatever you think is best in game for

On Jul 2, 2017, at 6:11 PM, delirious-lettuce notifications@github.com wrote:

@Ekultek ,

I was asking because I don't know which way you want it to be. If you want the log to show every different hash that's being tested, it seems like having the message above the regex check if preferable. But if you want only the matches shown, having the message underneath the regex check if preferable.

Just guessing, but the message seems to imply you want them all shown, not just the matches. In other words, remove the log message from inside of the regex check and just leave the one above the regex check...?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

delirious-lettuce commented 7 years ago

@Ekultek ,

I chose to leave all the hashes being logged (as you preferred). It seems like if this is found to be a performance bottleneck, it could always be moved underneath to only show the matches.

As they say, premature optimization is the root of all evil!

Ekultek commented 7 years ago

I'd like to talk to you about granting you access to the repo do you have slack?

delirious-lettuce commented 7 years ago

No slack but I guess I could make one. I use Gitter quite a bit to talk to people from github though.

Ekultek commented 7 years ago

Get slack and lemme know your username I'll send you a request

On Jul 2, 2017, at 6:24 PM, delirious-lettuce notifications@github.com wrote:

No, slack but I guess I could make one. I use Gitter quite a bit to talk to people from github though.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

delirious-lettuce commented 7 years ago

Ok, I'll go try and get signed up.

Ekultek commented 7 years ago

Sounds good bud, lemme know when you're done and I'll send the info to you

On Jul 2, 2017, at 6:25 PM, delirious-lettuce notifications@github.com wrote:

Ok, I'll go try and get signed up.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

Ekultek commented 7 years ago

Need the email you signed up with

On Jul 2, 2017, at 6:31 PM, delirious-lettuce notifications@github.com wrote:

delirious.lettuce

Ekultek commented 7 years ago

Sent the invite

On Jul 2, 2017, at 6:38 PM, delirious-lettuce notifications@github.com wrote:

Oh sorry.

delirious.lettuce@gmail.com

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.