Discord.py Bot : Sends with the loverate between 2 persons I'm creating a Discord Bot and i made a !love command which is testing the love rate between 2 persons or entities.
Example : !love Peignoir Emma
Result: The loverate between Peignoir and Emma is x%
Here's the code :
@bot.command(name="love", aliases=["l"])
async def love(ctx, Personne1, Personne2):
    if ctx.message.channel.id != 763352957579690018:
        printIt = 1
        wordBanList = ['@everyone', '@here', '<@&763489250162507809>','<@&777564025432965121>','<@&822200827347075132>',
                       '<@&763815680306184242>','<@&764422266560839680<','<@&763815728972300338>','<@&763815728972300338>',
                       '<@&763815228323528725>','<@&763815784904261632>','<@&764422166116171806>','<@&764422057353936897>',
                       '<@&804807279043674143>','<@&828664814678179861>','<@&823562218095640646>','<@&823638574809219163>']
        LoveRate = str(random.randrange(0, 100))
        for y in range(len(wordBanList)):
            if(Personne1 == wordBanList[y] or Personne2 == wordBanList[y]):
                printIt = 0

        if(printIt == 0):
            await ctx.send("Tu t'es pris pour qui ?")
            if debug == True:
                print("[DEBUG] !love : Someone tried to use a banned word !")
        else:
            await ctx.send("L'amour entre **"+Personne1+"** et **"+Personne2+"** est de **"+LoveRate+"%** <:flushed:830502924479758356>")
            if debug == True:
                print("[DEBUG] !love : The love rate ("+LoveRate+"%) between "+Personne1+" and "+Personne2+" has been printed in channel ID "+str(ctx.channel.id))
    else:
        botChannel = discord.utils.get(ctx.guild.channels, id=768194273970880533)
        messageBot = await ctx.send("Va faire cette commande dans "+botChannel.mention+" ou je te soulève <:rage:831149184895811614>")
        await asyncio.sleep(5)
        await ctx.message.delete()
        await messageBot.delete()

I'm pretty sure this code is optimizable, but i don't know how, can somebody help me :) ?

Never iterate over indices in python
In Python this pattern is generally not recommended:
for y in range(len(wordBanList)):
    if(Personne1 == wordBanList[y] or Personne2 == wordBanList[y]):
        printIt = 0

You almost never need to iterate over and access elements by their indices. Instead you should directly iterate over the elements of the iterable:
for word in wordBanList:
    if Personne1 == word or Personne2 == word:
        printIt = 0

Also notice that you do not need the parentheses around the or statement.
If you do need the index as well you can use built-in enumerate:
for index, word in enumerate(wordBanList):
    ...

Membership tests
As Harsha pointed out, sets are optimized for membership tests, so use them when possible. Doing so can also make your assignment of printIt way more concise:
wordBanList = {'@everyone', '@here', '<@&763489250162507809>', ...}
printIt = 0 if (Personne1 in wordBanList or Personne2 in wordBanList) else 1

Parantheses are also not required here, but might improve readability.

Naming convention
PEP 8:

Function names should be lowercase, with words separated by
underscores as necessary to improve readability.
Variable names follow the same convention as function names.

Variable and function names should follow snake_case.

debug == True
As a general rule of thumb, you should always use is with the built-in constants: True, False and None. It might also suffice to check debug for truthiness (if debug:). This however depends on the use case, since the expressions are not equivalent. The second one will fail for all falsey values, not just False (e.g. an empty string, an empty list). If you're interested, you can read more about Truthy and Falsy Values in Python.

String formatting
LoveRate = str(random.randrange(0, 100))

LoveRate (or better love_rate) should not be of type str, since you only need its str-represantation to print it. You can let Python handle formatting for you, by using f-strings. That way you never have to think about casting to str.
print("[DEBUG] !love : The love rate ("+LoveRate+"%) between "+Personne1+" and "+Personne2+" has been printed in channel ID "+str(ctx.channel.id))

becomes
print(f"[DEBUG] !love : The love rate ({LoveRate}%) between {Personne1} and {Personne2} has been printed in channel ID {ctx.channel.id}")

A few other small points:

Instead of wrapping the main code inside an if-else-block we can use ctx.message.channel.id == 763352957579690018 as an exit condition.
print_it is of type int, but basically acts as a variable of type bool. We can also eliminate that variable entirely, since we only use it once.
Line length should ideally not exceed 80 characters. You can use parantheses to conveniently concatenate strings across multiple lines, as described in this reddit tip. My IDE is set to hard wrap only at 100 characters, so that's the maximum length for the lines below.

@bot.command(name="love", aliases=["l"])
async def love(ctx, personne1, personne2):
    if ctx.message.channel.id == 763352957579690018:
        bot_channel = discord.utils.get(ctx.guild.channels, id=768194273970880533)
        message_bot = await ctx.send(f"Va faire cette commande dans {bot_channel.mention} "
                                     f"ou je te soulève <:rage:831149184895811614>")
        await asyncio.sleep(5)
        await ctx.message.delete()
        await message_bot.delete()
        return

    word_ban_list = {'@everyone', '@here', '<@&763489250162507809>', '<@&777564025432965121>',
                     '<@&822200827347075132>', '<@&763815680306184242>', '<@&764422266560839680<',
                     '<@&763815728972300338>', '<@&763815728972300338>', '<@&763815228323528725>',
                     '<@&763815784904261632>', '<@&764422166116171806>', '<@&764422057353936897>',
                     '<@&804807279043674143>', '<@&828664814678179861>', '<@&823562218095640646>',
                     '<@&823638574809219163>'}

    love_rate = str(random.randrange(0, 100))

    if personne1 in word_ban_list or personne2 in word_ban_list:
        await ctx.send("Tu t'es pris pour qui ?")
        if debug:
            print("[DEBUG] !love : Someone tried to use a banned word !")
    else:
        await ctx.send(f"L'amour entre **{personne1}** et **{personne2}** est de **{love_rate}%** "
                       f"<:flushed:830502924479758356>")
        if debug:
            print(f"[DEBUG] !love : The love rate ({love_rate}%) between {personne1} and"
                  f"{personne2} has been printed in channel ID {ctx.channel.id}")


Finding a passcode by comparing MD5 result This function finds the 8 char passcode from a given "doorId".

Is there a way to find the passcode in an undetermined number of steps without using a while? Maybe involving streams? val passcode = stream(logic).take(8).

def findPassCode(doorId: String): String = {
  var passcode = ""
  var count = 0

  while (passcode.length < 8) {
    val hash = md5(doorId + count)
    if (hash.startsWith("00000")) passcode = passcode + hash.drop(5).take(1)
    count = count + 1
  }

  passcode
}

First of all, you have the passcode length hard-coded.  It would be better to make it a parameter, perhaps with a default value.  The same goes for the string the "interesting" hashes start with ("00000").

Also, there are several ways you could make the code more functional.  Here are a couple ideas:

Use recursion:  Computing an \$n\$ character pascode is accomplished by first computing an \$n-1\$ character passcode, then computing the next character.  Or, you could rewrite the loop in a tail-recursive form.
You could use streams.  Streams are Scala's version of a lazy list / sequence.

Using streams, we can use the following logic:

Generate a stream of consecutive Ints, starting at 0.
Combine each number in with doorId
Compute the hash of each combined string
Filter the hashes, keeping only hashes that start with five '0's
Take the sixth character of each hash
Take the first 8 - doorId.length characters, and append them to doorId

We can express this in code using something like:

def findPassCode(doorId: String): String = {
  val chars = Stream.from(0)                            // Step 1
                    .map(n => doorId + n)               // Step 2
                    .map(md5)                           // Step 3
                    .filter(h => h.startsWith("00000")) // Step 4
                    .map(h => h(5))                     // Step 5
  doorId + chars.take(8 - doorId.length).mkString       // Step 6
}


Two while for the same query If I have to loop results of a query echoing first all fields of a column, then echoing something not to loop, then fields of another column.

