List files and download The following code list all files in a folder and allows the user to download one of them. It is working but I think it could be done better.

Is there a way to reduce the number of forms to only one and to improve the way I retrieve the name of the file to download?

My second concern is about the security of my getFile function.

function listDir($directoryName)
{
    $directory = opendir($directoryName);
    $files = array();
    while ($elem = readdir($directory))
    {
        if ($elem != '.' && $elem != '..' && $elem != '.htaccess')
        {
            $files[] = $elem;
        }
    }
    closedir();
    if (!empty($directory))
    {
        sort($files);
        echo "
";
        foreach ($files as $file)
        {
            $filePath = $directoryName.'/'.$file;
            echo "<form method='post' action=".htmlentities($_SERVER['PHP_SELF']).">";
            echo "
($fileDate)";
            echo "<input type='hidden' name='$file' value='$filePath'>";
            echo " <button type='submit' name='getFile'>Télécharger</button> ";
            echo "$file
";
            echo "</form>";
        }
        echo "
";
    }
}

function getFile($file)
{
    if (file_exists($file))
    {
        header('Content-Description: File Transfer');
        header('Content-Type: application/octet-stream');
        header('Content-Disposition: attachment; filename='.basename($file));
        header('Expires: 0');
        header('Cache-Control: must-revalidate');
        header('Pragma: public');
        header('Content-Length: ' . filesize($file));
        ob_clean();
        flush();
        readfile($file);
        exit;
    }
}

if (isset($_POST['getFile']))
{
    $keys = array_keys($_POST);
    $file = $_POST[$keys[0]];
    getFile($file);
}

listDir('data');

Let's post a dry review first, before jumping in to the conceptual stuff:

You are allowing your end-users download any file on the server. All I need to do is to change the path to /etc/passwd on any form and submit, and I get your server's password file! And if I don't have permissions to that, no biggy, I can still download all of your owned files on the server.
Invalid HTML markup - <form> isn't allowed directly inside of <ul>. Only <li> is.
Separate logic and presentation - You shouldn't mix the part where the user sees your form, and the part where he downloads the file.

A better approach?