<?php
$con = mysqli_connect();
echo 'Latest users who tried our test
';
//select the fields of the first column
$query1 = mysqli_query($con,"SELECT user FROM ris_uinversita LIMIT 0,10");
//loop the results
while($assoc1=mysqli_fetch_assoc($query1)){echo $assoc1['user'].'
';}
//echo something not to loop
echo 'Users got the following scores
';
//select the fields of the second column
$query2 = mysqli_query($con,"SELECT score FROM ris_universita LIMIT 0,10");
//loop the results
while($assoc2=mysqli_fetch_assoc($query2)){echo $assoc2['score'].'
';}
?>

Is it possible to use just one query like this

mysqli_query($con,"SELECT * FROM ris_universita LIMIT 0,10");

to select both fields just once?

If it could be helpful, table ris_universita has just the two columns: "score" (tinyint) and "user" (varchar 30).  There aren't so many records for now, but I hope in there will be in the future.

A few things here:

Comments:

Your comments are all compeletely useless.. nothing personal and no offense. That is a beginner-mistake. You don't need to comment everything.

Comments should explain the why and not the how or the what. Exceptions:

License comments
Documentation (python's docstring, java's javadoc, c#'s summary, ...)

Naming:

$con = //...
$query1 = //...
$assoc1 = //...

As opposed to what many people seem to think, source code characters are not costly. Spell it out:

$connection = //...
$userQuery = //...
$userRecord = //...

Formatting:

The convention is, to place subordinate blocks in a visually different place than surrounding code. This is usually done by indenting the block and putting it on a separate starting line

while($assoc1=mysqli_fetch_assoc($query1)){echo $assoc1['user'].'
';}

then becomes:

while ($userRecord = mysqli_fetch_assoc($userQuery)) {
    echo $userRecord['user'].'
';
}

This would definitely improve the readability of your code, as one line is not a whole loop! Putting too much on one line makes it extremely difficult to follow the thoughts behind your code and makes reading a pain.


Validating form fields I am using if statements within my functions to validate form fields and then on the isset event.

I want to be able to create many different functions to validate various fields groups, and it seems that this statement could grow very long:

if (($namecheck === TRUE) && ($emailcheck ===TRUE) && ($messagecheck ===TRUE)) {

This also causes the alert for each function if no data is entered. I can circumvent this by making statements if one proves false, then stop, but it is only getting messier and messier.

I don't need my code written for me. As such, I am interested in what you suggest, in terms of if statements, using switch or loop instead, or any other ideas.

I am not so much wanting a solution here, as reasons for why some ways are better than others. For the purposes of this discussion, I am not concerned with client side validation. It is a focus on PHP functions only.

<?php

include ("contact.html");

function namecheck ($fname, $lname) {
    $regexp ="/^[A-Za-z]+$/";
//filter through names
 if (preg_match($regexp,$fname,$lname)) {
     return TRUE; }
    else {
        echo'<script type="text/javascript">alert("Enter your names.")</script>';
        return FALSE; }}

function emailcheck ($email1, $email2) {
$regexp="/^[a-zA-A-Z0-9_.]+@[a-zA-Z0-9-]+\.[a-zA-Z0-9.-]+$/";
//validate email address
if (preg_match($regexp,$email1,$email2)) {
    return TRUE; }
    else {
        echo '<script type="text/javascript">alert ("Enter a valid email address.")</script>';
        return FALSE; }}

function messagecheck ($message) {
$regex = "/^[a-zA-Z0-9.?!_-]+$/";
//validate message text
if (preg_match($regex,$message)) {
    return TRUE;}
    else{
        echo'<script type="text/javascript">alert("Enter your message.")</script>';
        return FALSE; }}

if (isset($_POST['submit'])) {

  $fname=$_POST['fname'];
  $lname=$_POST['lname'];
  $namecheck=namecheck($fname);
  $email1=$_POST['email1'];
  $email2=$_POST['email2'];
  $emailcheck=emailcheck($email1,$email2);
  $message = $_POST['message'];
  $messagecheck=messagecheck($message);

   if (($namecheck === TRUE) && ($emailcheck ===TRUE) && ($emailcheck ===TRUE)) {

    $subject="website contact";
    mail("myemail", $subject ,$message , "From: $email1");
     echo '<script type="text/javascript"> alert("Your form has been submitted.") </script>';  }  }

?>

If I understand your problem well, the problem is that you don't want an if clause to become too long and that you have a problem with empty parameters. Your way of checking seems right to me, for the other part I asked myself if you can't just add something like this to the functions namecheck() emailcheck() and messagecheck():

 if (strlen($fname) == 0 || strlen($lname) == 0) return false;

Furthermore in your code line with the checks you have two times the emailcheck instead of the messagecheck once:

  if (($namecheck === TRUE) && ($emailcheck ===TRUE) && ($emailcheck ===TRUE)) {

Good luck, hope it helped.

Jef


Is it better to create a class instance, or create an instance each time a method is called? I am creating a converter for use in a Windows 8 XAML app (MVVM).  As you can see in the code below, the converter is used to convert a bool into one of two strings from a language resource file.
My question is, is it better to create one instance of ResourceLoader for the converter class or should I continue creating new instances each time the method is called?

class BooleanToBudgetYearStringConverter : IValueConverter
{
    private const string CALENDAR = "BudgetCycle_Calendar",
                        FISCAL = "BudgetCycle_Fiscal";

    private Windows.ApplicationModel.Resources.ResourceLoader loader;

    public object Convert(object value, Type targetType, object parameter, string language)
    {
        string entryName = (value is bool && (bool)value) ? CALENDAR : FISCAL;  //The name of the entry desired from the entry file.
        return new Windows.ApplicationModel.Resources.ResourceLoader().GetString(entryName);
    }

    public object ConvertBack(object value, Type targetType, object parameter, string language)
    {
        return (value is string && (string)value == new Windows.ApplicationModel.Resources.ResourceLoader().GetString(CALENDAR));
    }
}

I am specifically concerned about speed and memory use.

Use an Attribute

The class should be tagged with:

[ValueConversion(typeof(Boolean), typeof(String))]

If it's of the normal, old-school .NET variety (desktop, client profile, WP8). Windows store apps don't appear to have an analogous attribute in their .NET libraries, and thus this recommendation can be ignored when compiling such a program.

Drop Type Checks

If the passed-in value is not of the expected type, you should not do your conversion; you should allow an exception to be thrown, so that you know an invalid binding has taken place (and you can go correct the offending code). This applies to both conversion and back-conversion.

Explicit Back Conversion

Check for an explicit match -- don't default to returning "false" on a non-match with the CALENDAR string. Instead, check that the input string matches with the FISCAL string. Throw an exception if the input string matches neither acceptable string.

Use a Singleton

The documentation for Windows.ApplicationModel.Resources.ResourceLoader implies that the default constructor you're using should result in an object that is identical in function to the one available from the static GetForCurrentView() function. If not, then set up your own singleton pattern to ensure that you have at most one shared copy of the resource object around at any given time.


What are the latest Odds? The main problem the following query is solving, is the following:

Each Offer has multiple Odds (Odds has foreignkey to Offer). For each offer I want the latest Odds (ORDER BY time).

I'm pretty sure this is too heavy, so any input would be great.

SELECT "odds_odds"."id", "odds_odds"."offer_id", "odds_odds"."time", "odds_odds"."o1", "odds_odds"."o2", "odds_odds"."o3", "odds_odds"."o4", MAX(T3."time") AS "max_time"
FROM "odds_odds"
LEFT OUTER JOIN "odds_offer" ON ( "odds_odds"."offer_id" = "odds_offer"."id" )
LEFT OUTER JOIN "odds_odds" T3 ON ( "odds_offer"."id" = T3."offer_id" )
GROUP BY "odds_odds"."id", "odds_odds"."offer_id", "odds_odds"."time", "odds_odds"."o1", "odds_odds"."o2", "odds_odds"."o3", "odds_odds"."o4"
HAVING "odds_odds"."time" =  MAX(T3."time")

This is constructed in Django using the following syntax:

Odds.objects.annotate(
    max_time=Max('offer__odds__time')
).filter(
    time=F('max_time')
)

This is my schema:

odds_offer
id (int), .....

odds_odds
id (int), time (datetime), offer_id(int), .....

Now what I want to retrieve The Odds row with the latest time for each Offer. There is a unique constraint on (time, offer_id). One Offer can have multiple odds (historic odds).

Formatting

I think your query could really use some white space to make it easier to read. I realize there is no official standard for SQL, but here is my preferred style:

SELECT
    "odds_odds"."id",
    "odds_odds"."offer_id",
    "odds_odds"."time",
    "odds_odds"."o1",
    "odds_odds"."o2",
    "odds_odds"."o3",
    "odds_odds"."o4",
    MAX(T3."time") AS "max_time"
FROM "odds_odds"
    LEFT OUTER JOIN "odds_offer"
        ON ( "odds_odds"."offer_id" = "odds_offer"."id" )
    LEFT OUTER JOIN "odds_odds" T3
        ON ( "odds_offer"."id" = T3."offer_id" )
GROUP BY
    "odds_odds"."id",
    "odds_odds"."offer_id",
    "odds_odds"."time",
    "odds_odds"."o1",
    "odds_odds"."o2",
    "odds_odds"."o3",
    "odds_odds"."o4"
HAVING "odds_odds"."time" =  MAX(T3."time");

Table aliases

A table named "odds_odds" seems odd to me, but you could save some typing by aliasing as odds at least. Also not sure why you are using quotation marks everywhere, but maybe it's a requirement for your system. I have a Postgres database and never needed those, so it just feels like noise.

Simplifying

You said:

  For each offer I want the latest Odds (ORDER BY time).

What I see your code doing is what seems like unnecessary computation to find MAX(), where you likely could entirely remove your aggregation and write something along those lines:

SELECT
    "odds_odds"."id",
    "odds_odds"."offer_id",
    "odds_odds"."time",
    "odds_odds"."o1",
    "odds_odds"."o2",
    "odds_odds"."o3",
    "odds_odds"."o4",
    T3."time" AS "max_time"
FROM "odds_odds"
    LEFT OUTER JOIN "odds_offer"
        ON ( "odds_odds"."offer_id" = "odds_offer"."id" )
    LEFT OUTER JOIN "odds_odds" T3
        ON ( "odds_offer"."id" = T3."offer_id" )
ORDER BY "odds_odds"."time" DESC
LIMIT 1; -- or however many you need


Test cases for next palindromic number I recently did a programming challenge: given a positive number find the next palindromic number. In PHP, I decided to do something like this:

<?php

function palindromic($input) {
  while (1) {
    $number = (string) ++$input;

    if ($number == strrev($number)) {
      return $number;
    }
  }
}

I was then asked to provide test inputs for the function and to explain why, so I had (where x is a random number)

x < 10: test single digits
10 < x < 100: tests numbers with even length
100 < x < 1000: tests numbers with odd lengths
1,000,000 < x: tests a largish number

Note: we can always assume a positive number, so checking for non-positive numbers or inputs that are a different type, or asserting exceptions thrown back, etc aren't relevant to the challenge.

Apparently there were some other important numbers to test, can anyone explain what I've missed here?

I don't code in java or python so I don't fully follow the methods provided in the commented link under the question.

I've decided to write a method that avoids the incremental checking for palindromic integers.  Instead, it uses arithmetic based on a few preliminary string function results to discover the closest greater palindromic integer.  Note, there are limitations to my method because the input and output are integers rather than strings.  I can conclude after testing that my method will stay true into the 10's of trillions.

Code: (Demo)

function nextPalindromicInteger($integer) {
    if ($integer < 9) {
        $next = $integer + 1;
        return "$integer => $next (+1) " . ($next != strrev($next) ? "FAIL" : "PASS") . "\n";
    }
    $size = strlen($integer);
    $psize = ceil($size / 2);  // find length of rhs & lhs including pivot when length is odd
    $npsize = $size - $psize;  // find length of rhs & lhs excluding pivot when length is odd
    $rnplhs = (int)strrev(substr($integer, 0, $npsize));  // reverse nplhs
    $nprhs = (int)substr($integer, -$npsize);  // store right hand side integer excluding pivot  //KEEP
    $trimmed=strlen(rtrim(substr($integer, 0, $psize), '9')); // store length of rtrim'd plhs
    if ($rnplhs > $nprhs) {
        $bump = 0;
    } elseif (!strlen(trim($integer, '9'))) {
        $bump = 2;
        $nprhs = 0;
        $rnplhs = 0;
    } else {
        if ($psize != $npsize) {  // odd integer length
            if ($trimmed == 1) {
                $exponent = 1;  // dictates $bump=11
            } else {
                $exponent = $npsize;
            }
        } else {  // even integer length
            $exponent = $trimmed;
        }
        $bump = pow(10, $exponent);
        if ($psize == $npsize || $trimmed < $psize) {
            $bump += pow(10, $exponent - 1);
        }
    }
    $next = $integer + $rnplhs - $nprhs + $bump;
    return "$integer => $next (+" . ($next - $integer) . ") " . ($next != strrev($next) ? "FAIL" : "PASS") . "\n";
}
for ($x = 0; $x < 14; ++$x) {
    $integer = rand(pow(10, $x), pow(10, $x + 1) - 1);
    echo nextPalindromicInteger($integer);
}

If I spent some more time refining, I could probably write my function more eloquently, but for the purposes of this question it is sufficient to reveal (to me at least) some of the fringe cases.

I found in my development and testing that it was important to test against:

a range of integers, some with even length and some with odd length.
integers with a pivot (shared middle digit) of nine.
integers with an odd length and a non-9 pivot.
integers consisting entirely of 9's.
integers where the reversed left-hand-side component minus the right-hand-side component is:  >0, =0, and <0

Once I isolated the patterns that exist with certain sequences, I could declare the correct "bump" or adjustment.  Some bumps are static, others depend on the length or value of the left-hand-side component.

As for your relevant numbers to check using your method, I can't think of any that would cause trouble because you are looping and checking each iteration.  Perhaps the challenge was assuming that you would try to write a method without loops as I did or run "inside out" string comparison checks on each side's digits.

As for checking "large-ish" numbers, I found that once I had perfected the bumps from 1 to 9999, all integers above that were accounted for.

Possible Output:

9 => 11 (+2) PASS
75 => 77 (+2) PASS
776 => 777 (+1) PASS
4891 => 4994 (+103) PASS
92764 => 92829 (+65) PASS
999539 => 999999 (+460) PASS
7059657 => 7060607 (+950) PASS
48756572 => 48766784 (+10212) PASS
633149324 => 633151336 (+2012) PASS
2096785014 => 2096886902 (+101888) PASS
80448359477 => 80448384408 (+24931) PASS
237762850363 => 237763367732 (+517369) PASS
7730351516591 => 7730351530377 (+13786) PASS
79541453924626 => 79541455414597 (+1489971) PASS


No getter/setter on model with many instance vars While reading Object calisthenics and Getters/Setters. Evil. Period. I got confused. Let me explain it by example.

I have this organization class, representing the information about organization, the classic entity:

class Organization
{
    private $name;

    private $city;

    private $street;

    private $phone;

    private $email;

    public function __construct($name, $city, $street, $phone, $email)
    {
        $this->name = $name;
        $this->city = $city;
        $this->street = $street;
        $this->phone = $phone;
        $this->email = $email;
    }

    public function getName()
    {
        ....
    }

    public function getCity()
    {
        ....
    }

    public function setCity($newCity)
    {
        ....
    }

    public function getStreet()
    {
        ....
    }

    public function setStreet($newStreet)
    {
        ....
    }

    public function getPhone()
    {
        ....
    }

    public function getEmail()
    {
        ....
    }

    public function setPhone($newPhoneNumber)
    {
        ....
    }
}

The organization name, city, street, phone and email may change, so the setters looks kind a logical. Getters are used to to display the properties of organization in table. How can I remove the boilerplate code and break trough from this anemic model? Maybe this is the case where the getter/setter is unavoidable?

Also object initialization using __construct. Is it a good practice to pass so many instance variables to constructor?

I gave it a try, according to what I read. Created some ValueObject to store information and do validation (don't know if the ValueObject should do a validation on its data):

class Phone
{
    private $phone;

    public function __construct($phone)
    {
        if (!$this->validate($phone)) {
            throw new InvalidPhoneNumberException();
        }

        $this->phone = $phone;
    }

    public function getPhone()
    {
        return $this->phone;
    }

    private function validate($phone)
    {
        // validate the phone number
    }
}

class Email
{
    private $email;

    public function __construct($email)
    {
        if (!$this->validate($email)) {
            throw new InvalidEmailAddressException();
        }

        $this->email = $email;
    }

    public function getEmail()
    {
        return $this->email;
    }

    private function validate($email)
    {
        // validate email
    }
}

class Address
{
    private $city;

    private $street;

    public function __construct($city, $street)
    {
        $this->city= $city;
        $this->street = $street;
    }

    public function getCity()
    {
        return $this->city;
    }

    public function getStreet()
    {
        return $this->street;
    }
}

Then passed these objects instead of string representations of data to my Organization and did some renaming on methods:

class Organization
{
    private $name;

    private $address;

    private $phone;

    private $email;

    public function __construct($name, Address $address, Phone $phone, Email $email)
    {
        $this->name = $name;
        $this->address = $address;
        $this->phone = $phone;
        $this->email = $email;
    }

    public function getName()
    {
        return $this->name;
    }

    public function getAddress()
    {
        return $this->address;
    }

    public function getPhone()
    {
        return $this->phone;
    }

    public function getEmail()
    {
        return $this->email;
    }

    public function changeAddress(Address $newAddress)
    {
        $this->address = $newAddress;
    }

    public function changePhoneNumber(Phone $newPhoneNumber)
    {
        $this->phone = $newPhoneNumber;
    }
}

Does this code smells less and looks more mature from the perspective of OOP?

Getters and setters are not evil, period.

Mindlessly setting and getting fields of an object is bad, though. Especially with this syntax (setX, getX).

Simple example showing the difference between mindless and thoughtful setter :

# Mindless
class CartesianPoint {
    private $x;
    private $y;

    public function setX($x) {
        $this->x = $x;
    }

    public function setY($y) {
        $this->y = $y;
    }
}

# Thoughtful
class CartesianPoint {
    private $x;
    private $y;

    public function plot($x, $y) {
        $this->x = $x;
        $this->y = $y;
    }
}

Can you spot the differences ? You don't want to be able to only set X or Y location of a cartesian point, because that is not how it works.

Note that object is written in bold in the second sentence. Your Organization class is merely a data structure here. There's no sense attached to it, it doesn't follow rules and doesn't use its own logic.

In conclusion, if something has some logic/sense about it that the external world knows (should know) nothing about, make an object, but so far, Organization should only have public fields, and absolutely no methods inside. That's not an object.


Ignore nil values and unwrap optionals returning the value as the wrapped type My primary concern is that this has been written as a global function.  I think it would be better if this was a function of the SignalProducer class but I am not sure that is even possible.  Is this thinking justified or is the use of global functions acceptable?

(The following code uses ReactiveCocoa 4 and is written in Swift.)

func ignoreNilValues <Value,Error> (producer: SignalProducer<Value?,Error>) -> SignalProducer<Value, Error> {
    return producer.filter { return $0 != nil }.map { $0! }
}

There is already a function in reactiveSwift that you can use:

 .skipNil()


Number Guessing Game in Haskell I've been learning a lot about functional programming lately and I've recently started learning Haskell to which I'm very new.

I've written a simple number guessing game and I would like to have feedback on what is good or bad about my code and how it could be improved.

#!/usr/bin/env runhaskell

import Text.Printf (printf)
import System.Random (randomRIO)

smallest = 1 :: Integer
biggest = 100 :: Integer

main :: IO ()
main = do
  secret <- randomRIO (smallest, biggest) :: IO Integer
  printf "Find the number between %d and %d.\n" smallest biggest
  loop secret 1 where
    loop :: Integer -> Integer -> IO ()
    loop secret tries = do
      guess <- readLn :: IO Integer
      case compare guess secret of
        LT -> do
          putStrLn "Too small!"
          loop secret $ succ tries
        GT -> do
          putStrLn "Too big!"
          loop secret $ succ tries
        EQ -> do
          printf "You found the number in %d tries!\n" tries

Changing the where to a let allows you to skip passing the secret. Passing guesses in and messages out purifies the control flow part of the program. zip [1..] makes clear: The number of tries during each step does not depend on the control flow. interact, lines and foldr replace the recursion. (Recursion is too powerful for brains to handle in general, so we should avoid it when we have a simpler tool.) Your mileage may vary on any of these suggestions.

secret <- randomRIO (smallest, biggest) :: IO Integer
let foo :: (Integer, Integer) -> String -> String
    foo (tries, guess) = case compare guess secret of
      LT -> (++) "Too small!\n"
      GT -> (++) "Too big!\n"
      EQ -> const $ printf "You found the number in %d tries!\n" tries
printf "Find the number between %d and %d.\n" smallest biggest
interact $ foldr foo undefined . zip [1..] . map read . lines


T-SQL Pbdfk2 (Rfc2898 SHA2_256) Implementation Using my previous function as a base I've come up the following:

create function dbo.Pbkdf2 (
    @password varbinary(8000)
  , @salt varbinary(7996)
  , @iterations int = 1000
  , @derivedKeyLength int = 32
)
returns varbinary(max)
as
begin;
    declare @hmacLength int = 32;
    declare @i int = 1;
    declare @l int = (@derivedKeyLength + @hmacLength - 1) / @hmacLength;
    declare @r int = @derivedKeyLength - (@l - 1) * @hmacLength;
    declare @derivedKey varbinary(max) = Cast('' as varbinary(max));
    declare @u varbinary(8000);
    declare @uA binary(32);
    declare @uB binary(32);

    while(@i <= @l)
    begin;
        declare @j int = 1;

        select @u = @salt + Cast(@i as binary(4));
        select @uA = uA.[Hash]
             , @uB = uA.[Hash]
        from dbo.Hmac(@password, @u) as uA;

        while @j < @iterations
        begin;
            select @uA = tA.[Hash] from dbo.Hmac(@password, @uA) as tA;
            select @uB = tB.[Hash]
            from (values(
              -- unrolled loop to XOR uA and uB
                Cast(Substring(@uA, 1, 8) ^ Cast(Substring(@uB, 1, 8)  as bigint) as binary(8))
              + Cast(Substring(@uA, 9, 8) ^ Cast(Substring(@uB, 9, 8)  as bigint) as binary(8))
              + Cast(Substring(@uA, 17, 8) ^ Cast(Substring(@uB, 17, 8)  as bigint) as binary(8))
              + Cast(Substring(@uA, 25, 8) ^ Cast(Substring(@uB, 25, 8)  as bigint) as binary(8))
            )) tB ([Hash]);

            select @j = @j + 1;
        end;

        select @derivedKey = @derivedKey + Cast(case when @i = @l then Left(@uB, @r) else @uB end as varbinary(32));

        select @i = @i + 1;
    end;

    return @derivedKey;
end;

I'd really like to replace the loops with something set-based so that I can turn this into an inline-TVF but just can't seem to wrap my head around it...

I took a very, very wild swing at this. I also couldn't get it down to something entirely set-based, but I was able to get it to an inline TVF with a recursive CTE (two of them, unfortunately). With 1000 iterations, I needed a MAXRECURSION of at least 998 to get it to finish.

I wouldn't trust this as a cryptographically secure implementation. I strongly suspect that the optimizer could do something funny here that would enable timing attacks; likely some very aggressive query hinting would be needed to ensure a stable join order and methodology. With some digging, I think we would have to do the following (at a minimum) to make it cryptographically secure. All of these are to prevent timing attacks:

Force LOOP joins; HASH joins use hash tables, which have known timing attack vulnerabilities, and MERGE joins will sort, which will be impacted by the available data.
Force a join order (OPTION( FORCE ORDER )); if the optimizer knows something about the cardinality of the data set and re-orders as a result we'll also see different numbers.
Disable parallelism (OPTION( MAXDOP 1 ))

Throughout all of this I'm heavily relying on the behavior of the APPLY operator where it evaluates the right-hand side for each row of the left-hand side. This was vital to get some of these complicated calculations, and I suspect is the path towards a non-recursive implementation (if there is one).

The first thing I had to do was re-create your outer loop calculations, which I did with something like this. You might need more in the counter in the middle (just keep CROSS APPLYing to sys.all_objects), but this was fine for my test cases.

SELECT uAOuter.Hash UA,
       uAOuter.Hash UB,
       OuterLooper.Counter
  FROM ( SELECT TOP (( @derivedKeyLength + @hmacLength - 1 ) / @hmacLength )
                ROW_NUMBER() OVER ( ORDER BY ( SELECT NULL )) [Counter],
                @salt + CAST(ROW_NUMBER() OVER ( ORDER BY ( SELECT NULL )) AS binary(4)) U
           FROM sys.all_objects
           ORDER BY [Counter] ASC ) OuterLooper
    CROSS APPLY dbo.Hmac( @password, OuterLooper.U ) uAOuter

After that I got really stuck on how to calculate @uA in terms of itself; I still think that there is probably some really, really clever APPLY wizardry to get this, but I spent a few hours going nowhere on that one. Instead, I ultimately settled on a recursive CTE to get the base case and then do the calculation.

WITH InnerLoopRecursive AS
(
  SELECT tA.Hash uA,
         tB.Hash uB,
         OuterLoop.Counter OuterCount,
         1 [Counter]
    FROM ( SELECT uAOuter.Hash UA,
                  uAOuter.Hash UB,
                  OuterLooper.Counter
             FROM ( SELECT TOP (( @derivedKeyLength + @hmacLength - 1 ) / @hmacLength )
                           ROW_NUMBER() OVER ( ORDER BY ( SELECT NULL )) [Counter],
                           @salt + CAST(ROW_NUMBER() OVER ( ORDER BY ( SELECT NULL )) AS binary(4)) U
                      FROM sys.all_objects
                      ORDER BY [Counter] ASC ) OuterLooper
               CROSS APPLY dbo.Hmac( @password, OuterLooper.U ) uAOuter ) OuterLoop
      CROSS APPLY dbo.Hmac( @password, OuterLoop.UA ) tA
      CROSS APPLY ( VALUES (
                      -- unrolled loop to XOR uA and uB
                      CAST(SUBSTRING( tA.Hash, 1, 8 ) ^ CAST(SUBSTRING( OuterLoop.UB, 1, 8 ) AS bigint) AS binary(8)) + CAST(SUBSTRING( tA.Hash, 9, 8 ) ^ CAST(SUBSTRING( OuterLoop.UB, 9, 8 ) AS bigint) AS binary(8)) + CAST(SUBSTRING( tA.Hash, 17, 8 ) ^ CAST(SUBSTRING( OuterLoop.UB, 17, 8 ) AS bigint) AS binary(8)) + CAST(SUBSTRING( tA.Hash, 25, 8 ) ^ CAST(SUBSTRING( OuterLoop.UB, 25, 8 ) AS bigint) AS binary(8)))) tB ( [Hash] )
  UNION ALL
  SELECT tA.Hash uA,
         tB.Hash uB,
         InnerLoopRecursive.OuterCount,
         InnerLoopRecursive.Counter + 1
    FROM InnerLoopRecursive
      CROSS APPLY dbo.Hmac( @password, InnerLoopRecursive.uA ) tA
      CROSS APPLY ( VALUES (
                      -- unrolled loop to XOR uA and uB
                      CAST(SUBSTRING( tA.Hash, 1, 8 ) ^ CAST(SUBSTRING( InnerLoopRecursive.uB, 1, 8 ) AS bigint) AS binary(8)) + CAST(SUBSTRING( tA.Hash, 9, 8 ) ^ CAST(SUBSTRING( InnerLoopRecursive.uB, 9, 8 ) AS bigint) AS binary(8)) + CAST(SUBSTRING( tA.Hash, 17, 8 ) ^ CAST(SUBSTRING( InnerLoopRecursive.uB, 17, 8 ) AS bigint) AS binary(8)) + CAST(SUBSTRING( tA.Hash, 25, 8 ) ^ CAST(SUBSTRING( InnerLoopRecursive.uB, 25, 8 ) AS bigint) AS binary(8)))) tB ( [Hash] )
    WHERE InnerLoopRecursive.Counter + 1 < @iterations
)

After that I was really hoping to get away with just an aggregate (or maybe a window function), but apparently you can't do that with varbinary(MAX), so I had to do another recursive CTE.

, DerivedKeyPerOuterLoop AS
(
  SELECT CONVERT( varbinary(MAX), '' ) DerivedKey,
         0 [Counter]
  UNION ALL
  SELECT DerivedKeyPerOuterLoop.DerivedKey + CONVERT( varbinary(MAX),
                                                      CASE WHEN DerivedKeyPerOuterLoop.Counter = (( @derivedKeyLength + @hmacLength - 1 ) / @hmacLength ) THEN LEFT(InnerLoopRecursive.uB, @derivedKeyLength - ((( @derivedKeyLength + @hmacLength - 1 ) / @hmacLength ) - 1 ) * @hmacLength)
                                                           ELSE InnerLoopRecursive.uB END ) DerivedKey,
         DerivedKeyPerOuterLoop.Counter + 1
    FROM DerivedKeyPerOuterLoop
      INNER JOIN InnerLoopRecursive
        ON ( DerivedKeyPerOuterLoop.Counter + 1 ) = InnerLoopRecursive.OuterCount
    WHERE InnerLoopRecursive.Counter = @iterations - 1
)

Throughout the recursive CTEs the JOINs get pretty funky as we're trying to handle a potential granularity explosion, but it should work okay-ish.

Once we have that final CTE, then we just need to get the result from the last iteration:

CREATE OR ALTER FUNCTION dbo.Pbkdf2_modified
(
  @password         varbinary(8000),
  @salt             varbinary(7996),
  @iterations       int = 1000,
  @derivedKeyLength int = 32,
  @hmacLength       int = 32
)
RETURNS table
AS
  RETURN ( WITH InnerLoopRecursive AS
           (
             SELECT tA.Hash uA,
                    tB.Hash uB,
                    OuterLoop.Counter OuterCount,
                    1 [Counter]
               FROM ( SELECT uAOuter.Hash UA,
                             uAOuter.Hash UB,
                             OuterLooper.Counter
                        FROM ( SELECT TOP (( @derivedKeyLength + @hmacLength - 1 ) / @hmacLength )
                                      ROW_NUMBER() OVER ( ORDER BY ( SELECT NULL )) [Counter],
                                      @salt + CAST(ROW_NUMBER() OVER ( ORDER BY ( SELECT NULL )) AS binary(4)) U
                                 FROM sys.all_objects
                                 ORDER BY [Counter] ASC ) OuterLooper
                          CROSS APPLY dbo.Hmac( @password, OuterLooper.U ) uAOuter ) OuterLoop
                 CROSS APPLY dbo.Hmac( @password, OuterLoop.UA ) tA
                 CROSS APPLY ( VALUES (
                                 -- unrolled loop to XOR uA and uB
                                 CAST(SUBSTRING( tA.Hash, 1, 8 ) ^ CAST(SUBSTRING( OuterLoop.UB, 1, 8 ) AS bigint) AS binary(8)) + CAST(SUBSTRING( tA.Hash, 9, 8 ) ^ CAST(SUBSTRING( OuterLoop.UB, 9, 8 ) AS bigint) AS binary(8)) + CAST(SUBSTRING( tA.Hash, 17, 8 ) ^ CAST(SUBSTRING( OuterLoop.UB, 17, 8 ) AS bigint) AS binary(8)) + CAST(SUBSTRING( tA.Hash, 25, 8 ) ^ CAST(SUBSTRING( OuterLoop.UB, 25, 8 ) AS bigint) AS binary(8)))) tB ( [Hash] )
             UNION ALL
             SELECT tA.Hash uA,
                    tB.Hash uB,
                    InnerLoopRecursive.OuterCount,
                    InnerLoopRecursive.Counter + 1
               FROM InnerLoopRecursive
                 CROSS APPLY dbo.Hmac( @password, InnerLoopRecursive.uA ) tA
                 CROSS APPLY ( VALUES (
                                 -- unrolled loop to XOR uA and uB
                                 CAST(SUBSTRING( tA.Hash, 1, 8 ) ^ CAST(SUBSTRING( InnerLoopRecursive.uB, 1, 8 ) AS bigint) AS binary(8)) + CAST(SUBSTRING( tA.Hash, 9, 8 ) ^ CAST(SUBSTRING( InnerLoopRecursive.uB, 9, 8 ) AS bigint) AS binary(8)) + CAST(SUBSTRING( tA.Hash, 17, 8 ) ^ CAST(SUBSTRING( InnerLoopRecursive.uB, 17, 8 ) AS bigint) AS binary(8)) + CAST(SUBSTRING( tA.Hash, 25, 8 ) ^ CAST(SUBSTRING( InnerLoopRecursive.uB, 25, 8 ) AS bigint) AS binary(8)))) tB ( [Hash] )
               WHERE InnerLoopRecursive.Counter + 1 < @iterations
           ),
                DerivedKeyPerOuterLoop AS
           (
             SELECT CONVERT( varbinary(MAX), '' ) DerivedKey,
                    0 [Counter]
             UNION ALL
             SELECT DerivedKeyPerOuterLoop.DerivedKey + CONVERT( varbinary(MAX),
                                                                 CASE WHEN DerivedKeyPerOuterLoop.Counter = (( @derivedKeyLength + @hmacLength - 1 ) / @hmacLength ) THEN LEFT(InnerLoopRecursive.uB, @derivedKeyLength - ((( @derivedKeyLength + @hmacLength - 1 ) / @hmacLength ) - 1 ) * @hmacLength)
                                                                      ELSE InnerLoopRecursive.uB END ) DerivedKey,
                    DerivedKeyPerOuterLoop.Counter + 1
               FROM DerivedKeyPerOuterLoop
                 INNER JOIN InnerLoopRecursive
                   ON ( DerivedKeyPerOuterLoop.Counter + 1 ) = InnerLoopRecursive.OuterCount
               WHERE InnerLoopRecursive.Counter = @iterations - 1
           )
           SELECT DerivedKeyPerOuterLoop.DerivedKey
             FROM DerivedKeyPerOuterLoop
             WHERE DerivedKeyPerOuterLoop.Counter = (( @derivedKeyLength + @hmacLength - 1 ) / @hmacLength ));


Count words in a C string I have a string that must be splitted into an array of strings so using strtok makes perfect sense but before that I want to allocate the array of strings and thus I count how many words in it.

static inline int countWords(char *str)
{
        int count = 0;
        char *buf = str;

        if(isgraph(*buf) > 0)//checking for the first word
                count++;
        while(*buf != '\0')
        {
                if(isblank(*buf) > 0){//reached a blank
                        while(*buf == ' ' || *buf == '\t')//keep ignoring them
                                buf++;
                        if(*buf == '\0')//return if string is over
                                return count;
                        count++;//no more blank and string didn't end=new word
                }
                buf++;
        }
        return count;
}

Am I doing this right? I've tested all weird cases that can come to my mind and it seems to work; but is there a faster way to do this? I am mainly concerned about the performance of the above code.

Code comments:

The parameter to the function should be const - "const correctness".
Don't use the variable name "buf" for a variable that is not a memory buffer of some sort.
You have 3 different ways of checking for spaces in your code, you need to check for them in a consistent manner. Either use !isgraph which is the same as checking for ' '. Or isblank, which is the same as checking for ' '  and '\t'. Or possibly isspace, which checks for any form of white space character. I'll use isblank in my suggestion, since I don't know the nature of the string input.
Your code is a bit hard to follow, with several nested loops and if statements.

When I spontaneously rewrite your code to improve readability, I end up with this:

#include <ctype.h>

static int countWords (const char* str)
{
  int count = 0;

  while (*str != '\0')
  {
    while (*str != '\0' && isblank(*str)) // remove all spaces between words
    {
      str++;
    }
    if(*str != '\0')
    {
      count++;
    }

    while (*str != '\0' && !isblank(*str)) // loop past the found word
    {
      str++;
    }
  }

  return count;
}

This code is very easy to understand. However, you asked for optimization and my code doesn't look all that effective; it is possibly slower than the original. Particularly, we have multiple checks for '\0' all over the place.

Note that any truly meaningful optimization requires that 1) we have actually encountered and measured a real performance problem, 2) we have noticed that the compiler is doing a poor job at optimizing that particular problematic code, and 3) we have fairly good knowledge of the target CPU and hardware. It a bad idea to manually optimize code if those 3 above conditions are not met.

Still, I'll attempt a manual optimization of this, since it was requested. It may or may not be more effective. Branch prediction may be a more serious concern than the number of comparisons executed, for example. For what it is worth, here you go:

#include <ctype.h>
#include <stdbool.h>

static inline int countWords (const char* str)
{
  int count = 0;
  bool look_for_data = true;

  while (*str != '\0')
  {
    if( (look_for_data ^ isblank(*str)) != 0) // is it a character of interest?
    {
      count += (int)look_for_data; // increase count if we are looking for data
      look_for_data = !look_for_data;
    }
    str++;
  }

  return count;
}

With this optimization we have managed to reduce the number of instructions and the number of branches both. But as you can tell, the code now turned quite obscure.

Explanation:
The fundament of this code is that one lap in the loop corresponds to one character in the input string. The bool variable keeps track of whether the code is currently looking for spaces or non-spaces.

We are only interested in the places in the string where we go from looking for spaces to looking for data, or vice versa. When that happens, we should start looking for data if we were looking for spaces, and the other way around. Also, in the case where we go from spaces to data, we should increase the word counter.

The boolean logic truth table is:

look_for_data  isblank  of interest  comment
false          false    false        we are at symbols and found another symbol
false          true     true         we are at symbols and found a space
true           true     false        we are at spaces and found another space
true           false    true         we are at spaces and found a symbol

We can see that this happens to be the truth table for logical XOR.


Simulated annealing in R I am new in R and I have to implement simulated annealing for schaffer function and I did it. However I am not sure about the correctness of the code.

simulated_annealing <- function(schaffer, step, s0, niter) {
s_n <- s0
iter_count <- 0
for (k in 1:niter) {
    T <- (1 - step)^k
    s_new <- rnorm(2, s_n, 1)
    dif <- func(s_new) - func(s_n)
    if (dif < 0) {
        s_n = s_new
    } else {
        random <- runif(1,0,1)
        if (random < exp(-dif/T)) {
            s_n <- s_new
        }
    }
    iter_count <- iter_count + 1

    print(sprintf("The number of iteration was: %f",iter_count))
    print(sprintf("Function at s_new: %f", func(s_new)))
    print(sprintf("Function at s_n: %f", func(s_n)))
    print(sprintf("Current point: %f", s_n))
    print(sprintf("Temperature: %f", T))
}
return(s_n)
}

schaffer <- function(xx)
{x1 <- xx[1]
x2 <- xx[2]
fact1 <- (sin(x1^2-x2^2))^2 - 0.5
fact2 <- (1 + 0.001*(x1^2+x2^2))^2
y <- 0.5 + fact1/fact2
return(y)
}

It is how the simulated annealing works:

simulated_annealing(schaffer,0.1,c(0,2),10)

Output:

[1] "The number of iteration was: 1.000000"
[1] "Function at s_new: 0.899459"
[1] "Function at s_n: 0.572171"
[1] "Current point: 0.000000" "Current point: 2.000000"
[1] "Temperature: 0.900000"
[1] "The number of iteration was: 2.000000"
[1] "Function at s_new: 0.047084"
[1] "Function at s_n: 0.047084"
[1] "Current point: 1.022082" "Current point: 2.095950"
[1] "Temperature: 0.810000"
[1] "The number of iteration was: 3.000000"
[1] "Function at s_new: 0.153559"
[1] "Function at s_n: 0.047084"
[1] "Current point: 1.022082" "Current point: 2.095950"
[1] "Temperature: 0.729000"
[1] "The number of iteration was: 4.000000"
[1] "Function at s_new: 0.359053"
[1] "Function at s_n: 0.047084"
[1] "Current point: 1.022082" "Current point: 2.095950"
[1] "Temperature: 0.656100"
[1] "The number of iteration was: 5.000000"
[1] "Function at s_new: 0.828496"
[1] "Function at s_n: 0.047084"
[1] "Current point: 1.022082" "Current point: 2.095950"
[1] "Temperature: 0.590490"
[1] "The number of iteration was: 6.000000"
[1] "Function at s_new: 0.075788"
[1] "Function at s_n: 0.075788"
[1] "Current point: 0.344227" "Current point: 0.629732"
[1] "Temperature: 0.531441"
[1] "The number of iteration was: 7.000000"
[1] "Function at s_new: 0.713948"
[1] "Function at s_n: 0.075788"
[1] "Current point: 0.344227" "Current point: 0.629732"
[1] "Temperature: 0.478297"
[1] "The number of iteration was: 8.000000"
[1] "Function at s_new: 0.000717"
[1] "Function at s_n: 0.000717"
[1] "Current point: -0.106265" "Current point: -0.192747"
[1] "Temperature: 0.430467"
[1] "The number of iteration was: 9.000000"
[1] "Function at s_new: 0.844424"
[1] "Function at s_n: 0.000717"
[1] "Current point: -0.106265" "Current point: -0.192747"
[1] "Temperature: 0.387420"
[1] "The number of iteration was: 10.000000"
[1] "Function at s_new: 0.993481"
[1] "Function at s_n: 0.000717"
[1] "Current point: -0.106265" "Current point: -0.192747"
[1] "Temperature: 0.348678"
[1] -0.1062647 -0.1927468

I do not know if it works well and how to estimate the best point - every time it seems that it is the point of last iteration

Here is how I would rewrite your code, comments to follow.

simulated_annealing <- function(func, s0, niter = 10, step = 0.1) {

   # Initialize
   ## s stands for state
   ## f stands for function value
   ## b stands for best
   ## c stands for current
   ## n stands for neighbor
   s_b <- s_c <- s_n <- s0
   f_b <- f_c <- f_n <- func(s_n)
   message("It\tBest\tCurrent\tNeigh\tTemp")
   message(sprintf("%i\t%.4f\t%.4f\t%.4f\t%.4f", 0L, f_b, f_c, f_n, 1))

   for (k in 1:niter) {
      Temp <- (1 - step)^k
      # consider a random neighbor
      s_n <- rnorm(2, s_c, 1)
      f_n <- func(s_n)
      # update current state
      if (f_n < f_c || runif(1, 0, 1) < exp(-(f_n - f_c) / Temp)) {
         s_c <- s_n
         f_c <- f_n
      }
      # update best state
      if (f_n < f_b) {
         s_b <- s_n
         f_b <- f_n
      }
      message(sprintf("%i\t%.4f\t%.4f\t%.4f\t%.4f", k, f_b, f_c, f_n, Temp))
   }
   return(list(iterations = niter, best_value = f_b, best_state = s_b))
}

sol <- simulated_annealing(schaffer, s0 = c(0, 2))
#  It   Best    Current Neigh   Temp
#  0    0.5722  0.5722  0.5722  1.0000
#  1    0.1054  0.1054  0.1054  0.9000
#  2    0.1054  0.1922  0.1922  0.8100
#  3    0.1054  0.1922  0.3579  0.7290
#  4    0.1054  0.1922  0.9059  0.6561
#  5    0.1054  0.6428  0.6428  0.5905
#  6    0.1054  0.8893  0.8893  0.5314
#  7    0.1054  0.1597  0.1597  0.4783
#  8    0.1054  0.6489  0.6489  0.4305
#  9    0.1054  0.7535  0.7535  0.3874
#  10   0.1054  0.2236  0.2236  0.3487

sol
# $iterations
# [1] 10
#
# $best_value
# [1] 0.1053682
#
# $best_state
# [1] -0.3203791  1.7080835

Now the comments, in no particular order:

when defining your function, you meant to name the first argument  func, not schaffer. That was a bug.
niter and steps are the kind of arguments that could use an optional value so I made it so. And once an argument has an optional value, it is recommended to move it to the end of the argument list, hence function(func, s0, niter = 50, step = 0.1).
I renamed T to Temp for 1) it is more descriptive and more likely to suggest that this is the annealing's temperature variable, but mostly 2) R provides T as a shortcut for TRUE, although you can overwrite it like you just did here. This is a horrible feature of the R language and you should avoid using T both for TRUE or as your own variable.
message is probably a more appropriate tool for logging. I also transformed the code so it prints one row per iteration. I hope you'll agree the log is much easier to look at this way.
Keeping track of the best state is an improvement over the "vanilla" version simulated annealing process which only reports the current state at the last iteration. If you want it that way, then you need to use three states: best, current, neighbor. Which I implemented here.
In addition to keeping track of the current state and best state, it is recommended to keep track of their function values, so you do not have to make unnecessary function calls to re-compute them at each iteration.
Like the matlab function you referred to, it is preferable to return a list so you can provide more outputs than just the best state.
Always, it is important to comment your code to make it easier to understand and maintain.

I hope it helps.


Look up value from one dataframe and find percent of values in another dataframe that are less than that Is there a one-liner that can accomplish this same thing? Essentially I have two dataframes: one that has my raw data (data), and one that has values that I want to lookup (lookup). I want to find the percent of values in data (by 2 factors, Site and variable) that are less than the lookup value for those same 2 factors.

I'd love to just be able add the result to the lookup dataframe as a column, instead of creating two new objects.

combine <- merge(data, lookup, by = c("Site", "variable"), allow.cartesian = TRUE)
test <- ddply(combine, .(Site, variable), summarize, percent = sum(data.values < lookup.value) / length(data.values))

Any advice is appreciated!

If you want to encapsulate the ddply logic, you can use the lapply(split(data, data[ , c("Site", "variable") ], function)-approach. I'm assuming that lookup only has one "value" for each combination of "Site" and "variable". Something like:

lapply( split(data, data[ , c("Site", "variable") ]),
       function(d) { percent= 100*sum(d$value)/ lookup[
                                   lookup$Site==d$Site[1] lookup$variable==d$variable[1] ,
                                       "value"]}
       )

Another approach (only successful if the factor levels in both dataframes match) would be to use the quotient of two tapply-(matrix) results from each dataframes:

 percent= 100*tapply( data$value, data[ , c("Site", "variable")] , sum, na.rm=TRUE)/
 tapply( lookup$value, lookup[ , c("Site", "variable")] , sum, na.rm=TRUE)


Implementation of Longest Common Subsequence So I implemented the LCS algorithm once using a 2-D matrix to store values while the other one used a python dictionary. I found the dictionary implementation was easier to implement and was a more natural and intuitive way of solving the problem.

I just wanted to make sure if it's a correct way of implementing the LCS algorithm and how can I improve on it.

Using 2-D matrix

def LowestCommonSubstring(s1, s2):
    LCS = [[0 for x in range(len(s2) + 1)] for x in range(len(s1) + 1)]
    for i in range(1, len(s1)+1):
        for j in range(1, len(s2)+1):
            if s1[i - 1] == s2[j - 1]:
                LCS[i][j] = 1 + LCS[i-1][j-1]
            else:
                LCS[i][j] = max(LCS[i-1][j], LCS[i][j-1])
    return LCS[i][j]

using dictionary

cache = {}

def lcs(s1, s2):
  global cache
  if len(s1) == 0 or len(s2) == 0:
      return 0
  if (s1, s2) in cache:
      return cache[(s1, s2)]
  else:
      if s1[-1] == s2[-1]:
          cache[(s1, s2)] = 1 + lcs(s1[:-1], s2[:-1])
      else:
          cache[(s1, s2)] = max(lcs(s1[:-1], s2), lcs(s1, s2[:-1]))
  return cache[(s1, s2)]

This is the problem I'm trying to implement and for now my solution only calculates the length of the longest common substring.

You could use an explicit memoizer rather than incorporating caching into your function.
This will make your function easier to understand.
You can use functools lru_cache if you are using Python3.
You should evaluate based on truth, rather than length. len(s1) == 0 is 'bad'.
You can use a turnery operator to reduce the repetition of your code.
Your else can be removed, to prevent the arrow anti-pattern.
And reduce the amount of indentation.

def memoize(fn)
    def inner(*args):
        try:
            return cache[args]
        except IndexError:
            cache[args] = fn(*args)
            return cache[args]
    return inner

# Natural abstracted algorithm.
@memoize
def lcs(s1, s2):
    if not s1 or not s2:
        return 0

    return (
        1 + lcs(s1[:-1], s2[:-1])
        if s1[-1] == s2[-1] else
        max(lcs(s1[:-1], s2), lcs(s1, s2[:-1]))
    )


Angular wrapper for SignalR event aggregator I created a little wrapper for one of my JavaScript libraries to enable Angular functionality. Are there any pitfalls I should be wary of with my code?

angular.module("signalR.eventAggregator", [])
    .run([
        "$rootScope", function($rootScope) {
        function createScope(scope) {
            scope.$on('$destroy', function() {
                signalR.eventAggregator.unsubscribe(scope);
            });

            return {
                subscribe: function(type, handler, constraint) {
                    signalR.eventAggregator.subscribe(type, function(e) {
                        handler(e);
                        if (scope.$$phase == null) {
                            scope.$digest();
                        }
                    }, scope, constraint);
                },
                publish: function(event) {
                    signalR.eventAggregator.publish(event);
                }
            }
        }

        $rootScope.eventAggregator = function() {
            return this.__eventAggregator = this.__eventAggregator || createScope(this);
        };
    }
]);

More info on the code itself here.

It's used like this:

$scope.eventAggregator().subscribe(MyApp.MyEvent, onEvent);

Other scenarios it supports are

Generic events, (all events are server events proxied to javascript through a dynamic Owin javascript)

$scope.eventAggregator().subscribe(MyApp.MyGenericEvent.of("System.String"), onEvent);

Constraints, in this case listen to all events with id 5

$scope.eventAggregator().subscribe(MyApp.MyEvent, onEvent, { id: 5 });

It wraps an event-driven library, and when an event is triggered, I use digest to update the view, and it's this part of the code that I wonder about.

handler(e); could return a promise that I currently ignore. I tested the ng-click directive and it also ignores any promise returned from the click handler. So at least it's consistent with the existing event handlers in Angular.

Hmm... interesting use of run + $rootScope, but I would avoid it primarily because of $rootScope. It's essentially the "global space" of an Angular app. The only legit use I see for it is attaching event listeners ($rootScope.$on), and for broadcasting events ($rootScope.$emit).

As an alternative, you could wrap your event relay in a factory.

As a factory, the dependency is explicit. The dependent knows it's there and Angular will throw if the dependency is missing on initialization, unlike $rootScope where it only throws when you use it.
Factories are singletons. You don't have to worry about checking and using the existing or create a new one.
You're not polluting the $rootScope nor depending on something that might not be there or have been overridden by something in a lower, enclosing scope. This is a headache to debug, especially without tools like Batarang or ngInspector.

Additionally, you might also want to put your event names in a constant. That way, you have a global lookup of event names, and you won't be hardcoding string literals all over the app.

Here's an example of how it would look like as a factory (using implicit dependency injection syntax for brevity).

angular.module('SignalrModule', []);
  .constant('SIGNALR_EVENTS', {
    FOO_EVENT: 'fooevent',
    BAR_EVENT: 'barevent',
  })
  .factory('SignalrFactory', function(){
    return {
      publish: function(){...},
      subscribe: function(){...},
      unsubscribe: function(){...},
    };
  });

angular.module('app', ['SignalrModule'])
  .controller('MyController', function(SignalrFactory, SIGNALR_EVENTS){
    SignalrFactory.subscribe(SIGNALR_EVENTS.FOO_EVENT, function(){
      // on foo event
    });
  });

Now I did mention that angular has a built-in pub-sub system. We can just hook on to it so your controllers will simply use regular events over $rootScope (we're not adding stuff to $rootScope, just making it a relay for events). Not sure of the following works, but the concept is there.

angular.module('SignalrModule', []);
  .constant('SIGNALR_EVENTS', {
    FOO_EVENT: 'fooevent',
    BAR_EVENT: 'barevent',
  })
  .run(function($rootScope, SIGNALR_EVENTS){

    // Iterate through our registry of events
    Object.keys(SIGNALR_EVENTS).forEach(function(eventName){

      // Relay angular events to signalr over $rootScope
      $rootScope.$on(SIGNALR_EVENTS[eventName], function(){
        signalR.eventAggregator.publish(SIGNALR_EVENTS[eventName]);
      });

      // Relay signalr events to angular over $rootScope
      signalR.eventAggregator.subscribe(SIGNALR_EVENTS[eventName], function(e) {
        $rootScope.$emit(SIGNALR_EVENTS[eventName], e);
      });
    });

  });

angular.module('app', ['SignalrModule'])
  .controller('MyController', function($rootScope, SIGNALR_EVENTS){

    // Using regular angular-ish emit and on

    $rootScope.$emit(SIGNALR_EVENTS.FOO_EVENT, { foo: 'data' });

    $rootScope.$on(SIGNALR_EVENTS.BAR_EVENT, function(){
      // bar event emitted somewhere
    });
  });


Two (or more) LRU caches for a Node.js app Here is a caching code of a nodejs / typescript app using lru Cache module from https://www.npmjs.com/package/lru-cache.
For various reason i need to setup separate caches: one for homeCache another one for detailThreadCache... but this is just the beginning I may need to set up more.

This code is working, but how would you refactor it to improve code reuse ? Would you use object oriented paradigm ? If yes how ?

const homeCache = lruCache( 50 )
const detailThreadCache = lruCache( 150 )

export function homeCachedData( key ) {
    return homeCache.get( key )
}

export function homeSetCache( key: string, value, duration: number ) {
    homeCache.set( key, value, duration )
}

export function detailThreadGetCache( key ) {
    return detailThreadCache.get( key )
}

export function detailThreadSetCache( key: string, value, duration: number ) {
    detailThreadCache.set( key, value, duration )
}

As mentioned in the comments, remove the functions. Don't even go into OOP. As to why, they're simply unnecessary wrappers to your cache objects which you can just access directly. detailThreadCache.set is just as verbose as detailThreadSetCache.

Improving code doesn't necessarily mean reuse or encapsulation or a paradigm shift. Often times, the best code... is the one that's never written.


Returns Office Details from Database I am building an Administration web site for the Office details in my database. Right now, I am just trying to display the office details.

public class OfficeRepository
{
    public static Office GetOffice(int syncID)
    {
        List<Office> results = new List<Office>();

        var sql = @"Select so.SyncID, so.title From Offices o Left Outer Join SyncOffices so On so.id = o.SyncID Where o.SyncID = @syncID";

        using (var connection = new SqlConnection(Settings.ConnectionString))
        using (var command = new SqlCommand(sql, connection))
        {
            command.CommandType = CommandType.Text;
            command.Parameters.AddWithValue("@syncID", syncID);
            connection.Open();

            using (var reader = command.ExecuteReader())
            {
                while (reader.Read())
                {
                    var office = new Office();
                    office.SyncID = reader.GetInt32(0);
                    office.OfficeName = reader.GetString(1);
                    results.Add(office);
                }
            }
        }

        Office returnOffice = new Office();
        returnOffice.SyncID = results.FirstOrDefault().SyncID;
        returnOffice.OfficeName = results.FirstOrDefault().OfficeName;
        return returnOffice;
    }
}

public class HomeController : Controller
{
    public ActionResult Index(int id = 0)
    {
        Office currentOffice = OfficeRepository.GetOffice(id);
        var officeMgrViewModel = new OfficeMgrViewModel();
        officeMgrViewModel.Office = currentOffice;
        return View(officeMgrViewModel);
    }

}

public class OfficeMgrViewModel
{
    public Office Office { get; set; }
}

public class Office
{
    public Int32 SyncID { get; set; }
    public string OfficeName { get; set; }
}

You may want your properties to be private set so that they can't be changed from the outside:

public class Office
{
    public Int32 SyncID { get; private set; }
    public string OfficeName { get; private set; }
}

To do that you can make GetOffice a member of Office instead of OfficeRepository.

Or you can define a constructor ...

// Logically immutable
public class Office
{
    public Office(Int32 SyncID, string OfficeName)
    {
        this.SyncID = SyncID;
        this.OfficeName = OfficeName;
    }
    public Int32 SyncID { get; private set; }
    public string OfficeName { get; private set; }
}

... and change your GetOffice method to invoke the constructor instead of invoking the set properties.

Similarly, OfficeMgrViewModel needn't have a public set property if it had a constructor.

See also this answer which mentions how to simplify your GetOffice method to not use a List, if you expect that your join where will never produce more than one record.


Secured registration script I created code for my registration form on my social network. I want to know if it is secured enough to release on the web as a public social network. Also, all of the code works. I just want to make sure it's all secure.

registration.php:

ini_set('display_errors', 1);
ini_set('display_startup_errors', 1);
error_reporting(E_ALL);

//check if form is submitted
if ( $_SERVER['REQUEST_METHOD'] != 'POST' || ! isset($_POST['Register']))
{
// looks like a hack, send to index.php
header('Location: index.php');
die();
}

include("connect.php");

            if(isset($_POST['Register'])) {

               if (empty($_POST["username"])) {
                echo"Fill in username to sign up";
                } else {

                if (empty($_POST["pw"])) {
                 echo"Fill in password to sign up";
                } else {

                if (empty($_POST["pw2"])) {
                echo"Confirm password to sign up";
                 } else {

                if (empty($_POST["email"])) {
                     echo"Fill in email to sign up";
                 } else {

                if ($_POST['pw'] == $_POST['pw2']) {
                 $username = mysqli_real_escape_string($conn, $_POST["username"]);
                 $pw= mysqli_real_escape_string($conn, $_POST["pw"]);
                 $pw2= mysqli_real_escape_string($conn, $_POST["pw2"]);
                 $email = mysqli_real_escape_string($conn, $_POST["email"]);

                 $result = mysqli_query($conn ,"SELECT * FROM users WHERE username='" . $username . "'");

                    if(mysqli_num_rows($result) > 0)
                    {
                    echo "Username exists . Try again
 ";
                    } else {

                       $result2 = mysqli_query($conn ,"SELECT * FROM users WHERE email='" . $email. "'");

                       if(mysqli_num_rows($result2) > 0)
                       {
                       echo "Email exist.  Try again
 ";
                       } else {

                       $pw = password_hash($pw, PASSWORD_BCRYPT, array('cost' => 14));

                       $stmt = $conn->prepare("INSERT INTO users (username, pw, email) VALUES(?, ?, ?)");
                       $stmt->bind_param("sss", $username, $pw, $email);

                      $username = $username;
                      $pw = $pw;
                      $email = $email;
                      $stmt->execute();

                      echo"Registration successful Login here
";

                    } } } else{
            echo "The passwords do not match.";  // and send them back to registration page
            }
}
}}}}

login.php:

error_reporting(E_ALL);
ini_set('display_errors', 1);

include("connect.php");

$_SESSION['username'] = $_POST['username'];

//check if form is submitted
if ( $_SERVER['REQUEST_METHOD'] != 'POST' || ! isset($_POST['signin']))
{
// looks like a hack, send to index.php
header('Location: index.php');
die();
}

if (empty($_POST["username"])) {
echo 'Fill in username to sign in. Try again
';
die();
}

if (empty($_POST["pw"])) {
echo 'Fill in password to sign in. Try again
';
die();
}

$sql = "SELECT pw FROM users WHERE username = ?";
$stmt = mysqli_prepare($conn, $sql);
if ( !$stmt ) {
echo mysqli_error($conn);
die();
}

$stmt->bind_param('s', $_POST['username']);
if ( !$stmt->execute() ) {
echo mysqli_error($conn);
die();
}
// we found a row with that username,
// now we need to check the password is correct

// get the password from the row
$stmt->bind_result($hashed_pwd);
$stmt->fetch();

if ( password_verify($_POST['pw'], $hashed_pwd) ) {
// password verified
$_SESSION["username"] = $_POST['username'];
header('Location: profile.php');
} else {
echo 'Incorrect username or Password. Try again
';
}

Correctness

  Also, all of the code works.

No, it doesn't, so let's start with that.

                 $username = mysqli_real_escape_string($conn, $_POST["username"]);

  ...

                 $result = mysqli_query($conn ,"SELECT * FROM users WHERE username='" . $username . "'");

                    if(mysqli_num_rows($result) > 0)
                    {
                    echo "Username exists . Try again
 ";
                    } else {

  ...

                       $stmt = $conn->prepare("INSERT INTO users (username, pw, email) VALUES(?, ?, ?)");
                       $stmt->bind_param("sss", $username, $pw, $email);

                      $username = $username;
                      $pw = $pw;
                      $email = $email;
                      $stmt->execute();

If the supplied username is such that mysqli_real_escape_string is not a no-op, the check for an existing user and the insertion of the user use different usernames. This is an important bug. Quite how important depends on things you haven't shown us, in particular on the database schema.

Given the current mix of database access techniques, you should ditch the one which requires you to escape things yourself (dangerous, because it's easy to forget when you change the code) and use prepared statements for everything.

I also struggle to see how this can possibly work:

//check if form is submitted
if ( $_SERVER['REQUEST_METHOD'] != 'POST' || ! isset($_POST['Register']))
{
// looks like a hack, send to index.php
header('Location: index.php');
die();
}

  ...

                    } } } else{
            echo "The passwords do not match.";  // and send them back to registration page

If registration.php is the registration page then (a) it seems rather lacking in content; and (b) there seems to be a catch-22 with rendering it for the first time in order to submit data.

Maintainability

Maintainability is important in its own right, but also particularly relevant to  security because unmaintainable code is more likely to acquire bugs over time.

Fix the indentation. It currently appears to be at random, and is completely unreadable.

Avoid massive chains of if statements. The various checks here would be better refactored into a function which can take advantage of early return.

Avoid pointless code such as

                      $username = $username;
                      $pw = $pw;
                      $email = $email;

Security

This code leaks far too much information to a malicious caller.

ini_set('display_errors', 1);
ini_set('display_startup_errors', 1);
error_reporting(E_ALL);

is absolutely not something you want to do on a production system unless you're also using set_error_handler to log it to somewhere private and ensure that the end user never sees the error messages.

Similarly

echo mysqli_error($conn);

The registration form asks for an e-mail address. Why? If you don't use it, don't ask for it. If you do use it, does your security model not require it to be correct? So shouldn't there be an e-mail validation step in the registration process?


Simplifying AJAX success code Please take a look at this Fiddle.

Is there a better way of building a table with titles in the first cell of each row, like this:

    A
    B
     C

   Germany
   Yes
   No
   Yes

  .........

In building a string of HTML, I use an each(function) for each returned object and then at the end set the div #area with the content. I can't think of a better way of doing this, but would like to know if there is a way to simplify the code. My actual JSON Data is way bigger than the example below, so the code would be very long and use far more each function in the buildup of the string.

JSON:

[{"title":"A","nation":"Germany,Japan","city":"Hamburg,Toyko","Name":"John,Peter,Tom"},{"title":"B","nation":"Japan,Italy","city":"Toyko","Name":"Adam,Tom"},{"title":"C","nation":"Germany","city":"Berlin,Hamburg","Name":"Mary,Tom"}]

Code:

   $.ajax({
        url: "https://query.yahooapis.com/v1/public/yql?q=select%20*%20from%20json%20where%20url%20%3D%22http%3A%2F%2Fgoo.gl%2FRgDyl4%22&format=json&diagnostics=true&callback=",
        success: function (data) {
            var item_html="";

            item_html += "

	";

            $(data.query.results.json.json).each(function (index, item) {
                var title = item.title;
                item_html += ''+title+'	';
            });

            item_html += "

Germany	";

            $(data.query.results.json.json).each(function (index, item) {
                var nations = item.nation.replace(/ /g,'').split(",");
                console.log(nations);
                if ($.inArray('Germany', nations) < 0)
                {
                  item_html += "No	";
                }else{
                  item_html += "Yes	";
                }
                 });

             item_html += "
Berlin	";

             $(data.query.results.json.json).each(function (index, item) {
                var  citys = item.city.replace(/ /g,'').split(",");
                if ($.inArray('Berlin', citys) < 0)
                {
                  item_html += "No	";
                }else
                {
                  item_html += "Yes	";
                }

             });

              item_html += "
Peter	";

             $(data.query.results.json.json).each(function (index, item) {
                var  names = item.Name.replace(/ /g,'').split(",");
                if ($.inArray('Peter', names) < 0)
                {
                  item_html += "No	";
                }else
                {
                  item_html += "Yes	";
                }

             });

           item_html += "

";

           $('#tablearea').append(item_html);

        }
    });

you can merge

 var item_html="";

 item_html += "

	";

to

var item_html="

	";

and i also cant see the id "tablearea" in your html.
UPDATE:
by your fiddle link that tablearea is the id of a div and are you sure that you want to use $('#tablearea').append(item_html); because it will append to div everytimes whenever your ajax method called.

so better that you use

$('#tablearea').html(item_html);


Input/output for a simple Brainfuck interpreter The following code comes from a simple Brainfuck interpreter I'm working on to learn the Rust language. Error handling is omitted for simplicity. It is tested on rustc version 1.1.0-dev (af522079a 2015-05-14) (built 2015-05-14).

The code implements just the input and output routines for the interpreter. The read_cell method reads a single byte from the Read object input and stores it in the memory cell at pointer. write_cell writes the byte a the memory cell at pointer to the Write object output.

use std::io::Read;
use std::io::Write;

pub struct Machine<In, Out> {
    pub memory: Vec<u8>,
    pub pointer: usize,
    input: In,
    output: Out,
}

impl<In: Read, Out: Write> Machine<In, Out> {
    pub fn new(input: In, output: Out) -> Machine<In, Out> {
        Machine {
            memory: vec![0; 2],
            pointer: 0,
            input: input,
            output: output
        }
    }

    pub fn read_cell(&mut self) {
        self.input.read(&mut self.memory[self.pointer..self.pointer+1]).unwrap();
    }

    pub fn write_cell(&mut self) {
        self.output.write(&self.memory[self.pointer..self.pointer+1]).unwrap();
    }
}

#[cfg(test)]
mod test {

    use super::*;

    #[test]
    fn test_write_cell() {
        let input = "".as_bytes();
        let mut output = vec![];
        {
            let mut machine = Machine::new(input, &mut output);
            machine.memory[machine.pointer] = 1;
            machine.write_cell();
        }
        assert_eq!(vec![1], output);
    }
}

There are two main points in the code that are concerning me:

I cannot find in the standard library API an obvious method to read or write a single byte. There is a better/clearer way to implement the read_cell and write_cell methods?
In the test function test_write_cell I had to declare machine inside a nested scope. If it is declared in the same scope of output I get this error at the assert_eq! line:

 cannot borrow `output` as immutable because it is also borrowed as mutable

Can I write the same test without having to nest scopes? Is so, how?

Any other comment, hint or advice will be very welcome!

a better/clearer way to implement the read_cell and write_cell methods?

What you have seems pretty good to me. Rust's IO methods rely on having a buffer to read into. In many cases, you might see a single byte read with something like

let mut buf = [0; 1];
reader.read(&mut buf).unwrap();

In your case, you already have a buffer, so you might as well read directly into it.

  Can I write the same test without having to nest scopes?

Nope. Let's comment out the nested braces and look at the error messages:

error: cannot borrow `output` as immutable because it is also
       borrowed as mutable

match ( & ( $ left ) , & ( $ right ) ) {
                         ^~~~~~~~~~~
note: in expansion of assert_eq!

We are attempting to immutably borrow something that's already borrowed. The compiler is kind enough to show us where the borrow occurs:

note: previous borrow of `output` occurs here; the mutable borrow
      prevents subsequent moves, borrows, or modification of
      `output` until the borrow ends

let mut machine = Machine::new(input, &mut output);
                                           ^~~~~~

And where it ends:

note: previous borrow ends here

fn test_write_cell() {
...
}
^

When there is an outstanding mutable borrow, Rust only that borrow to exist. This removes whole classes of bugs around "spooky action at a distance" as well as potential data races. The extra braces provide a lifetime that the borrow can live during; once the block is exited, the borrow is no longer needed.

The only solution I know of is to use what I like to call an exploder. If you are familiar with constructors and destructors, then an exploder is like a destructor that returns values. There's a common pattern in the standard library with methods called into_inner. Here's how it could look for your code:

impl<In: Read, Out: Write> Machine<In, Out> {
    pub fn into_inner(self) -> (In, Out) {
        (self.input, self.output)
    }
}

fn test_write_cell() {
    let input = "".as_bytes();
    let output = vec![];

    let mut machine = Machine::new(input, output);
    machine.memory[machine.pointer] = 1;
    machine.write_cell();

    let (_, output) = machine.into_inner();

    assert_eq!(vec![1], output);
}


Checking if a column belongs to a provided list I've the following code, which checks if one column belongs to the list I provide.  After that, it takes the value contained in it.

I've tried and it's 9 times faster than raising and handling an exception. But I don't find this code very readable.

For example, I don't need to define a variable drsIDAgentClient. I've thought of using an extension method over reader and provide a ColumnExist(string columnName) method.

Can you see a better solution?

DataRow[] drsIDAgentClient = reader.GetSchemaTable().Select("ColumnName = 'IDAgentClient'");
if (drsIDAgentClient.Length > 0)
    coupon.IdAgentClient = reader["IDAgentClient"] == DBNull.Value ? null : (int?)reader["IDAgentClient"];

Can you see a better solution?

Yes use braces {} also for single line statements of an if condition.

Using .Any() instead of checking .Length > 0 is faster but only for an IEnumerable

I also would enclose reader["IDAgentClient"] == DBNull.Value in () as it is more readable or even better you can introduce a boolean isDbNullValue

DataRow[] drsIDAgentClient = reader.GetSchemaTable().Select("ColumnName = 'IDAgentClient'");
if (drsIDAgentClient.Length > 0)
{
    Boolean isDbNullValue = (reader["IDAgentClient"] == DBNull.Value);
    coupon.IdAgentClient = isDbNullValue  ? null : (int?)reader["IDAgentClient"];
}

After rereading your question it seems that you want to know if a column with a specific name exists in the SchemaTable.

So you can use instead of the code above simply this

String columnName = "IDAgentClient";
if (reader.GetSchemaTable().Columns.Contains(columnName))
{
    Boolean isDbNullValue = (reader[columnName] == DBNull.Value);
    coupon.IdAgentClient = isDbNullValue  ? null : (int?)reader[columnName];
}

I would like to expand @Malachi's answer with not using a tenary

Malachi's check

bool IsColumnEmpty(string columnName)
{
    return reader.GetSchemaTable().Columns.Contains(columnName))
        ? Reader[columnName] == DBNull.Value
        : true;
}

can be rewritten like

bool ColumnHasValue(string columnName)
{
    return reader.GetSchemaTable().Columns.Contains(columnName) &&
           Reader[columnName] != DBNull.Value;
}

and if you really need to use an extension method you can do this like

public static class HelperExtension
{
    public static bool ColumnHasValue(this IDataReader reader, string columnName)
    {
        return reader.GetSchemaTable().Columns.Contains(columnName) &&
               Reader[columnName] != DBNull.Value;
    }
}

and call it

coupon.IdAgentClient = reader.ColumnHasValue(columnName) ? (int?)reader[columnName] : null;


Inclusion and exclusion Problem Statement:

  First Case: Where inclusion and exclusion both are not empty so that means inclusion overrides anything in exclusion so for that algorithm is like this:

  If key is either equal to 155 or 156 and if temp is there in inclusion
  then go into that if loop and do whatever you want. But if temp is not
  there in inclusion then don't do anything.

  Second Case: Where inclusion is null and exclusion is not null so that means now you need to exclude everything present in exclusion string, so the algorithm is like this:

  If key is either equal to 155 or 156 and if temp is not there in exclusion then do whatever you want to do in that else loop otherwise don't do anything.

  Third Case: If key is not equal to 155 or 156 then do whatever you want to do in that loop.

Is there any way we can improve this code in terms of performance, readability, accuracy? as when I see this it contains lot of if/else statements to make the above problem works

P.S: Things that I am doing in each if/else loop is same everytime.

Cases1: Where input parameters can be-

String inclusion = null; // It can be null also
String exclusion = "100;77";

String temp = "0"; // It can change anytime

Cases2: Where input parameter can be-

String inclusion = "0"; // It can be null also
String exclusion = null;

String temp = "0"; // It can change anytime

Below is the code in which above input parameters can be used:

for(TesxAttrKey key: pdsxWriteRequest.getAttrKeys()){

if((key.equals("156") || key.equals("155"))) {
    if(inclusion != null && !inclusion.trim().isEmpty()) {
        if(inclusion.matches("(^|.*;)" + temp + "(;.*|$)")) {
            //Check the DsxAttrValue has a Synthesizer
            if(metadata!=null){
                Synthesizer synthesizer = metadata.getSynthesizer();
                if(synthesizer!=null){
                    //Synthesize the attribute
                    DsxAttrValue synthesizedVal =
synthesizer.synthesize(key, record);
                    val = synthesizedVal;
                }
            }
            if(val!=null){
                writeRecord.getAttrs().put(key, val);
            }
        }
    } else {
        if(!exclusion.matches("(^|.*;)" + temp + "(;.*|$)")) {
            //Check the DsxAttrValue has a Synthesizer
            if(metadata!=null){
                Synthesizer synthesizer = metadata.getSynthesizer();
                if(synthesizer!=null){
                    //Synthesize the attribute
                    DsxAttrValue synthesizedVal =
synthesizer.synthesize(key, record);
                    //if(synthesizedVal!=null){
                    val = synthesizedVal;
                    //}
                }
            }
            if(val!=null){
                writeRecord.getAttrs().put(key, val);
            }
        }
    }
} else {
    //Check the DsxAttrValue has a Synthesizer
    if(metadata!=null){
        Synthesizer synthesizer = metadata.getSynthesizer();
        if(synthesizer!=null){
            //Synthesize the attribute
            DsxAttrValue synthesizedVal = synthesizer.synthesize(key, record);
            //if(synthesizedVal!=null){
            val = synthesizedVal;
            //}
        }
    }
    if(val!=null){
        writeRecord.getAttrs().put(key, val);
    }

}

Is there any way we can improve this code in terms of performance, as I have lot of if/else loops to solve the above problem?

Your logic seems good. The problem statement was a tough to understand, but I think I got it.

Since inclusion and exclusion do not change, we should extract that checking from the loop and place it before the loop.

Furthermore, to reduce programmer errors, I reduced the code duplicity. The "do whatever you want" is now only typed once, and the regex pattern is only typed once.

String tempPattern = "(^|.*;)" + temp + "(;.*|$)";

boolean tempInInclusionOrNotInExclusion;
if(inclusion != null && !inclusion.trim().isEmpty()){
    tempInInclusionOrNotInExclusion = inclusion.matches(tempPattern);
} else if(exclusion != null) {
    tempInInclusionOrNotInExclusion = !exclusion.matches(tempPattern);
} else {
    tempInInclusionOrNotInExclusion = true;
}

for(PdsxAttrKey key: pdsxWriteRequest.getAttrKeys()){
    boolean shouldDoWhatIWantToDo;
    if(key.equals("156") || key.equals("155")) {
        shouldDoWhatIWantToDo = tempInInclusionOrNotInExclusion;
    } else {
        shouldDoWhatIWantToDo = true;
    }

    if(shouldDoWhatIWantToDo){
        //Check the PdsxAttrValue has a Synthesizer
        if(metadata!=null){
            Synthesizer synthesizer = metadata.getSynthesizer();
            if(synthesizer!=null){
                //Synthesize the attribute
                PdsxAttrValue synthesizedVal = synthesizer.synthesize(key, record);
                //if(synthesizedVal!=null){
                val = synthesizedVal;
                //}
            }
        }
        if(val!=null){
            writeRecord.getAttrs().put(key, val);
        }
    }
}


Strategy Pattern in Java 8 I need to know if the following lines of code represent a valid implementation of Strategy Pattern in Java 8:

public interface AIStrategy {
    void play();
}

public class AIPlayer {

    private AIStrategy aiStrategy;

    public AIPlayer() {
    }

    public AIPlayer(AIStrategy aiStrategy) {
        this.aiStrategy = aiStrategy;
    }

    public void changeStrategy(AIStrategy aiStrategy) {
        this.aiStrategy = aiStrategy;
    }

    public void fight() {
        aiStrategy.play();
    }
}

public class App {

    public static void main(String[] args) {
        AIPlayer aiPlayer = new AIPlayer();
        String difficulty;

        Scanner sc = new Scanner(System.in);
        difficulty = sc.next();

        switch (difficulty) {
            case "chill":
                aiPlayer = new AIPlayer(()
                        -> System.out.println("The opponent is playing a chill strategy..."));
                aiPlayer.fight();
                break;
            case "calculated":
                aiPlayer = new AIPlayer(()
                        -> System.out.println("The opponent is playing a calculated strategy..."));
                aiPlayer.fight();
                break;
        }

        System.out.println("Is the opponent hurt?");
        boolean isHurted = sc.nextBoolean();
        if (isHurted) {
            aiPlayer.changeStrategy(()
                    -> System.out.println("The opponent changed his strategy. Now he is having a holy rage!!"));
            aiPlayer.fight();
        }
    }
}

Yes, this is a good implementation of the strategy pattern. I don't know if the code you posted is just a simplification or not but I think that you have to prevent a NullPointerException when a client class doesn't specify the strategy to use. You can achieve this task in many ways, for example eliminating the empty constructor or inserting a default strategy in the empty constructor. Another elegant way would be making the strategy a parameter of the fight() method (obviously only if it used within the method):

public void fight(AIStrategy strategy) {
     strategy.player();
}

Making this way you will make the classes lighter, and it will be more difficult to get a NullPointerException.

public class App {

    public static void main(String[] args) {
        AIPlayer aiPlayer = new AIPlayer();
        AIStrategy stragey = () -> System.out.println("Default strategy");
        Scanner sc = new Scanner(System.in);
        String difficulty = sc.next();
        switch (difficulty) {
            case "chill":
                strategy = () -> System.out.println("The opponent is playing a chill strategy...");
                break;
            case "calculated":
                strategy = () -> System.out.println("The opponent is playing a calculated strategy...");
                break;
        }
        aiPlayer.fight(strategy);
        System.out.println("Is the opponent hurt?");
        boolean isHurted = sc.nextBoolean();
        if (isHurted) {
            strategy = () -> System.out.println("The opponent changed his strategy. Now he is having a holy rage!!"));
            aiPlayer.fight();
        }
        aiPlayer.fight(strategy);
    }
}


Searching multidimensional arrays I have been trying to parse results and make sure when I am doing so that I don't repeat the same data. I tried to use a few different options built into php with no luck. I did find an example of a recursive array search that seems to work but it's very intensive and adds a lot of time to the script.

What I'm needing: Does anyone know a better way to handle this without changing the array that I supply to it so something built-in like in_array or array_search?

Array example:

array (size=3)
  0 =>
    array (size=3)
      'author' => string 'Jim Beam' (length=8)
      'id' => string '1' (length=1)
      'md5' => string 'f2ebf4d4f333c31ef1491a377edf2cc4' (length=32)
  1 =>
    array (size=3)
      'author' => string 'Jack Daniels' (length=12)
      'id' => string '2' (length=1)
      'md5' => string 'd1839707c130497bfd569c77f97ccac7' (length=32)
  2 =>
    array (size=3)
      'author' => string 'Jose Cuervo' (length=11)
      'id' => string '3' (length=1)
      'md5' => string '64e989b4330cc03dea7fdf6bfe10dda1' (length=32)

Code example:

function recursive_array_search($needle,$haystack) {
    foreach($haystack as $key=>$value) {
        $current_key=$key;
        if($needle===$value OR (is_array($value) && recursive_array_search($needle,$value) !== false)) {
            return $current_key;
        }
    }
    return false;
}

$agentArray = array(
    array('author'=>'Jim Beam','id'=>'1','md5'=>'f2ebf4d4f333c31ef1491a377edf2cc4'),
    array('author'=>'Jack Daniels','id'=>'2','md5'=>'d1839707c130497bfd569c77f97ccac7'),
    array('author'=>'Jose Cuervo','id'=>'3','md5'=>'64e989b4330cc03dea7fdf6bfe10dda1')
);

$fakeMD5 = '84d7dc19766c446f5e4084e8fce87f82'; //StackOverflow MD5
$realMD5 = 'd1839707c130497bfd569c77f97ccac7'; //Jack Daniels MD5

echo 'In_Array:
';

$faketest = in_array($fakeMD5,$agentArray);
$realtest = in_array($realMD5,$agentArray);

var_dump($faketest,$realtest);

echo 'Search_Array:
';

$faketest2 = array_search($fakeMD5,$agentArray);
$realtest2 = array_search($realMD5,$agentArray);

var_dump($faketest2,$realtest2);

echo 'Custom Recursive Array Seach Function:
';

$faketest3 = recursive_array_search($fakeMD5,$agentArray);
$realtest3 = recursive_array_search($realMD5,$agentArray);

var_dump($faketest3,$realtest3);

Results:

In_Array:
Fake: boolean false
Real: boolean false

Search_Array:
Fake: boolean false
Real: boolean false

Custom Recursive Array Seach Function:
Fake: boolean false
Real: int 1

I have two comments.

First, what is the purpose of $current_key? Since you're not changing it, just use $key.

Second, by your usage, I'd say that

foreach ($haystack as $key => $item)
    if ($item["md5"] === $needle) return $key;
return false;

is quite enough. Of course, if the above was just an example and you really want to check all the values in (sub)arrays, then the above is O.K.

Of course, be careful when using this function, because key 0 may be interpreted as false.

A bit neater (but essentially the same) function was provided on Stack Overflow here. I like the additional $strict argument there (which you may or may not need).