A better approach would be a PHP file which is capable of serving files. That PHP file needs to make sure that the file being requested is found in a very specific list of allowed directories (i.e. I allow from images/ and from static/ but not from anywhere else.

Then, when you download a file, all you need to do is to link (With an <a>) to

http://example.com/path/to/file_server.php?filename=somefile.jpg

And have the PHP file_server handle the serving of the requested file.

Getting Fancy

You can even use .htaccess and mod_rewrite to rewrite URLs like this:

http://example.com/static/somefile.jpg

to

http://example.com/path/to/file_server.php?filename=somefile.jpg

Making the fact that PHP is the one sending the file completely transparent to the user!


Mixing Lens operations with "original" operations Is it reasonable to mix Control.Lens operations with the ones provided by the "standard library"?

Consider the following fragment using qualified Data.Map as Map:

getUniqueIdent :: String -> State (Map.Map String Int) (Maybe String)
getUniqueIdent typeName = do indices <- StM.get
                             let identifiers = Map.lookup typeName typeNameMap
                             let uniqueIndex = Map.lookup typeName indices
                             StM.modify $ over (at typeName) (fmap succ)
                             let uniqueIdent = (^?) <$> identifiers <*>
                                                        (element <$> uniqueIndex)
                             return $ join uniqueIdent

For example, I do use both the Map.lookup and Lens.at and also the (^?) as a safe alternative for (!!).

Should I just stick to lenses "everywhere" (at least in spatially local code)?

I wouldn't say there's anything like a proscription against doing so, but as a non-user of lens I can't say it helps me understand what's going on. I'm also not sure that your data structure is complex enough to warrant using lens in this case, functions out of base tend to produce a pretty concise solution on their own when properly deployed.

I haven't tested this, but I think it's equivalent.

import Control.Monad.State
import Data.Map
import Safe (atMay) -- "safe".Safe.atMay :: [a] -> Int -> Maybe a

getUniqueIdent :: String -> State (Map String Int) (Maybe String)
getUniqueIdent typeName =
  do indices <- get
     modify $ adjust succ typeName
     return $ do identifiers <- Data.Map.lookup typeName typeNameMap
                 uniqueIndex <- Data.Map.lookup typeName indices
                 identifiers `atMay` uniqueIndex


Uploading images via MySQL in Blob Is it safe to upload images via MySQL in Blob? I've constructed my code like the following:

    public function upload_avatar($file, $user_id = false, $maxbytes = 102400) {
    global $sql;
    if($user_id == false) {
        $user_id = $this->_id;
    }
    $check = getimagesize($file);
    if($check == false) {
        return false;
    }
    $width = $check[0];
    $height = $check[1];
    if($width > 300) {
        return false;
    }
    if($height > 300) {
        return false;
    }
    $fp = fopen($file, 'rb');
    if(strlen($file) < $maxbytes) {
        $q = $sql->prepare("UPDATE `users` SET `avatar` = :file WHERE id = :user_id;");
        try {
            $q->bindParam(':file', $fp, PDO::PARAM_LOB);
            $q->bindParam(':user_id', $user_id);
            $q->execute();
        }
        catch(PDOException $e) {
            die($e->getMessage());
        }
        return true;
    } else {
        return false;
    }
}

Is it possible for a hacker to upload viruses via this script?

Nitpicks

public function upload_avatar($file, $user_id = false, $maxbytes = 102400) {
global $sql;

These two lines are indented inconsistently.  The first line should be indented less than the second.  Perhaps a copy/paste error.

There's an argument that the global line should be used closer to where the $sql variable is used.

I personally don't like the use of $sql to represent a database connection.  To me, SQL is a language rather than a generic name for a database.

To be consistent with the other names (e.g. $user_id), $maxbytes should be $max_bytes.

I'd like this function better if it had more vertical whitespace.  In particular, I would expect a blank line after global $sql; to separate the declaration from the code that follows.  Then another line after the if block to separate the user ID check from the code relating to the getimagesize call.  This is called paragraphing, and it breaks your function into smaller segments that are easier to read and process.

$check = getimagesize($file);
if($check == false) {
    return false;
}

This is the security portion of your code.  It verifies that the file is something that getimagesize would consider to be an image.  You should probably comment this, as it is rather important in the context of this function.  It's also not immediately obvious that it is security related.  The more common interpretation would be that you are failing here purely because you can't get the size information.

I might call $check $image_data instead.  That would be clearer about what is being returned.

$width = $check[0];
$height = $check[1];

Technically speaking, this is unnecessary.  You only use those values once each and could just use them rather than reassigning them.  However, this does make it considerably clearer what the code is doing.

if(strlen($file) < $maxbytes) {

This is unlikely to be doing what you want.  Presumably you want to check if the image size is less than $maxbytes.  But what this actually does is check that the file name is less than $maxbytes.

        $q->bindParam(':user_id', $user_id);

This is binding $user_id as a string.  If $user_id is numeric, you may be better off telling it so.

Security Issues

As I said earlier, you are checking that $file is an image with the getimagesize function.  Unfortunately though, this might not work.  The problem is that in PHP $file can be either a local file or a remote file.  If it's a local file, then this should be fine so long as the file doesn't change from under you.  Since it's local, hopefully you can control that.

If it's a remote file, then someone else can answer the getimagesize with one file and then return a different file to the fopen.  They control the file, so the first access can be to a nice image and the second access to a dangerous file.  So if this function is actually doing the upload, because you are passing it a URL, then it is insecure.  You should get the file before doing the getimagesize (or do getimagesize twice:  once before and once after).  That way you can ensure that the getimagesize file is the same as the fopen file.

You can also use the filesize function to check the file size of a local file against $maxbytes.  That may not work with a remote file.

Performance

This approach does put more pressure on the database.  You are storing static image content in the database.  It's not clear that you need to do this.  It would generally be easier to store the image on your server and supply it from there.  If you have multiple servers, you could designate one to hold the content.  Or you can use a service like Amazon's S3 to hold your images and load them from there.

It's also unclear how you are going to display the image, but it will certainly require reading the image into memory from the database.  That can stomp over things like the query cache, displacing smaller, more cacheable content.

Display Issues

There are other issues that may impact performance and security based on how you display the image.  Rather than try to guess at the specifics of your method, I'll leave that to a future code review.  But you should be aware that there are additional issues that may arise depending on exactly how you display the image.  Since it is untrusted user data, this could include a security hole that is not visible from just this perspective.  We really need to know the whole sequence to evaluate it.

More context makes it easier to evaluate code.  Some problems may only appear in the interaction of two systems.  Or something may be an irrelevant comment because it's handled outside the code that we can see.


Count unique IP address in a date range from Apache log I use the following command to get specific entries between certain dates from Apache log files. Then I filter out my own address and some bots, then I print out the IP, sort it, get the uniques, output the ip with count, and for summary I count lines to get a total.

Is there a way to improve this, reducing the amount of awk commands?
Or to get this process down more efficient? I'm not that proficient with awk, but I have the feeling this should be accomplish able in maybe one awk instruction.

zgrep '"GET /my/path/to/page.html' other_vhosts_access*
| awk -F'[][]'
    -v dstart=`date -d"2019-12-09" +%Y%m%dT%0H:%0M:%0S`
    -v dend=`date -d"2020-01-09" +%Y%m%dT%0H:%0M:%0S`
    '{ $2 = substr($2,8,4)sprintf("%02d",(match("JanFebMarAprMayJunJulAugSepOctNovDec",substr($2,4,3))+2)/3)substr($2,1,2)"T"substr($2,13,8);
    if ($2 >= dstart && $2 < dend) print }'
| awk '$0 !~ /127\.0\.0\.1|bot\.|bot\/|dotbot|crawler/'
| awk '{print $2}'
| sort
| uniq -c
| sort -n
| wc -l

the one liner:

zgrep '"GET /my/path/to/page.html' other_vhosts_access* | awk -F'[][]' -v dstart=`date -d"2019-12-09" +%Y%m%dT%0H:%0M:%0S` -v dend=`date -d"2020-01-09" +%Y%m%dT%0H:%0M:%0S` '{ $2 = substr($2,8,4)sprintf("%02d",(match("JanFebMarAprMayJunJulAugSepOctNovDec",substr($2,4,3))+2)/3)substr($2,1,2)"T"substr($2,13,8); if ($2 >= dstart && $2 < dend) print }' | awk '$0 !~ /127\.0\.0\.1|bot\.|bot\/|dotbot|crawler/' | awk '{print $2}' | sort | uniq -c | sort -n | wc -l

I assume there's a simply copy-paste error where you have | at the start of a line, instead of at the end of the preceding line?

If we're just going to count lines, there's no need for sort | uniq -c | sort -n - we can replace all that with sort -u.

These two Awk programs can be trivially combined:

awk '$0 !~ /127\.0\.0\.1|bot\.|bot\/|dotbot|crawler/' | awk '{print $2}'

That would become

awk '$0 !~ /127\.0\.0\.1|bot\.|bot\/|dotbot|crawler/ {print $2}'


Enum vs lookup table for temporary and permanent teeth I'm implementing a solution in Java using Ebean and I'm having some issues choosing between using enums or simply look up tables.

I have a table "Tooth". A tooth can be "Temporary" or "Permanent".

I could create a simple enum:

@EnumMapping(nameValuePairs = "TEMPORARY=T, PERMANENT=P")
public enum DentitionType { TEMPORARY, PERMANENT; }

However, if I want to do a direct SQL query, I have to convert "T" and "P", so a solution would be to use a lookup table as below:

@Entity
public class DentitionType {

   @Column(length = 15)
   public String name;

    private static DentitionType permanent;

    public boolean isTemporary() {
        return !this.equals(getPermanent());
    }

    public boolean isPermanent() {
        return this.equals(getPermanent());
    }

    public static DentitionType getPermanent() {
        if (permanent == null) {
            permanent = DentitionType.FIND.byId(2L);
        }

        return permanent;
    }
}

This feels kind of hardcoded and for larger tables a lot of isSomething functions are required.

Is there a better solution?

I would recommend making an enum with properties, such as:

public enum DentitionType {
    TEMPORARY(1), PERMANENT(2);

    private final int id;
    private DetentionType(int id) {
        this.id = id;
    }

    public int getId() {
        return this.id;
    }

    public static DententionType forId(int id) {
        for (DententionType dentention : DetentionType.values()) {
            if (dentention.getId() == id) {
                return dentention;
            }
        }
        return null;
    }
}

This is an easy way of mapping your enum to an int (you can also use a 'char' or 'String' or whatever you feel like). Then you can inside your @Entity class use some JPA events to transform to/from enum and int.

Another solution is of course to simply use the @Enumerated annotation and automatically use the ordinal of the enum. Be aware however that there has been incidents where someone accidentally switched the order of the enums, which caused DIAMONDS to become black.


Very slow Discord bot to play music I have been writing a discord.py bot, running on replit.com. I gotten to the music bot part after finishing the queue part. My code is logically correct; however, the code is way too inefficient causing the bot to crash when someone adds another song to the queue. How can I program my bot to be more efficient? If my code must be completely different, then a short explanation would be appreciated.
The code for play:
@bot.command(name='play', aliases=['sing', 'p'])
async def _play(ctx, url : str):
    queueurls = [] #list with the urls
    vc = ctx.voice_client
    if not vc:
      channel = ctx.author.voice.channel
      await channel.connect()
    url = ctx.message.content
    url = ttourl(url) #the message is processed to an url
    queueurls.append(url) #add the url to the list
    voicec = ctx.voice_client
    if not voicec.is_playing(): #when nothing is going on
      while queueurls != []: #as long as the url list isn't empty I want the process to repeat
        voice = discord.utils.get(bot.voice_clients, guild=ctx.guild)
        url = queueurls[0] #url is the first item in the list
        queueurls.pop(0) #now erase the now used first item (url)
        with youtube_dl.YoutubeDL(ytdlopts) as ydl:
          ydl.download([url]) #download the song
        voice.play(discord.FFmpegPCMAudio("songstemp/song.mp3")) #play the song
        vcs = ctx.voice_client
        while vcs.is_playing: #wait for the song to be over
          continue
        if os.path.exists("songstemp/song.mp3"):
          path = "songstemp/song.mp3"
          os.remove(path)
    else: #if the bot is already playing, then don't disturb the loops above
      pass

Prefer four-space indentation instead of a mix of four and two
queueurls should be queue_urls
else / pass is redundant and can be deleted
It's not a great idea to use a single, statically-named temporary file for your playback. Instead, use tempfile to generate you a random one-off temporary filename. It's possible that the inefficiency you see is actually some kind of contention around your single filename.
while queueurls != [] is better-stated as while len(queue_urls) > 0.
while vcs.is_playing is a polling loop that's probably going to saturate your CPU. At the least, add a sleep(0.1) to the inside of your loop to release your thread from the OS scheduler. Better yet would be a blocking call to your audio library, though I don't know whether that's possible. Based on the documentation, you could wait until an after finalizer hook is called.


Combining text in an array in JavaScript My JS looks like this:

preLoadImages("/media/img/elements/frontpage/1.jpg","/media/img/elements/frontpage/2.jpg","/media/img/elements/frontpage/3.jpg","/media/img/elements/frontpage/4.jpg","/media/img/elements/frontpage/5.jpg","/media/img/elements/frontpage/6.jpg","/media/img/elements/frontpage/7.jpg","/media/img/elements/frontpage/8.jpg","/media/img/elements/frontpage/9.jpg","/media/img/elements/frontpage/10.jpg");

What would be a better way to do this?

var directory = "/media/img/elements/frontpage/"

preLoadImages(directory+"1.jpg",directory+"2.jpg",directory+"3.jpg",directory+"4.jpg",directory+"5.jpg",directory+"6.jpg",directory+"7.jpg",directory+"8.jpg",directory+"9.jpg",directory+"10.jpg");

Doesn't seem much better. There must be a zip/map/join type thing I can use here?

perhaps you can try apply(), then you can pass in the image paths as an array:

preLoadIamges.apply(this, absoluteFileNames)

then you can build you array in a the way you like, perhaps looking at this unobtrusive implementation of Array.map:

var directory = "/media/img/elements/frontpage/"
var fileNames = ["1.jpg","...","9.jpg"];
var absoluteFileNames = fileNames.map(function (value) {
    return directory + value;
});


Permutations Script Needs Optimization I have functions of permutation :

function permutations($letters,$num)
{
   $last = str_repeat($letters[0],$num);
   $result = array();
   while($last != str_repeat($this->lastchar($letters),$num))
   {
       $result[] = $last;
       $last = $this->char_add($letters,$last,$num-1);
   }
   $result[] = $last;
   return $result;
}

function char_add($digits,$string,$char)
{
    error_reporting(0);

    if($string[$char] < $this->lastchar($digits))
    {
       $string[$char] = $digits[strpos($digits,$string[$char])+1];
       return $string;
    }
    else
    {
       $string = $this->changeall($string,$digits[0],$char);
       return $this->char_add($digits,$string,$char-1);
    }
}

function lastchar($string)
{
   return $string[strlen($string)-1];
}

function changeall($string,$char,$start = 0,$end = 0)
{
   if($end == 0) $end = strlen($string)-1;
   for($i=$start;$i<=$end;$i++){
       $string[$i] = $char;
   }
   return $string;
}

Example to run this :

print_r($this->permutations('1234',2));

And results will be:

Array(
[0] => 11
[1] => 12
[2] => 13
[3] => 14
[4] => 21
[5] => 22
[6] => 23
[7] => 24
[8] => 31
[9] => 32
[10] => 33
[11] => 34
[12] => 41
[13] => 42
[14] => 43
[15] => 44)

It can run well. However, when I run with greater number of permutation like : $this->permutations('1234',5) , the script will cause memory exhaustion.

What can I change to optimize the functions ?

I've approached it from a hint @hacatu said, that you are storing all the permutations in an array. I thought move this to a generator, although I think trying out Heap's algorithm could be beneficial too.

I'm only adding the function that has changes, the rest of the code stayed the same. When using generators, you don't need to build up a whole array, you get an Generator object which implements the Iterator interface. The execution of your code stops at the yield statement and only continue if next() is called ie in a foreach

function permutations($letters,$num)
{
    $last = str_repeat($letters[0],$num);
    // no result array
    while($last != str_repeat($this->lastchar($letters),$num))
    {
        yield $last; // <-- one change
        $last = $this->char_add($letters,$last,$num-1);
    }

    yield $last; // <-- another change
}

I've put these methods in a class called Permutation and used it like this:

$perm = new Permutation();
$perms = $perm->permutations('1234', 10); // $perms is a Generator object here

// I'm looping through the results, if needed you can build up an array here, but that will need a lot of memory as you already seen
foreach ($perms as $result) {
    printf("%s\n", $result);
}

printf("Max memory: %sK\n", number_format(memory_get_peak_usage(true)/1024));

When I run this code (as it doesn't do much just prints) it ends up printing me:

lots of permutations here
....
4444444444
Max memory: 2,048K

When I add all the permutations to an array, the memory usage jumps

lots of permutations here
....
4444444444
Max memory: 75,780K

I hope this helps.

Ps: If you are into JavaScript and redux have a look at redux-saga, it can also help understanding generators. If not, I hope the PHP docs will help you :)


Random Weighted Classifier in R I am computing a random weighted classifier based on the rates at which 3 labels appear in a "train" set. I want to use this RWC as a baseline for other classifiers. I'm doing this over 1000 iterations and then computing the mean of F1, Precision and Recall of each class besides the overall kappa.

Can this code run faster/look nicer? Minimum example here:

library(caret)

random_weighted_classifier <- function(weightA, weightB, weightC){
  random_number = sample(1:100,1) / 100

  if(random_number <= weightA){
    return("better")
  }else if (random_number > weightA && random_number <= (weightA + weightB)){
    return("worse")
  }else if(random_number > (weightA + weightB) && random_number <= (weightA + weightB + weightC)){
    return("no change")
  }
}

test <- function(){

  betters = rep(x = "better", 100)
  worses = rep(x = "worse", 50)
  no_changes = rep(x = "no_change", 10)
  reference = sample(c(betters, worses, no_changes))

  better = sum(reference == "better")
  worse = sum(reference == "worse")
  no_change = sum(reference == "no_change")
  total = length(reference)

  # rwc = random weighted classifer
  prediction_rwc = vector("character", total)

  iterations = 1000
  f1_rwc = matrix(0., iterations, 3)
  pres_rwc = matrix(0.,iterations, 3)
  rec_rwc = matrix(0., iterations, 3)
  kappa_rwc = vector("double", iterations)

  for(i in seq(1:iterations)){

    for(j in seq(1:total)){
      prediction_rwc[[j]] = random_weighted_classifier(better/total, worse/total, no_change/total)
    }
    cm = (confusionMatrix(data = factor(prediction_rwc, levels = c("better","worse", "no_change")),
                          reference = factor(reference, levels = c("better","worse", "no_change")),
                          positive = c("better", "worse"),
                          mode = "everything"))

    f1_rwc[i,1:3] <-  cm$byClass[,"F1"]
    pres_rwc[i,1:3] = cm$byClass[,"Precision"]
    rec_rwc[i,1:3] = cm$byClass[,"Recall"]
    kappa_rwc[[i]] = round(cm$overall["Kappa"],2)
  }

  print(list("f1" = c(mean(f1_rwc[,1], na.rm = T),mean(f1_rwc[,2], na.rm = T),mean(f1_rwc[,3], na.rm = T)),
             "precision" = c(mean(pres_rwc[,1], na.rm = T),mean(pres_rwc[,2], na.rm = T),mean(pres_rwc[,3], na.rm = T)),
             "recall" = c(mean(rec_rwc[,1], na.rm = T),mean(rec_rwc[,2], na.rm = T),mean(rec_rwc[,3], na.rm = T)),
             "kappa" = mean(kappa_rwc, na.rm = T)))

}

test()

Some improvements:

random_weighted_classifier2 <- function(n = 1, weightA, weightB, weightC){
  x <- sample(1:100, n, replace = T) / 100
  i1 <- x <= weightA
  i2 <- x > weightA & x <= (weightA + weightB)
  rez <- rep('no_change', n)
  rez[i2] <- "worse"
  rez[i1] <- "better"
  rez
}

test <- function(){

  betters <- rep("better", 100)
  worses <- rep("worse", 50)
  no_changes <- rep("no_change", 10)
  reference <- sample(c(betters, worses, no_changes))

  better <- sum(reference == "better")
  worse <- sum(reference == "worse")
  no_change <- sum(reference == "no_change")
  total <- length(reference)

  iterations <- 1000
  f1_rwc <- pres_rwc <- rec_rwc <- matrix(0., iterations, 3)
  kappa_rwc <- vector("double", iterations)

  referenceF <- factor(reference, levels = c("better","worse", "no_change"))
  for (i in seq(1:iterations)) {

    prediction_rwc <- random_weighted_classifier2(total,
                                                  better/total,
                                                  worse/total,
                                                  no_change/total)
    prediction_rwc <-
      factor(prediction_rwc, levels = c("better","worse", "no_change"))
    conTable <- table(prediction_rwc, referenceF)
    cm <- confusionMatrix(conTable, positive = c("better", "worse"),
                          mode = "everything")

    f1_rwc[i, 1:3] <-  cm$byClass[,"F1"]
    pres_rwc[i, 1:3] <- cm$byClass[,"Precision"]
    rec_rwc[i, 1:3] <- cm$byClass[,"Recall"]
    kappa_rwc[[i]] <- round(cm$overall["Kappa"], 2)
  }

  print(list("f1" = colMeans(f1_rwc, na.rm = T),
             "precision" = colMeans(pres_rwc, na.rm = T),
             "recall" = colMeans(rec_rwc, na.rm = T),
             "kappa" = mean(kappa_rwc, na.rm = T)))
}

It should be around 50 % faster (16.80 vs 11.02 sec for 1k iterations).

You, possibly, could speed it up even more by removing the confusionMatrix function and calculation of all of the necessary parameters by yourself.

Update

Based on confusionMatrix I managed to extract relevant parts of code and wrap into a function:

statistics <- function(data, beta = 1) {
  stat <- sapply(rownames(data), function(relevant) {

    if (nrow(data) > 2) {
      m <- matrix(NA, 2, 2)
      colnames(m) <- rownames(m) <- c("rel", "irrel")
      irrelCol <- which(!(colnames(data) %in% relevant))
      relCol <- which(colnames(data) %in% relevant)
      m[1, 1] <- sum(data[relCol, relCol])
      m[1, 2] <- sum(data[relCol, irrelCol])
      m[2, 1] <- sum(data[irrelCol, relCol])
      m[2, 2] <- sum(data[irrelCol, irrelCol])
      m <- as.table(m)
      relevant <- "rel"
    }
    numer <- m[relevant, relevant]
    denom <- sum(m[relevant, ])
    prec <- ifelse(denom > 0, numer/denom, NA) # Precision

    denom <- sum(m[, relevant])
    rec <- ifelse(denom > 0, numer / denom, NA) # Recall

    F1 <- (1 + beta^2)*prec*rec/((beta^2 * prec) + rec) # F1
    c('Precision' = prec, 'Recall' = rec, 'F1' = F1)
  })

  k <- unlist(e1071::classAgreement(data))["kappa"]
  list(stat, kappa = k)
}

and then test3 looks like:

test3 <- function(iterations = 100){

  vals <- c("better","worse", "no_change")
  betters <- rep("better", 100)
  worses <- rep("worse", 50)
  no_changes <- rep("no_change", 10)
  reference <- sample(c(betters, worses, no_changes))

  better <- sum(reference == "better")
  worse <- sum(reference == "worse")
  no_change <- sum(reference == "no_change")
  n <- length(reference)

  f1_rwc <- pres_rwc <- rec_rwc <- matrix(0., iterations, 3)
  kappa_rwc <- vector("double", iterations)
  referenceF <- factor(reference, levels = vals)

  for (i in seq(1:iterations)) {

    prediction_rwc <-
      random_weighted_classifier2(n, better/n, worse/n, no_change/n)
    prediction_rwc <-  factor(prediction_rwc, levels = vals)
    conTable <- table(prediction_rwc, referenceF)
    cm2 <- statistics(conTable)
    f1_rwc[i, 1:3] <-  cm2[[1]][3, ]
    pres_rwc[i, 1:3] <- cm2[[1]][1, ]
    rec_rwc[i, 1:3] <- cm2[[1]][2, ]
    kappa_rwc[[i]] <- round(cm2[[2]], 2)
  }

  list("f1" = colMeans(f1_rwc, na.rm = T),
       "precision" = colMeans(pres_rwc, na.rm = T),
       "recall" = colMeans(rec_rwc, na.rm = T),
       "kappa" = mean(kappa_rwc, na.rm = T))
}

This should run under 1sec for 1k iterations.

p.s. kappa can be calculated with this:

  n <- sum(data)
  ni <- rowSums(data)
  nj <- colSums(data)
  p0 <- sum(diag(data, names = F))/n
  pc <- sum((ni/n) * (nj/n))
  k <- (p0 - pc)/(1 - pc)

p.s.s. when reducing to those formulas, I stripped the code which was used for testing, so if you change your data format, bugs may appear. I assumed that the setting doesn't change.


Getting list from an Oracle database and storing it in an object I have two admin groups - VCS, and VPN. For each group, I need to get the list of all the people in that group, along with their IDs.

What is the best way to do this?  Could you please suggest if it is good enough?

Also, should I use a map to store the data instead of creating a new object GetAdminInfo?

public class GetAdminInfo
{
private List<String> persons;
private List<String> personIds;

public List<String> getPersonIds() {
    return personIds;
}
public void setPersonIds(List<String> personIds) {
    this.personIds = personIds;
}

public List<String> getPersons() {
    return persons;
}
public void setPersons(List<String> persons) {
    this.persons = persons;
}
}

This is the method I have written in my DAO class.

public static GetAdminInfo fetchPersonIdInfo(String adminGroup) throws SQLException
{
    GetAdminInfo personInfo =null;
    Connection connection = null;
    PreparedStatement statement = null;
    ResultSet rs = null;
    List<String> personId = new ArrayList<String>();
    List<String> persons = new ArrayList<String>();
    try {
        connection = createConnection();
        statement = connection.prepareStatement("SELECT PERSON_ID,PERSONS FROM ADMIN_INFO WHERE ADMINGROUP = ?");
        statement.setString(1, adminGroup);
        rs = statement.executeQuery();
        if(rs.next())
        {
            personInfo = new GetAdminInfo();
            adminGroup = rs.getString(COLUMN_ADMINGROUP)!=null?rs.getString(COLUMN_ADMINGROUP):"";
            personId.add(rs.getString(1));
            persons.add(rs.getString(1));
            personInfo.setDeviceIds(deviceId);
            personInfo.setPersonIds(personId);
        }

    } finally{
        rs.close();
        conn.close();
        statement.close();
    }

    return personInfo;
}

Well my first gut reaction is that I don't like what I see. Here is why though and hopefully that will help you understand why I feel this way. First is with the naming convention. GetAdminInfo is a verb. Classes are nouns. This is true in all of mainstream OO languages. Therefore a more appropriate name would be AdministratorInformation. But even that name is misleading because of the contents. Your class has 2 arrays which of type String. Since you are dealing with a database it would be ok to make a class called AdministratorGroup which has 2 properties Name and Id with appropriate Getters and setters. Then your AdministratorInformation class would be renamed to AdministratorGroupCollection and have only 1 Array of type AdministratorGroup. Now after you put this program down for some time and come back to it you will find it easier to remember what these classes do. A name should always indicate its intent. You would be very angry at me if i had a class called CartesianPoint and instantiated it like this CartesianPoint point = new CartesianPoint("5Mhz"); You'd be angry because I lied to you. I normally don't do this as I leave it up to you to make changes, but I want to show some sort of code. SO here is what your code would look like after I got my hands on it.

AdministratorInfo

public class AdministratorInfo
{
    private String ID;
    private String name;

    public String getID()
    {
        return ID;
    }
    public String getName()
    {
        return name;
    }

    public void setID(String ID)
    {
        this.ID = ID;
    }
    public void setName(String name)
    {
        this.name = name;
    }
}

AdministratorGroupCollection

public class AdministratorGroupCollection
{
    private List<AdministratorInfo> persons;

    public List<AdministratorInfo> getPersons()
    {
        return persons;
    }

    public void setPersons(List<AdministratorInfo> newList)
    {
        persons = newList;
    }
}

With this code you will not have to add more and more Lists to your collection class to add or remove properties. You'll only have to change the AdministratorGroup class to adjust those properties.

Last point I want to point out is the confusing basis for your SQL method. By the name of it I get the impression that you are returning a single user and not a collection of users. but your code says that you are returning a collection of users. Don't let your names and methods lie about their intent. If you only intend to return a single user then return a class that is not a collection. If your intent is to get a group or a collection of things then you will return a list. Otherwise you are just making it hard for others (others can mean you in the future) to know how to use your code. Although I would break this method up some more to make it a little bit easier to read it serves its purpose. This is how I interpreted your code, and how I would have fixed it.

public static AdministratorInfo fetchPersonIdInfo(String adminGroup) throws SQLException
{
    AdministratorInfo personInfo = new AdministratorInfo();
    Connection connection = null;
    PreparedStatement statement = null;
    ResultSet rs = null;
    try
    {
        connection = createConnection();
        statement = connection.prepareStatement("SELECT PERSON_ID,PERSONS FROM ADMIN_INFO WHERE ADMINGROUP = ?");
        statement.setString(1, adminGroup);
        rs = statement.executeQuery();
        if (rs.next())
        {
            //what purpose does it serve to change adminGroup?
            adminGroup = rs.getString(COLUMN_ADMINGROUP) != null ? rs.getString(COLUMN_ADMINGROUP) : "";
            personInfo.setID(rs.getString(1));
            personInfo.setName(rs.getString(2));
            personInfo.setDeviceIds(deviceId); //I don't like not knowing what this is
        }

    }
    finally
    {
        rs.close();
        connection.close();
        statement.close();
    }

    return personInfo;
}


Is this password-submission form valid? Does my code contain anything invalid?

<form name="myForm" id="myForm">

                User ID:
                <input type="text" id="id" />

                Password:
                <input type="password" id="pass" name="pass" />

                <button type="button" class="submit">Submit</button>

<input type="reset" />

</form>

According to the W3C (http://validator.w3.org/)

Your form tag is not valid.
Technically it must have an action attribute:

  required attribute "ACTION" not specified in:
  <form name="myForm" id="myForm">

  The attribute given above is required for an element that you've used, but you have omitted it.

There are a couple of warnings:

  Warning: NET-enabling start-tag requires SHORTTAG YES
  <input type="text" id="id" /   >
                                      ^^^  <!-- auto closing tag -->

  The sequence  can be interpreted in at least two different ways, depending on the DOCTYPE of the document. For HTML 4.01 Strict, the '/' terminates the tag '). However, since many browsers don't interpret it this way, even in the presence of an HTML 4.01 Strict DOCTYPE, it is best to avoid it completely in pure HTML documents and reserve its use solely for those written in XHTML.


Zero pad function trim extra leading zeros Here is a zero pad function I have:

''' <summary>
''' Take a number string and zero pad it to the specified length.
''' </summary>
''' <param name="Number">The string representation of the value that will be zero-padded.</param>
''' <param name="ToLength">The length of the string that will result after the zero-padding.</param>
''' <param name="TrimExtraZeroes">An optional boolean which, when true, indicates that any
''' extra leading zeroes that make the string longer than the desired length should be removed.</param>
''' <returns>A value which is the same as <paramref name="Number" /> but zero padded to the length specified by <paramref name="ToLength" />.</returns>
''' <remarks></remarks>
Friend Function ZeroPad(ByVal Number As String, ByVal ToLength As Integer, Optional ByVal TrimExtraZeroes As Boolean = False) As String
    Const ZeroChar = "0"c

    Select Case Number.Length
        Case Is < ToLength
            Return New String(ZeroChar, ToLength - Number.Length) & Number
        Case Is = ToLength
            Return Number
        Case Is > ToLength
            ZeroPad = Number

            If (TrimExtraZeroes) Then
                Do While ZeroPad.StartsWith(ZeroChar) AndAlso ZeroPad.Length > ToLength
                    ZeroPad = ZeroPad.Substring(1)
                Loop
            End If
        Case Else
            'This case can never occur, but it's here to suppress compiler warning.
            Throw New InvalidOperationException(String.Format("Unknown number length: {0}", Number.Length))
    End Select
End Function

Now, focusing on just one portion of it -- the part that trims leading zeros:

Do While ZeroPad.StartsWith(ZeroChar) AndAlso ZeroPad.Length > ToLength
    ZeroPad = ZeroPad.Substring(1)
Loop

is there a more efficient, cleaner, or "slick" way to do this other than looping?

Naming

Based on the naming guidelines parameters should be named using camelCase casing.

Select Case

If you omitt the Case Is > ToLength and the else part aka having only 2 cases you will have

Friend Function ZeroPad(ByVal Number As String, ByVal ToLength As Integer, Optional ByVal TrimExtraZeroes As Boolean = False) As String
    Const ZeroChar = "0"c

    Select Case Number.Length
        Case Is < ToLength
            Return New String(ZeroChar, ToLength - Number.Length) & Number
        Case Is = ToLength
            Return Number
     End Select

     If (TrimExtraZeroes) Then
         Do While Number.StartsWith(ZeroChar) AndAlso Number.Length > ToLength
             Number = Number.Substring(1)
         Loop
     End If
    Return Number
End Function

Why don't you use the built in String.PadLeft() method which would obsolete the Select Case. In addition to first stripping any 0 of the left side of Number you won't need the loop at all.

This boils down to only this

Friend Function ZeroPad(ByVal Number As String, ByVal ToLength As Integer, Optional ByVal TrimExtraZeroes As Boolean = False) As String
    Const ZeroChar = "0"c
    If (TrimExtraZeroes) Then
        Number = Number.TrimStart({ZeroChar})
    End If
    Return Number.PadLeft(ToLength, ZeroChar)
End Function

Update
Some timetables for calling both methods 1.000.000 times

                                    Yours(ms)  Mine(ms)
ZeroPad("0000000001", 2, False)       227       231
ZeroPad("0000000001", 2, True)       1696       297
ZeroPad("01", 2, False)               251       235
ZeroPad("01", 2, True)                248       263
ZeroPad("01", 12, False)              256       247
ZeroPad("01", 12, True)               257       260


Finding the latest file in a folder I'm a bit new to Python and sort of learning on my own. I wrote a small function to help me find the latest file in a directory. Taking a step back, it reads a bit janky and I was curious what steps or what resources I could look into to help me make this more friendly. Should I be returning False? Or 0?
Inside my example/files directory are 3 files which were, for this example, created on the dates specified in the file name:

example/files/randomtext011.201602012.txt
example/files/randomtext011.201602011.txt
example/files/randomtext011.201602013.txt

import os.path
import glob
import datetime

dir = 'example/files'
file_pattern = 'randomtext011.*.txt'

def get_latest_file(file_pattern,path=None):
    if path is None:
        list_of_files = glob.glob('{0}'.format(file_pattern))
        if len(list_of_files)> 0:
            return os.path.split(max(list_of_files, key = os.path.getctime))[1]
    else:
        list_of_files = glob.glob('{0}/{1}'.format(path, file_pattern))
        if len(list_of_files) > 0:
            return os.path.split(max(list_of_files,key=os.path.getctime))[1]
    return False

First of all, I think that your variable names are quite good.

  Should I be returning False? Or 0?

I would recommend None

Don't repeat yourself

As you can see, the two branches of your if else are very similar.

Instead you could do a

if path is None:
    fullpath = file_pattern
else:
    fullpath = path + '/' + file_pattern

But joining paths like this is not very pythonic (and might cause problems on windows).

Instead, fullpath = os.path.join(path, file_pattern) is what you are looking for.

About the arguments

You can take inspiration of the os.path.join even further and change the order of your arguments (and completely remove the branching):

def get_latest_file(path, *paths):
    fullpath = os.path.join(path, paths)
    ...
get_latest_file('example', 'files','randomtext011.*.txt')

Use docstrings

And then you might think that the way to call it is not trivial and want to document it: let's use a docstring !

def get_latest_file(path, *paths):
    """Returns the name of the latest (most recent) file
    of the joined path(s)"""
    fullpath = os.path.join(path, *paths)

Miscellaneous

If you use Python 3, you can use iglob instead.

For the os.path.split, I prefer using it like this (instead of the 1 index):

folder, filename = os.path.split(latest_file)

The import datetime is not used.

Instead of if len(list_of_files)> 0:, you can simply do if list_of_files:

Revised code

def get_latest_file(path, *paths):
    """Returns the name of the latest (most recent) file
    of the joined path(s)"""
    fullpath = os.path.join(path, *paths)
    list_of_files = glob.glob(fullpath)  # You may use iglob in Python3
    if not list_of_files:                # I prefer using the negation
        return None                      # because it behaves like a shortcut
    latest_file = max(list_of_files, key=os.path.getctime)
    _, filename = os.path.split(latest_file)
    return filename


Constructing expressions that produce integers by only using digits in string '2017' and high school level operators and functions At school, we have a competition where the winner is determined by the length of their unbroken sequence of expressions that evaluate to integers 0 to n.
You must use the next year's number in the correct order, every digit exactly once.

example:

0 = 2*0*17
1 = 2-01^7
2 = 2-0*17
...

The competition doesn't have a section for computer-aided results, but I thought I would try it for fun.

The problem is, I don't know how to do this efficiently.
In Python 3.5, I am using
itertools.product('2017()*/+-',repetitions=a)
where a is an integer 4 to n to gather different lengths of expressions,
and picking the results that evaluate to integers and fit the criteria.
This takes more than O(10^n) time.

Is there a way to do this more efficiently in Python?
If I should change language, what would be relatively easy to write but efficient?

Here is my whole code:

import itertools
import timeit

max_length = 8
min_val = 0
max_val = 100

start_time = timeit.default_timer()

year = '2017'
special_chars = '()*/+-'
these_chars = year + special_chars

def perm(seq, n):
    for p in itertools.product(seq, repeat=n):
        yield p

# Does every character in str_a occur in str_b
# in the right order and exactly once, with arbitrary in-betweens
def right_order(str_a, str_b):
    list_a = list(str_a)
    list_result = []
    for c in str_b:
        if c in str_a:
            list_result.append(c)
    if list_result == list_a:
        return True
    else:
        return False

def strs(length, dictionary):
    for a in perm(these_chars, length):
        cand = 0
        a = ''.join(a)
        try:
            cand = eval(a)
            if int(cand) != cand:
                continue
        except:
            pass

        if right_order(year, a) == True:
            if not any(c in '()' for c in str(cand)) and min_val <= cand <= max_val and '//' not in a: # To remove tuples and non-standard operators
                if cand in dictionary:
                    if len(dictionary[cand]) < 10:
                        dictionary[cand].append(a)
                else:
                    dictionary[cand] = [a]
    return dictionary

result_dict = {}
for i in range(len(year),max_length + 1):
    result_dict = strs(i, result_dict)

for k,v in sorted(result_dict.items()):
    key = str(k) + ': '
    values = "   ".join(v[0:4])

    print('{:<8} {}'.format(key, values),end=' ')

    print('...' * (len(v) > 4))

print()
stop_time = timeit.default_timer()
print('execution time: ' + str(stop_time - start_time))

input('EXIT')

Where your code is currently lacking is using all the knowledge you have. You already know that in the end the digits have to be in order and that in between the digits there can (or can not) be an operator.

This means, we only need to generate all strings of the form s = "{}2{}0{}1{}7{}", where the {} denote a place for an operator.

In addition, we know that certain operators will never make sense at the beginning (like /) and most will not make sense at the end either (basically, only ) makes sense there).

If we use this fact, we can end up with something like:

from collections import defaultdict
from itertools import product
from math import *

allowed_begin = set(["", "+", "-", "sin(", "cos(", "tan(", "exp(", "("])
allowed = allowed_begin | set(
    ["*", "/", "**", "*sin(", "*cos(", "*tan(", "*exp(", "*(", ")"])
allowed_end = set(["", ")"])

def find_expressions():
    expressions = defaultdict(list)
    template = "{}2{}0{}1{}7{}"
    for ops in product(allowed_begin, allowed, allowed, allowed, allowed_end):
        try:
            expression = template.format(*ops)
            n = eval(expression)
            if n >= 0 and n % 1 == 0:
                print n, x
                expressions[n].append(expression)
        except Exception:
            pass

    return expressions

if __name__ == "__main__":
    expressions = find_expressions()
    print sorted(expressions.keys())

This runs in about 0.5s on my machine and finds expressions for
0.0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 12, 13, 14, 15, 16, 17, 18, 19, 20.0, 21, 26, 27, 28, 30, 34, 37, 120, 140, 160, 194, 208, 340, 1407, 2017, 32768, 1.1719142372802612e+16, 13107200000000000000000L, 6.327431707155585e+60, 1.7911398206275708e+84, 2.1540324218248465e+90, 4.572185553551339e+147. They are stored in expressions and all possible expressions for an existing key can be accesses via e.g. expressions[22].

Note that I chose to allow *( as an operator, meaning implicit multiplication (see comment on the question). If you think this is not allowed, just take them out of the allowed operators.

The loop approach has one advantage over itertools.permutations. It allows a == b == c == d. Whereas permutations does not:

>>> list(itertools.permutations([1,2], 2))
[(1,2), (2,1)]

Note the absence of (1,1) and (2,2).


Function to insert a tag into a database I'm developing a MVC structured RESTful API project (thanks to the help of Mārtiņš Tereško for explaining it).

Almost all of my functions look similar to this:

 /**
    * Insert Tag
    */
    public function insertTag(Tag\Tag $tag)
    {
        try{
            //Names
            $name = new Common\Name();

            //Insert into main table
            $sql = "INSERT INTO tag VALUES(null)";
            $statement = $this->connection->prepare($sql);
            $statement->execute();

            //Get main table id
            $tagId = $this->connection->lastInsertId();

            //Insert into audit tag table
            $sql = "INSERT INTO tag_audit(behaviour,action_peformed,state,tag_id) VALUES(:behaviour,'insert','P',:tag_id)";
            $statement = $this->connection->prepare($sql);
            $statement->bindValue(':behaviour',$tag->getBehaviour(),PDO::PARAM_STR);
            $statement->bindValue(':tag_id',$tagId,PDO::PARAM_INT);
            $statement->execute();

            //Insert into names
            $name->setNames($tag->getNames()[0]);
            $values = $name->getBulkValue(
                $name->getValue(),
                ["'insert'","'P'",$tagId]
            );

            $sql = "INSERT INTO tag_name_audit(name,language,action_peformed,state,tag) VALUES".$values;
            $statement = $this->connection->prepare($sql);
            $statement->execute();

            $response = ['status'=>200,'Message'=>'Successfully inserted.'];

        }catch(\Exception $e){

             $sql = "DELETE FROM tag WHERE id_tag = :id_tag";
             $statement = $this->connection->prepare($sql);
             $statement->bindValue(":id_tag",$tagId,PDO::PARAM_INT);
             $statement->execute();

             $sql = "DELETE FROM tag_audit WHERE tag_id = :id_tag";
             $statement = $this->connection->prepare($sql);
             $statement->bindValue(":id_tag",$tagId,PDO::PARAM_INT);
             $statement->execute();

             $response = ['status'=>409,'Message'=>'Error '.$e->getCode()];
        }
        $tag->setResponse($response);
    }

I don't like how much code there is and i tought of having some mapper helper classes which will handle at least a portion of it.But still i don't think that it's the right solution.

Also since i parse the bulk value over an array i know that it's not secure ....how could i solve that part?

So the question is how could i improve it?

Use a transaction, so you won't need all the manual delete queries
Use shorter PDO syntax.
Log errors
Use prepared statements

In the end it should be like

public function insertTag(Tag\Tag $tag)
{
    try{
        //Names
        $name = new Common\Name();

        $pdo->beginTransaction();

        //Insert into main table
        $this->connection->query("INSERT INTO tag VALUES(null)");

        //Get main table id
        $tagId = $this->connection->lastInsertId();

        //Insert into audit tag table
        $sql = "INSERT INTO tag_audit(behaviour,action_peformed,state,tag_id) VALUES(?,'insert','P',?)";
        $this->connection->prepare($sql)->execute([$tag->getBehaviour(), $tagId]);

        //Insert into names
        // you should be using prepared statements here
        $sql = "INSERT INTO tag_name_audit(name,language,action_peformed,state,tag) VALUES (?,?,?,?,?)";
        $statement = $this->connection->prepare($sql);
        foreach (whatever function supplies the name as $name) {
            $statement->execute([$name]);
        }
        $response = ['status'=>200,'Message'=>'Successfully inserted.'];

        $pdo->commit();
    }catch(\Exception $e){

         $pdo->rollback();
         error_log($e);
         $response = ['status'=>409,'Message'=>'Error '.$e->getCode()];
    }
    $tag->setResponse($response);
}


PDO insert data through transaction method I have an import books function to insert more than 5000 rows at once, I did some research and did this one but some other places I saw a different kind of methods to insert without looping and execute this. is my code got any trouble with the performance of app

public function importBooks($data, $no_of_rows)
{
    try {
        // Start transaction
        $this->db->beginTransaction();

        // Init query
        $this->db->query('INSERT INTO books_pre (title, author, publication, container, tmp_no, created_by, created_at) VALUES (:title, :author, :publication, :container, :tmp_no, :created_by, now())');

        for ($i=2; $i<=$no_of_rows; $i++) {
            // Bind values
            $this->db->bind(':title', $data[$i]['B']);
            $this->db->bind(':author', $data[$i]['C']);
            $this->db->bind(':publication', $data[$i]['D']);
            $this->db->bind(':container', $data[$i]['E']);
            $this->db->bind(':tmp_no', $data[$i]['A']);
            $this->db->bind(':created_by', $_SESSION['user_id']);

            // Execute query
            if (!$this->db->execute()) {
                return false;
            }
        }
        // Commit data
        $this->db->commitTransaction();
    } catch (Exception $e) {
        $this->db->cancelTransaction();
        echo "Failed: " . $e->getMessage();
    }
}

Your code is all right from the performance point of view.

But again, you are doing the double work with your homegrown wrapper. For the life of me I won't understand a wrapper that makes your life harder.

So again, a code with vanilla PDO instance in $this->db

$stmt = $this->db->prepare('INSERT INTO books_pre
    (title, author, publication, container, tmp_no, created_by, created_at)
    VALUES (:B, :C, :D, :E, :A, :user_id, now())');

try {
    $this->db->beginTransaction();
    foreach(data as $i => $row) {
        // skip the first two lines
        if ($i == 0 or $i == 1) {
            continue;
        }
        $row['user_id'] = $_SESSION['user_id'];
        $stmt->execute($row);
    }
    $this->db->commit();
} catch (Exception $e) {
    $this->db->rollback();
    throw new $e;
}

Given you already have an associative array we can use its keys as placeholder names.

PDO's execute() method can accept an associative array with data, for example

$stmt = $pdo->prepare("SELECT * FROM t WHERE id=:id");
$stmt->execute(['id'] => $id);

As you can see, we are sending an array directly into execute(), which  keys that must match placeholder names. Given your array already have keys, I just used them as placeholder names in the query.

However, it'a a matter of taste and you can keep use your bind() calls instead.

An important note, there should be no such thing like echo "Failed: " in your code. Always just throw an error and let it be echoed by PHP.

On a side note, a comment like this

    // Start transaction
    $this->db->beginTransaction();

makes very little sense. Do not write comments that just duplicate a code below. Use comments to explain something not that obvious.


Basic myVector class raw vs smart pointer I'm writing a myVector class for a class I'm taking.  I don't believe I'm "strong" with smart pointers right now, so that's my question.  This is just an excerpt from a much larger class that I've completed.

Is there a way to use the shared pointer and dynamically allocate the size, so that I can use the [] notation with that pointer?  I've been looking on Stack Overflow and don't see anything referencing this particular usage.  My next question/conclusion would then be that it's best to stick with raw pointers in the case where I want to use array notation.

#include <iostream>
//#include <memory>

class myVector
{
private:
    int sz;
    int * array;

public:
    myVector(int a, int b)
    {
        sz = a;
        array = new int[sz];
        for(int i(0); i < sz; ++i)
            array[i] = b;
    }
    ~myVector(){ delete [] array; };
    int size() const
    {
        return sz;
    }
    int &operator[](const int num)
    {
        return array[num];
    }
};

int main()
{
    myVector testing(10, 6);
    for(int i(0); i < testing.size(); ++i)
    {
        testing[i] = testing[i] * i;
        std::cout << testing[i] << std::endl;
    }
    return 0;
}

In the following, I tried to collect the options you have for your custom vector class, and summarize their pros and cons. But before that I'd suggest to use generic programming when you write your vector class, i.e.

template<typename T>
class myVector
{
     size_t sz;
     T* array;

     //all the rest
};

Note two things here: Instead of the int *, I've wrote T*, where T is the type you specify in the program via myVector<int>, say. This you can hold elements of arbitrary type and you do not need to copy the whole class once you need a double type.

Further, I've used the type size_t for the size of the vector, which is the type often used in C++ to denote sizes of objects (in fact, it's just a large unsigned int type).

Choosing the right storage type:

First, and most naturally, you can use an STL container inside, preferably std::vector: this is one example of the RAII way to do it, which basically means that you usually don't have to care for the memory management by yourself. Although from the first it sounds a bit redundant to include a std::vector in your custom vector class, it actually can be rather useful, for instance when you want to add arithmetics to your vector.

template<typename T>
class myVector
{
     std::vector<T> array;
};

This has several advantages: visibly, you don't need a variable sz anymore, as the std::vector already manages the size. Next -- and this is an important thing -- as your array becomes correctly copied, moved, assigned and destructed, your class will perform these operations correctly as well.

In the following, let us consider the copy operation as an example: in the above approach, when one copies the class, the new class contains a copy of the original std::vector.
Using a raw pointer as in your OP. This is nowadays a discouraged way to do it, just because you have to manually care for the memory management. Once you invoke array = new int[sz], array is a pointer to some allocated memory on the heap, i.e. it's so to say something outside of the class, and the class itself must take care that this memory is correctly released for further usage when it is destructed. Thus you need to write a destructor, and therefore -- according to the rule of three(five) -- also copy constructors, assignment operators (, move constructors).

Consider again the copy process. The copied myVector just copies the raw pointer, such that now two pointers point to the same memory region. However, these two pointers don't know about each other. So, it becomes a hard task to prevent one of the classes to delete the memory when it goes out of scope, as then the other class will become a dangling pointer: it points to a destructed and thus invalid object. In order to avoid this, you'd need to count how many pointers point to a given object, and call the delete only if the last pointer goes out of scope (i.e. when nobody needs the object anymore).
Shared pointers exactly do this: they count how many pointers point to a given object which has been allocated on the heap.

template<typename T>
class myVector
{
     std::shared_ptr<T> array;
     // construct via array= std::make_shared( new T[sz], std::default_delete<T[]>() );
     // this is necessary so that "delete[]" is called, an not "delete".

     //or better
     std::shared_ptr<std::vector<T> > array;
};

By using them, you get almost the same behaviour as by using a raw pointer but have a lot of advantages. Most importantly, you again are on the RAII track, you don't need to care for all the default functions and let the compiler generate them. A disadvantage is that a shared_ptr is usually less performant than a raw pointer, but this is often taken into account in favor of a correct code.

Regarding the example, copying works fine: the newly copied class only contains the pointer to the object and not the object itself, which is an advantage when the arrays are large and do not need to be changed in the copy. In the copy process, the shared_ptr increases its so-called reference count, such that it knows when exactly to call delete.
Finally, there is another smart pointer, namely std::unique_ptr. It addresses the above problem in a completely different way: it simply doesn't allow copying, so that there is always one pointer which does the memory management. It's also said that the unique_ptr owns the object. Other, non-owning pointers are allowed, but these should never do any memory-related operations (--never call delete).

It is possible to use it here and it could also be reasonable, but be aware that your class won't possess automatically generated copy and assignment operators. This is probably not intended.

Summary: as written above, I'd use the std::vector<T> alternative, and depart from it only for a good reason. Only then, I'd use the shared_ptr alternative. I would never use the raw pointer array.


Identifying most recent blogXX.php file I create blog posts as blog1.php...blog20.php...blog500.php etc and redirect users from blog.php (code below) to the most recent blog post as determined by the blog post with the highest number in the filename.

Here's how I'm doing it, but I have the sense this could be much cleaner and more elegant:

$directory = "/var/www/html";
$scanned_directory = array_diff(scandir($directory), array('..', '.'));

$blogNumbers = Array();
foreach($scanned_directory as $file){
        if(preg_match("/blog(?<digit>[0-9]+)/", $file, $matches)){
                foreach($matches as $match){
                                 if(is_numeric($match["digit"])) array_push($blogNumbers, $match["digit"]);
                                     //had to put this numeric check in because otherwise
                                     //script was pushing some 'b's onto array
                                 }
        }

}

$currentBlog = max($blogNumbers);
header('Location: http://www.sunnysideworks.nyc/Simply-Run/blog'.$currentBlog);
die();

People are always complaining that bad programming is too easy in PHP, and I suspect what I've posted is just that easy bad programming that makes people mad. Can someone tell me how to make this more PHP-like and more elegant? I've worked much more in Python, but I'd like to be less of a hack in PHP.

One thing that makes me cringe is the indentation. It really is off.
I had to look 20 times to that stray brace to realize it isn't closing that if(in_numeric()).

Your casing is inconsistent. You have array() and Array(). Pease, use array() only.

Same goes for $scanned_directory, $blogNumbers, $scanned_directory and $currentBlog.
Pick one style and use it!

Same goes with quotes. You have single-quoted and double-quotes strings everywhere!

Just pick 1 style and use it! (My recommendation goes to single-quotes.)

You have the following line:

if(is_numeric($match["digit"])) array_push($blogNumbers, $match["digit"]);

You should always use braces. Always! There have been serious bugs on iOS for this.

Why do you have a die(); at the end? As soon as the script finishes, it will just exit. No need to have the die(); there.

You have the following line:

$directory = "/var/www/html";

That sounds like a constant. Try something like this:

define('DIRECTORY', '/var/www/html');

Or:

const DIRECTORY = '/var/www/html';

And use it like this:

$scanned_directory = array_diff(scandir(DIRECTORY), array('..', '.'));

Almost at the end, you have this:

$currentBlog = max($blogNumbers);
header('Location: http://www.sunnysideworks.nyc/Simply-Run/blog'.$currentBlog);

After you fix your variables' casing, you can just do like this:

header('Location: http://www.sunnysideworks.nyc/Simply-Run/blog' . max($blog_numbers));

How I would write this:

define('DIRECTORY', '/var/www/html');

$scanned_directory = array_diff(scandir(DIRECTORY), array('..', '.'));

$blog_numbers = array();
foreach($scanned_directory as $file) {
    if(preg_match('/blog(?<digit>[0-9]+)/', $file, $matches)) {
        foreach($matches as $match) {
            //had to put this numeric check in because otherwise
            //script was pushing some 'b's onto array
            if(is_numeric($match['digit'])) {
                array_push($blog_numbers, $match['digit']);
            }
        }
    }
}

header('Location: http://www.sunnysideworks.nyc/Simply-Run/blog' . max($blog_numbers));

Outside this review, that foreach doesn't seem necessary. But I don't know the actual names.


Compare 2 arrays and preserve the row number Background:

You have an eCommerce order of 10 products but you want to refund only 5 of them. The stock managing system needs to preserve the order of row number (orderline) when you do the returns, but don't send the items that are not refunded.

The order looks like this:

"Poduct1" Orderline ="1"
"Poduct2" Orderline ="2"
"Poduct3" Orderline ="3"
"Poduct4" Orderline ="4"

Refund all except Product2 and Product3 becomes:

"Poduct1" Orderline ="1"
"Poduct4" Orderline ="4"

But the creditmemo object returns it like this:

"Poduct1" Orderline ="1"
"Poduct4" Orderline ="2"

Because there is nothing in the refunded object about non-refunded items, my strategy is to compare the refund object with the order one and spot the difference (empty place).

Get order items and build and array of all SKUs:

$items = $products->getAllVisibleItems();
$all_skus = array();

    foreach ($items as $key => $item) {

        $all_sku = $item->getData("sku");
        $all_skus[] = $all_sku;

    }

Get refunded items and build an array of all SKUs:

    $all_memo_skus = array();
    foreach ($creditmemo->getAllItems() as $key => $item) {

        $memo_sku = $item->getData('sku');
        $all_memo_skus[] = $memo_sku;

    }

Check the difference between them:

$memo_diff = array_diff($all_skus, $all_memo_skus);

returns keys and SKUs. But I'm interested only in the key, the position where there is an empty place:

$memo_diff = array_keys($memo_diff);

because the array starts at 0, but the Orderline rows have to begin with 1, add 1 to each item of the array:

        $memo_increase = array();
        foreach($memo_diff as $val){
            $memo_increase[] = $val + 1;
        }

If there are consecutive numbers group them, for example if rows 2,3,4,5 are not refunded, you want the Orderline to jump to 6.

Here I am grouping the consecutive rows:

$result  = array();
        $temp = array();
        foreach($memo_increase as $val) {
            if(next($memo_increase) == ($val + 1)){
                $temp[] = $val;
            }
            else{
                if(count($temp) > 0) {
                    $temp[] = $val;
                    $result[]  = $temp[0].'-'.end($temp);
                    $temp   = array();
                }
                else{
                    $result[] = $val;
                }
            }
        }

This will return something like array("1", "4-9", "12") if rows 1,4,5,6,7,8,9,12 are missing.

//start array key from 1 instead of 0

array_unshift($result,"");
        unset($result[0]);

Then loop through each refunded item and assign it the right Orderline:

    $i = 1;

    foreach ($creditmemo->getAllItems() as $key => $item) {

            //if item is inside a grouped element, i.e 2-7 put the orderline at 2 then the next one at 8
            foreach($result as $val){
                if(strpos($val,"-")){
                    $piece = explode("-", $val);
                    if(($piece[0]) == $i){
                        $orderline = $piece[0];
                        $i = $piece[1] + 1;

                    }
                }
            }
//if item is single
            if(in_array($i, $result)){
                $orderline = $i+1;
                $i++;
            }
            else{
                $orderline = $i;
            }

It is overly complex for such a simple task. Is there a better way?

The stock managing system needs to preserve the order of row number (orderline) when you do the returns, but don't send the items that are not refunded.

If that's the requirement, then you should store the row number in the database at the time that the order is placed.  The problems that you are having come from trying to rebuild this information later.  If you want something to be persistent, store it.  You would only want to build it on the fly if you want it to change with other changes.  But in this case, that's explicitly not what you want.

This would be easier to answer if you included more code.  For example, how do you write to your database?  I'm suggesting a change in that.  How do you read from your database?  I'm suggesting that you read another column.  How do you build your data structures?  They might change as well.

In the following:

    foreach ($creditmemo->getAllItems() as $key => $item) {

            //if item is inside a grouped element, i.e 2-7 put the orderline at 2 then the next one at 8
            foreach($result as $val){
                if(strpos($val,"-")){
                    $piece = explode("-", $val);
                    if(($piece[0]) == $i){
                        $orderline = $piece[0];
                        $i = $piece[1] + 1;

                    }
                }
            }
//if item is single
            if(in_array($i, $result)){
                $orderline = $i+1;
                $i++;
            }
            else{
                $orderline = $i;
            }

You could just say

    foreach ($creditmemo->getAllItems as $item) {
        $orderline = $item->getOrderLine();

Of course, this adds additional work when inserting into the database, but since you don't provide that, we can't help you make changes to it.  Nor can we help with the changes to the $item object, as we don't see how it's built.


Function that takes an object and a query obj that tries to regexp matches its values I have the follow matcher method that should do:

obj = {eins: 'one', zwei: 'two'}
queryA = {eins: 'one'}
queryB = {eins: 'one', zwei: 'deux'}

matcher(obj, queryA) // true
matcher(obj, queryB) // false

const matcher = curryRight(
  (state, query) =>
    flowRight([
      every(Boolean),
      map(
        (val, key) => {
          return  state[key] && state[key].match(new RegExp(val, 'ig'))
        }
      )
    ])(query)
)

Wanted:
Because I plan on using this function, matcher() in filter(), it should take a query and returns a function which can take an object for equality check against that query. My version above does that, the improvement sought is can we extract the

(val, key) => {
  return  state[key] && state[key].match(new RegExp(val, 'ig'))
 }

out so that I can in the future swap it out for some function which returns a boolean value?

Is there any way that the iteratee inside map may be split into two such that state is passed in separately? That way, I can refactor the .match(...) part out as a standalone function.

I have reviewed your code and I can make the following remarks:

FP concepts

I understand that you want to solve the issue of matching objects to queries. If you pass an object and a query you should be able to check if the query satisfies all parts or no. However to be honest with you you try too much in the wrong direction. All that curryRight, flowRight, partial etc are very interesting and useful but in the average programmer they don't really mean anything for the problem you are trying to tackle.

For example I was able to rewrite the code using lodash chain in 5 minutes and with better readability. I will post my code at the end but before that I'll comment a few things:

matcher(obj, queryA): Why the name matcher? What does it matches? An object, an array? I think its too generic and confusing. And why does it accepts 2 parameters? Ideally it should accept only one at the time.It should have been like makeMatcher(obj) => (query) => ... and now you would have more flexibility with naming.

const matcher = curryRight(
  (state, query) =>
    flowRight([
      every(Boolean),
      map(
        (val, key) => {
          return  state[key] && state[key].match(new RegExp(val, 'ig'))
        }
      )
    ])(query)

This looks too esoteric and I had to search for the documentation for both flowRight and curryRight and after that I had to run your code to the browser with breakpoints to understand the internals etc. At some point I stopped because I thought it took me enough time already and its not worth it. Its clearly too complicated to read and to understand, not only now but also in 2 years time when you revisit your code and think what exactly did you meant to do. My point is that in the current state is un-maintainable.

Anyway here is my take on this:

const makeMatcher = (obj) => (query) => {
  const matchesQuery = (val, key) => {
    return obj[key] && obj[key].includes(query[key]);
  };

  return _(query)
          .chain()
          .map(matchesQuery)
          .every(Boolean)
          .value();
}

const objMatches = makeMatcher(obj);
objMatches(queryA); // True
objMatches(queryB); // False

Note how the code speaks of its self and it doesn't have to exaggerate. Every name has a meaning with the right intent. A makeMatcher makes matchers and an objMatches returns a boolean by accepting a query and checking if its matches the query.

Inside the makeMatcher we simply chain over the query and validate that either every query parameter matches the object or in case of a mismatch we return false and that's it.

My point is that you should always have a birds eye view of your code and try to think of ways to make your work easier to handle. Search for alternatives and don't fall into the rabbit hole of applying things that they don't fit accurately. There is always a simpler method to do your job. Good luck with your FP endeavors.


Implementing a generic DropDownList attribute and templating solution My goal was to be able to render my view models using a standard Object.cshtml editor and display template. In order to do so I needed to be able to always call Html.Editor(propertyName) or Html.Display(propertyName) in order to render the HTML element.

There were plenty of resources for creating custom attributes and custom editor templates. However I ran into a wall when creating the DropDownListAttribute and template since I needed to obtain the values for the select list as well as set the selected item based on the current instance of the view model.

Here is the reference I used to create the custom ModelMetadataProvider and custom attributes.

Here is the problem I ran into with getting the current instance of the view model.

It turns out using reflection here to access the view model is not safe. Depending on how you iterate over the property metadata the view model can sometimes be null.

Here is my implementation using the resource referenced above. This is a simple version that only uses a List<string> for the DropDownList. Using this example it would be trivial to use dictionaries and set the text/value properties:

View Model:

public class PhoneNumberViewModel
{
    // ... normal properties and attributes
    [DropDownList("ContactTypes")]  // <-- This is the important one!
    public string ContactType { get; set; }
    public static IEnumerable<string> ContactTypes
    {
        get
        {
            return new List<string>()
            {
                "Home", "Work", "Mobile"
            }
        }
    }
}

DropDownListAttribute Implementation

public class DropDownListAttribute : MetadataAttribute
{
    private string _items;

    public DropDownListAttribute(string items)
    {
        _items = items;
    }

    public override void Process(System.Web.Mvc.ModelMetadata modelMetaData)
    {
        modelMetaData.AdditionalValues.Add("Items", _items);
        modelMetaData.TemplateHint = TemplateHints.DropDownList; // Just a const string
    }
}

Custom HTML Helper used to render elements that need access to the view model instance

@Html.ContextualEditor(propertyName)

ContextualEditor HTML Helper Implementation

public static MvcHtmlString ContextualEditor(this HtmlHelper html, string expression)
{
    var model = html.ViewData.Model;
    var propertyMetadata = html.ViewData.ModelMetadata.Properties.Where(x => x.PropertyName == expression).Single();
    switch (propertyMetadata.TemplateHint)
    {
        case TemplateHints.DropDownList:
            var items = (IEnumerable<string>)model.GetType().GetProperty((string)propertyMetadata.AdditionalValues["Items"]).GetValue(model, null);
            var selectedValue = (string)model.GetType().GetProperty(expression).GetValue(model, null);
            var selectList = new TagBuilder("select");
            selectList.MergeAttribute("id", expression);
            selectList.MergeAttribute("name", expression);
            var options = new StringBuilder();
            foreach (var item in items)
            {
                var option = new TagBuilder("option");
                option.InnerHtml = item;
                if (item == selectedValue)
                    option.MergeAttribute("selected", "selected");
                options.Append(option.ToString());
            }
            selectList.InnerHtml = options.ToString();
            return MvcHtmlString.Create(selectList.ToString());
        default:
            return System.Web.Mvc.Html.EditorExtensions.Editor(html, expression);
    }
}

Please let me know what you think of the implementation. I'd be interested to know if anyone has found a more elegant way of achieving this result.

Why not just do this?

public class PhoneNumberViewModel
{
    // ... normal properties and attributes
    [DropDownList("ContactTypes")]  // <-- This is the important one!
    public string ContactType { get; set; }

    public static IEnumerable<SelectListItem> ContactTypes
    {
        get { return new[] { "Home", "Work", "Mobile" }.ToSelectList(); }
    }
}

public static class ExtensionsForSelectListItem
{
    public static IEnumerable<SelectListItem> ToSelectList(this IEnumerable<string> items)
    {
        return items.Select(x => new SelectListItem { Value = x });
    }

    public static MvcHtmlString ContextualEditor(this HtmlHelper html, string expression)
    {

        var model = html.ViewData.Model;

        var propertyMetadata = ModelMetadata.FromStringExpression(expression, html.ViewData);

        switch (propertyMetadata.TemplateHint)
        {
            case TemplateHints.DropDownList:
                var items = (IEnumerable<SelectListItem>)model.GetType().GetProperty((string)propertyMetadata.AdditionalValues["Items"]).GetValue(model, null);
                return html.DropDownList(expression, items);
            default:
                return System.Web.Mvc.Html.EditorExtensions.Editor(html, expression);
        }
    }

}

I didn't test the code to get the items.  But I'm assuming that you had that part working.

I also authored my own HTML Helper Library if you want check out some other examples.

https://bitbucket.org/grcodemonkey/buildmvc


Double LinkedList Deep Copy in Kotlin Goal

Return a deep copy of a double LinkedList.
Each node also contains an additional random pointer, potentially to any node or null.

Code to start
data class Node<T>(
    var data: T?,
    var previous: Node<T>? = null,
    var next: Node<T>? = null,
    var random: Node<T>? = null

class LinkedList {
    // TODO: Implement deep copy here.
}

Implement
fun main() {
    // Setup
    val node1 = Node(1)
    val node2 = Node(2)
    val node3 = Node(3)
    node1.next = node2
    node1.random = node3
    node2.previous = node1
    node2.next = node3
    node3.previous = node2
    node3.random = node1

    val linkedList = LinkedList()
    val deepCopy = linkedList.nextDeepCopy(node1)
    // LinkedList data changed.
    node1.data = 101
    node2.data = 202
    node3.data = 303
    val shallowCopy = linkedList.shallowCopy(node1)
    println("Deep copy made")
    linkedList.print(deepCopy)
    println()
    println("Shallow copy made")
    linkedList.print(shallowCopy)
}

data class Node<T>(
        var data: T?,
        var previous: Node<T>? = null,
        var next: Node<T>? = null,
        var random: Node<T>? = null
)

class LinkedList {
    fun <T> nextDeepCopy(node: Node<T>?): Node<T>? {
        if (node != null) {
            return Node(
                    data = node.data,
                    previous = newDeepCopy(node.previous),
                    next = nextDeepCopy(node.next),
                    random = newDeepCopy(node.random)
            )
        } else return null
    }

    fun <T> newDeepCopy(node: Node<T>?): Node<T>? {
        if (node != null) {
            return Node(
                    data = node.data,
                    previous = node.previous,
                    next = node.next,
                    random = node.random
            )
        } else return null
    }

    fun <T> shallowCopy(node: Node<T>?) = node!!.copy()

    fun <T> print(node: Node<T>?){
        if (node != null){
            println("Node data:${node.data} previous:${node.previous?.data} next:${node.next?.data} random:${node.random?.data}")
            print(node.next)
        }
    }
}

Misplaced Responsibility: print()
The class LinkedList (or data class Node, see below) should not print() functions.
Calling print() functions is a separate responsibility and should be done elsewhere in the code:
What if you want your program to support different output formats like JSON or XML and they shall be sent over the network?
Of course, we don't prepare software for all what-ifs.
But we do make the obvious "cuts" between responsibilities.
To get a printable representation, extract the text used by the print() calls into a toString() method.
Then call print(node) from main().
Avoid Feature Envy (from LinkedList on Node)
Feature Envy is a special type of Misplaced Responsibility.
The class LinkedList doesn't have any features of its own, and no state.
Instead, it only operates on everything in the data class Node.
This is a design smell that we call Feature Envy.
Move all the methods from the class LinkedList to the data class Node.
Then remove the class LinkedList.
You will notice that when you fix Feature Envy problems, the lines become shorter: References that have to wander around because of the Feature Envy become the omittable this.
There are situations where this type of Feature Envy is justified:
In cases of specific design patterns like Proxy, Delegate, Facade.
The class LinkedList does not qualify as a Facade because it exposes the type Node<T>.
It would only qualify if its type Node<T> would be hidden from the user, and the only other type the user sees were <T>.
The sitaution would, of course, change once you give class LinkedList fields head and tail.
Avoid error-prone interfaces
The current interface is error-prone.
It allows for broken linked lists.
The fields next and previous should be read-only for the user of Node.
Instead, the user of Node should have to go through a method like insertAfter(), insertBefore(), delete().
After all, insertion and deletion are not atomic operations but should be transactions.
As a next step, you could ponder about the thread-safety of these operations.
Avoid exposing implementation detail
That a LinkedList is implemented with Nodes is an implementation detail that the user doesn't need to know.
The user should be able to focus on the primary purposes of LinkedLists:

Payload (data)
Iteration/Traversal
Insertion and Removal
Your interface is too low-level.

The perfect LinkedList interface is (almost?) indistiguishable from an ArrayList interface. It should be possible to swap one implementation, like LinkedList, for another, like ArrayList, due to performance considerations (O(1) random access for ArrayList, O(n/2) random access for LinkedList vs O(1) insert/delete for LinkedList, O(n/2) insert/delete for ArrayList) without having to change all the code that uses the list.
Null-Checking
Only use Type? if you really need to support nullability.
I recommend to use Type instead of Type? whereever you can.
That the Kotlin compiler can enforce NonNull is one of the many strengths that it has over Java.
Don't squander it by making everything nullable with ?.
BUG: fun nextDeepCopy() doesn't create a deep copy
It doesn't, because it doesn't recurse to itself but calls newDeepCopy(), and that is not recursive.
The correct way to create a deep copy of a LinkedList with additional random pointers would be to

Create a map with the old nodes as key and the new node as value.
Loop over the map, setting the pointers of the new nodes by a lookup in the map.
Traditional ways of copying a LinkedList won't work because of the random pointer.

Also, fun newDeepCopy() does exactly the same thing as the built-in fun copy(), it creates a shallow copy of the current object.
Unit Tests?
There are ways how to prevent bugs like the one above.
I recommend writing unit tests.
Even better, use Test-Driven Development.
Use if-expressions
In Kotlin, if is an expression.
You can make use of it.
The code:
if (condition) return a else return b

can also be written as, more idiomatic:
return if (condition) a else b

This would allow for all of your functions to become expression functions.
Piece of Sample Code
Here's a code snippet to show how your code could look like:
data class Node<T>(
        var data: T?,
        var previous: Node<T>? = null,
        var next: Node<T>? = null,
        var random: Node<T>? = null
) {
    fun shallowCopy() = copy() // You could even omit this
    fun deepCopy(): Node<T> = TODO("Implement this")
    fun toString() = "Node data:${data} previous:${previous?.data} next:${next?.data} random:${random?.data}"
}


Making backtracking Sudoku solver more functional I'm implementing the backtracking solving algorithm for a Sudoku in F#.  I'm wondering if I could make my code better respect the functional programming paradigm or even just making it simpler/better.

let solve originalSudoku =
    let sudoku = Array2D.copy originalSudoku

    let isValidNumber (x,y) n =
        let square =
            let (x1,y1) = (x/3)*3,(y/3)*3
            [| for i in x1..(x1+2) do for j in y1..(y1+2)->(i,j) |]
            |> Array.forall(fun (i,j)->sudoku.[i,j]<>n)
        let line =
            [|0..8|] |> Array.forall(fun i->x=i || sudoku.[i,y]<>n)

        let column =
            [|0..8|] |> Array.forall(fun i->y=i || sudoku.[x,i]<>n)

        line && column && square

    let rec isGridValid position=
        let x,y = position/9, position%9
        if position = 9*9 then
            true
        else if originalSudoku.[x,y] <> 0 then
            isGridValid(position+1)
        else
            let isValid = isValidNumber (x,y)
            let testCurrentCase n =
                if isValid n then
                    sudoku.[x,y] <- n
                    isGridValid(position+1)
                else
                    false
            if ( not([|0..9|] |> Array.exists(testCurrentCase))) then
                sudoku.[x,y] <- 0
                false
            else true

    isGridValid 0 |> ignore

    sudoku

A purely functional solution would be to get rid of the mutable array and instead use an immutable type like Map<int * int, int>. Your recursive backtracking function would then fill it and return it, instead of modifying a global array. Something like:

let rec solvePosition position sudoku =
    let x,y = position/9, position%9
    if position = 9*9 then
        Some sudoku
    else if Map.containsKey (x,y) sudoku then
        solvePosition (position+1) sudoku
    else
        let isValid n = isValidNumber (x,y) n sudoku
        let solveCurrentCase n =
            if isValid n then
                let newSudoku = Map.add (x, y) n sudoku
                solvePosition (position+1) newSudoku
            else
                None
         Array.tryPick solveCurrentCase [|1..9|]


Using Reflection to Initialize Instance Variables I'm facing the issue of how to reconstruct data across a network in Java.  As part of my Component based game engine (read about it here, I have to encode components as XML on the server, send it to the client, and then reconstruct the appropriate component with all the data included in the XML.  For example:

package.TextureComponent
id 1 /id
width 100 /width
height 100 /height
/package.TextureComponent

Right now, I use reflection on the client to construct a new TextureComponent (but this will work for any component with primitive data types) using the complete name of the class.  I then sequentially read all the String parameters from the XML into the component using reflection in this BasicDecoder:

public class BasicDecoder implements DataDecoder {

    @Override
    public void decode(DataComponent component, String[] data) {
        Class<? extends DataComponent> compClass = component.getClass();

        // fills in all public fields with data in order
        Field[] fields = compClass.getFields();
        for (int i = 0; i < fields.length; i++) {
            Field field = fields[i];
            Class fieldType = field.getType();
            String typeName = fieldType.getName().toUpperCase().replace(".", "");
            FieldType type;
            try {
                type = FieldType.valueOf(typeName);
            } catch (Exception ex) {
                throw new RuntimeException("BasicDecoder cannot handle field: " + typeName);
            }
            try {
                switch (type) {
                case INT:
                    field.setInt(component, Integer.parseInt(data[i]));
                    break;
                case DOUBLE:
                    field.setDouble(component, Double.parseDouble(data[i]));
                    break;
                case BYTE:
                    field.setByte(component, Byte.parseByte(data[i]));
                    break;
                case SHORT:
                    field.setShort(component, Short.parseShort(data[i]));
                    break;
                case LONG:
                    field.setLong(component, Long.parseLong(data[i]));
                    break;
                case FLOAT:
                    field.setFloat(component, Float.parseFloat(data[i]));
                    break;
                case BOOLEAN:
                    field.setBoolean(component, Boolean.parseBoolean(data[i]));
                    break;
                case CHAR:
                    field.setChar(component, data[i].charAt(0));
                    break;
                case JAVALANGSTRING:
                    field.set(component, data[i]);
                    break;
                }
            } catch (Exception ex) {
                ex.printStackTrace();
            }
        }
    }

    private enum FieldType {
        INT, DOUBLE, BYTE, SHORT, LONG, FLOAT, BOOLEAN, CHAR, JAVALANGSTRING
    }
}

Is there any way I can improve this code?  Is reflection the wrong way to go?  I shied away from having each Component define its own decoder because I felt that it would result in a lot of classes which look more or less similar.

Reflection is not the correct way to do this. You will need Serialization for this. Another option is to use XML marshalling and unmarshalling, but I am not so familiar with that.

For serialization I suggest you to pick up a copy of Effective Java, and read the last couple of items of the book, which are all about Serialization.

You really need to take care of the following, since you are dealing with a game and network:

Check your class invariants, constructors may not violate them.
Make sure that your seriliazation is all safe as attackers can exploit a lot of things to do some quite surprising things.
Take care of the evolvability of your classes, serialization does pose some issues on that.

I would recommend all your classes to implement a static inner classes called ProxyHelper and implement the Proxy Serialization Pattern on the classes.

Some other ad-hoc review points of your current code:

Do not catch Exception, catch the subclass(es) that you actually see happening, currently it will also catch RuntimeExceptions.
Do preferably not throw RuntimeException, but add somewhat more detail, IllegalStateException may be a candidate here.
Follow the coding conventions, you do it quite well in general, but in the switch (type) { } every case (and inherited code lines) need to be indented one more factor.

In general I would never let Reflection do such a task, I would add with the following code, either a public constructor taking all arguments of the object (which you can get from the String[] data), or if there are too many arguments (more than five usually), you can also emply the Builder Pattern.


Constructors - lots of paramters or none and use of set methods In your opinion what is the best way to construct an object given the two examples below:

Option 1: Lots of parameters

private string firstname;
private string surname;
private Address homeAddress;
private PhoneNumber homeNumber;

public Person(string firstname, string surname, Address homeAddress,
              PhoneNumber homeNumber)
{
    _firstname = firstname;
    _surname = surname;
    _homeAddress = homeAddress;
    _homeNumber = homeNumber;
}

public string getFirstname()
{
    return firstname;
}

public string getSurname()
{
    return surname;
}

public Address getAddress()
{
    return address;
}

public PhoneNumber getHomeNumber()
{
    return _homeNumber;
}

Option 2: no parameters and use of set methods

private string firstname;
private string surname;
private Address homeAddress;
private PhoneNumber homeNumber;

public Person()
{
}

public string getFirstname()
{
    return firstname;
}
public void setFirstname(String value)
{
    firstname = value;
}

public string getSurname()
{
    return surname;
}
public void setSurname(String value)
{
    surname = value;
}

public Address getAddress()
{
    return address;
}
public void setAddress(Address value)
{
    address = value;
}

public PhoneNumber getHomeNumber()
{
    return _homeNumber;
}
public void setHomeNumber(PhoneNumber value)
{
    _homeNumber = value;
}

Which method do you prefer?  In my mind, option 1 is better because you know that the state of your object cannot be changed.  I believe this is called immutability and is something that should be desired?  On the other hand, it can quickly get out of hand and ugly if you have more than a handful of parameters.

I am aware that some languages such as C# have nice features that would probably make option 1 more appealing such as named and optional parameters however this question was prompted while dealing with Java code.

Update: Thank you for all of your suggestions.  I will comment on each answer individually but a few answers suggested using a factory method instead of having the constructor do all of the work.  This was actually the pattern I was using because it is a standard at work but I've never really seen the advantage of it as opposed to using the constructor to do the work.  This is more how the code looked:

private string firstname;
private string surname;
private Address homeAddress;
private PhoneNumber homeNumber;

private Person()
{
}

public Person factoryCreate(string firstname, string surname, Address homeAddress,
                            PhoneNumber homeNumber)
{
    Person person = new Person();
    person.setFirstname(firstname);
    person.setSurname(surname);
    person.setHomeAddress(homeAddress);
    person.setHomeNumber(homeNumber);
    return person;
}

public string getFirstname()
{
    return firstname;
}
public void setFirstname(String value)
{
    firstname = value;
}

public string getSurname()
{
    return surname;
}
public void setSurname(String value)
{
    surname = value;
}

public Address getAddress()
{
    return address;
}
public void setAddress(Address value)
{
    address = value;
}

public PhoneNumber getHomeNumber()
{
    return _homeNumber;
}
public void setHomeNumber(PhoneNumber value)
{
    _homeNumber = value;
}

From the discussion about having constructors do little work and using builders, I can see why having the factory method is a good one.  My understanding of factory methods was that they were used to create a class given a few options.  For example, if you have an Employee class and that is sub-classed by FullTimeEmployee and PartTimeEmployee, you would have a factory() method on Employee to deal with creating the more concrete class.

It depends on whether the data elements are required for the object to be well defined or if they are completely optional.

My personal preference is for the constructor to have parameters for all the required data elements and use set methods for everything else.  I also declare the default (parameterless) constructor to be private.  That way the compiler guarantees that no one will create an incompletely defined object.

If firstname and lastname are required but address and phone number are optional, I would do the following:

private string _firstname;
private string _surname;
private Address _homeAddress;
private PhoneNumber _homeNumber;

private Person()
{
    // Ensure unitialized Person cannot be created
}

public Person(string firstname, string surname)
{
    _firstname = firstname;
    _surname = surname;
}

// getters and setters omitted for brevity

The disadvantage of using the Builder pattern as recommended in other answers is that the completeness check is deferred to runtime.  If you have a code path that is very rarely executed and you change Person so that more data is required, then you could deliver code that will fail in the field.  With my approach, you will know about the problem the next time you try to compile your code.