Event timers database interaction layer I've just started with the basics of OOP. At first it looked complicated, but once I just started with it, it seemed so easy and everything seems to work like a charm, and that's why I'm unsure if I'm doing this right.

I'm creating a system to track actions:

This is the Timer class:

class Timer {

    private $_db;
    public $user_id;

    public function __construct($db, $user_id) {

        $this->_db = $db;
        $this->user_id = $user_id;

    }

    public function checkActiveTimer() {

        $db = $this->_db;

        $query = $db->prepare("SELECT * FROM qrm_logs WHERE `active` = :active");
        $query->execute(array(

            ':active'   =>  1

        ));

        $result = $query->fetchAll(PDO::FETCH_OBJ);

        if (count($result) > 0) {

            return true;

        }

        return false;

    }

    public function getTimerTypes() {

        $db = $this->_db;

        $query = $db->prepare("SELECT * FROM qrm_types WHERE `active` = :active");
        $query->execute(array(

            ':active'   =>  1

        ));

        $types = $query->fetchAll(PDO::FETCH_OBJ);

        if (count($types) > 0) {

            return $types;

        }

        return false;

    }

    public function startTimer() {

        $db = $this->_db;
        $user_id = $this->user_id;

        $date = new Datetime();

        $query = $db->prepare("INSERT INTO `qrm_logs` (`user_id`, `type_id`, `active`, `started_at`) VALUES (:user_id, :type_id, :active, :started_at)");
        $query->execute(array(

            ':user_id'      =>  $user_id,
            ':type_id'      =>  6,
            ':active'       =>  1,
            ':started_at'   =>  $date->format('Y-m-d H:i:s')

        ));

    }

    public function getTime() {

        $db = $this->_db;
        $user_id = $this->user_id;

        $query = $db->prepare("SELECT `started_at` FROM qrm_logs WHERE `active` = :active AND `user_id` = :user_id");
        $query->execute(array(

            ':active'   =>  1,
            ':user_id'  =>  $user_id

        ));

        $result = $query->fetchAll(PDO::FETCH_OBJ);

        if (count($result) > 0) {

            foreach ($result as $time) {

                $date = new Datetime($time->started_at);
                $date2 = new Datetime();

                $difference = $date2->getTimestamp() - $date->getTimestamp();

                return $difference;

            }

        }

        return false;

    }

    public function stopActiveTimer() {

        $db = $this->_db;
        $user_id = $this->user_id;

        $date = new Datetime();

        $query = $db->prepare("UPDATE `qrm_logs` SET `active` = :inactive, `ended_at` = :ended_at WHERE `user_id` = :user_id AND `active` = :active");
        $query->execute(array(

            ':inactive' =>  0,
            ':ended_at' =>  $date->format("Y-m-d H:i:s"),
            ':user_id'  =>  $user_id,
            ':active'   =>  1

        ));

    }

    public function startWhiteSpaceTimer($type_id) {

        $db = $this->_db;
        $user_id = $this->user_id;

        $date = new Datetime();

        $query = $db->prepare("INSERT INTO `qrm_logs` (`user_id`, `type_id`, `active`, `started_at`) VALUES (:user_id, :type_id, :active, :started_at)");
        $query->execute(array(

            ':user_id'      =>  $user_id,
            ':type_id'      =>  $type_id,
            ':active'       =>  1,
            ':started_at'   =>  $date->format('Y-m-d H:i:s')

        ));

    }

}

This is a piece of code I use in an AJAX request:

$timer = new Timer($db, $_SESSION['id']);

if ($timer->checkActiveTimer() !== false) {

    $timer->stopActiveTimer();

}

$timer->startWhitespaceTimer($type_id);

Can you give me some feedback on how I'm currently approaching it (right or wrong), and eventually correct some errors/mistakes?

There's a number of issues with your code. I'll walk through your class pointing out several issues as I go along:

class Timer {

    private $_db;
    public $user_id;

I'll just mention the same point as chervand mentioned here, but it applies to the entire class: coding standards matter, Please follow the PSR standards as much as possible: brackets go on a new line, private or protected property names needn't start with an underscore (that's something that stems from the old PHP4 days).
Also try to add doc-blocks, documenting what arguments should be passed, and what the value/type of each property is:

class Timer
{
    /**
     * @var PDO
     */
    protected $db = null;//protected, in case you'll extend this class
    /**
     * @var int
     */
    public $userId = null;//I'm assuming int, and I prefer camelCase

Next: your constructor: you're expecting the user to pass a "db". You don't specify what that variable must be. Use type-hints to prevent users from passing invalid things to methods. Especially when you're dealing with dependencies.

The userId argument is also required to be passed to the constructor. If it is that important to have this value, then why is it public? There's nothing stopping me from doing something like this:

$timer = new Timer($pdo, 123);
$timer->userId = 'Some invalid value';

It's probably best to make the property protected here, or require the caller to pass the userId as an argument to the methods that require this value. Anyway...

Here's how I'd write the constructor:

    public function __construct(PDO $db, $userId)
    {//PSR

        $this->db = $db;
        $this->userId = (int) $userId;//cast to int, so we're sure we're not dealing with a string
    }

Moving on:

    public function checkActiveTimer()
    {//PSR

        $db = $this->db;

        $query = $db->prepare("SELECT * FROM qrm_logs WHERE `active` = :active");
        $query->execute(array(

            ':active'   =>  1

        ));

        $result = $query->fetchAll(PDO::FETCH_OBJ);

        if (count($result) > 0) {

            return true;

        }

        return false;

    }

your methods, almost all of them, have the same problem: they don't take any arguments, they contain hard-coded (and sometimes bad) queries. They don't handle possible exceptions, you're using prepared statements (which generally is a good thing), even if you don't (in which case, prepared statements are just overhead).
The biggest problem with your using prepared statements, though, is that you're missing out on the best part about them. Prepared statements allow you to re-use a query. You're preparing the same query string over and over again... pointlessly. Here's what my method would look like:

public function getActiveTimerCount()
{
    $stmt = $this->db->query(
        "SELECT COUNT(`active`) as cnt FROM qrm_logs WHERE `active` = 1"
    );
    $row = $stmt->fetch(PDO::FETCH_ASSOC);
    return (int) $row['cnt']
}

Now this method is a lot shorter. I know that active will be 1, so I'm not at risk of SQL injection attacks here. Instead of fetching all rows, I'm asking the DB to return me the row count, and I'm returning the count. This is more informative to the caller. 0 is still a fasly value, other values are truthy. The caller can still use this method as though it returned a boolean. However, because I'm returning the actual count, the caller can use the same method to work out how many active timers there are.

I also got rid of that reassignment ($db = $this->_db). It's personal preference mainly, but in this case, I think $this->db->query is just as easy to read.
The same basic comments apply for the getTimerTypes method

First, slightly late update

Ok, I promised to revisit this review when I had the time, it took some patience, but I found a free 15min, which I'll spend reviewing this code:

public function getTime() {

    $db = $this->_db;
    $user_id = $this->user_id;

    $query = $db->prepare("SELECT `started_at` FROM qrm_logs WHERE `active` = :active AND `user_id` = :user_id");
    $query->execute(array(

        ':active'   =>  1,
        ':user_id'  =>  $user_id

    ));

    $result = $query->fetchAll(PDO::FETCH_OBJ);

    if (count($result) > 0) {

        foreach ($result as $time) {

            $date = new Datetime($time->started_at);
            $date2 = new Datetime();

            $difference = $date2->getTimestamp() - $date->getTimestamp();

            return $difference;

        }

    }

    return false;

}

As before, the comments about PSR coding standards, and there being no real need to reassign the properties to local variables still apply.

You are using prepared statements, and you're using them to use the userId value (which is provided by the caller, and therefore not to be trusted) safely. This of course makes perfect sense. You're also binding a hard-coded 1 which really doesn't make that much sense at all. For starters, if this value is something only of interest to your class, you'd probably want to define it as a constant. If the value ever needs to change, it's easier to change the value assigned to a constant once, then going through the code and changing all occurrences of 1 to their new value. Especially if the value, like 1, can be used for things other than the active value.
More importantly here, though, is that there is no real reason to bind a known integer value. You can just make that part of the WHERE clause part of the prepared statement. It's an immutable part anyway. Here's what I'd do:

//at the top of the class:
const TIMER_ACTIVE = 1;

public function getTime()
{
    //makes query easier to read IMO
    $stmt = $this->db->prepare(
        'SELECT started_at
        FROM qrm_logs
        WHERE `active` = ' . self::TIMER_ACTIVE .
        ' AND `user_id` = :uid'
    );
    $stmt->execute([':uid' => $this->userId]);

The next thing I notice about your code is that you're using fetchAll, then iterate over the results, and return inside the loop. This suggests that all you're really after is a single result. Using LIMT 1 in your query would make this clearer to whoever ends up maintaining this code. If a query can return multiple results, but I'm only after one row, I'd never ever use fetchAll. Personally, I'd use fetch, to get a single row. Your call to fetchAll implies that all results are being fetched and loaded into memory. That's extremely wasteful considering you're never ever processing more than a single row.
You're also using count to check whether or not there are any results to process. There's no reason for that function call, though. If you use fetch, the return value is null if there aren't any results, or the row (as an object, or array, or whichever fetch mode you use).
even if you use fetchAll, though, an empty array in PHP is a falsy value, so you could just as well write if (count($result) > 0) { like so:

if ($result) {
    //$result is NOT empty
}
//or:
if (!$result) {//in PHP, !array() is true
    return false;//
}
//process $results here

So what does that mean for the code we've discussed so far, and how would I write it?

public function getTime()
{
    //makes query easier to read IMO
    $stmt = $this->db->prepare(
        'SELECT started_at
        FROM qrm_logs
        WHERE `active` = ' . self::TIMER_ACTIVE .
        ' AND `user_id` = :uid'
    );
    $stmt->execute([':uid' => $this->userId]);

    $row = $stmt->fetch(PDO::FETCH_OBJ);
    if (!$row) {
        return false;//no active records found for user
    }

Now all that's left for us to do is to return the interval in seconds. I personally advocate the use of the DateTimeInterface objects as much as possible. However, in this particular case, I don't think it's worth it. You want to subtract a date (in timestamp format) from the current timestamp. You can do that as a one-liner by calling 2 simple functions: time() and strtotime(). The first just returns the current timestamp, the second returns the timestamp for the string representation of a date. Seeing as started_at is coming from the DB, I'm assuming it's a MySQL DATETIME or TIMESTAMP field, and its values can be handled by strtotime just fine. Here's how I'd write your function in full:

public function getTime()
{
    $stmt = $this->db->prepare(
        'SELECT started_at
        FROM qrm_logs
        WHERE `active` = ' . self::TIMER_ACTIVE .
        ' AND `user_id` = :uid'
    );
    $stmt->execute([':uid' => $this->userId]);

    $row = $stmt->fetch(PDO::FETCH_OBJ);
    if (!$row) {
        return false;
    }

    return time() - strtotime($row->started_at);
}

This looks simple, clean and easy.
A Final not, instead of passing those PDO::FETCH_* constant every time you're fetching, you might want to consider setting the default fetch mode on the PDO instance ($this->db->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);), provided no other piece of code has access to the same PDO instance, or that other piece of code doesn't change the default fetch mode, you don't have to bother passing the fetch mode every time again...

I'll look into the rest of the code a bit later on, and update this answer as I go along. This is all I have time for for now.


Push an item onto the end of array in Java I want to make a method arrayIntPush(3, {5,4,8}) that will return {5,4,8,3}.
This method seems to work:

public static int[] arrayIntPush(int item, int[] oldArray) {
    int len = oldArray.length;
    int[] newArray = (int[]) Array.newInstance(oldArray.getClass().getComponentType(), len + 1);
    System.arraycopy(oldArray, 0, newArray, 0, len);
    System.arraycopy(new int[] {item}, 0, newArray, len, 1);

    return newArray;
}

Is there a better way to do this that would be faster?

Is this really needed?

int[] newArray = (int[]) Array.newInstance(oldArray.getClass().getComponentType(), len + 1);

I would do something like:

public static int[] arrayIntPush(int item, int[] oldArray) {
    int len = oldArray.length;
    int[] newArray = new int[len+1];
    System.arraycopy(oldArray, 0, newArray, 0, len);
    newArray[len] = item;

    return newArray;
}

So for an array of length 10 you make a new array or length 11, copy all existing data into it and then assign the last index to the item.


Add events dispatching for ActionScript framework Robotlegs I have the following function in a Service class

protected function writeToFile():void{
        var destinationFile:File = File.applicationStorageDirectory.resolvePath(_newPath);

        var fs:FileStream = new FileStream();

        fs.open(destinationFile, FileMode.WRITE);
        fs.writeBytes(_buffer, 0, _buffer.length);

        fs.close();

    }

this Service class should dispatch a SuccessfulCreateLocalFileEvent when writeToFile is successful and an UnsuccessfulCreateLocalFileEvent if writeToFile is unsuccessful.

I know that FileStream functions open and writeBytes may throw IOError if there is any problems. And all these FileStream functions all have void as return types.

I believe I need try catch, but I am not entirely sure how to do so.

Please advise.

This question is replicated at http://knowledge.robotlegs.org/discussions/questions/924-what-is-best-way-to-write-dispatch-events-in-this-service-class-function Experimenting which site has better answers for such a question.

FileStream class has its own events which are dispatched when the file operation is successful or not.

http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/filesystem/FileStream.html#eventSummary

A possible refactor of the code is

protected function writeToFile():void{
    _destinationFile = File.applicationStorageDirectory.resolvePath(_newPath);

    var fs:FileStream = new FileStream();

    fs.open(_destinationFile, FileMode.WRITE);
    fs.writeBytes(_buffer, 0, _buffer.length);

    fs.addEventListener(Event.CLOSE, dispatchFileCreatedEvent);

    fs.close();

}

Thanks to krasimir of knowledge Robotlegs Community for his help in arriving at this answer.


Change CSS style with Javascript I have written this small function in Javascript for changing the padding on an element in HTML. It works well, however, I wonder if there is a better way to write it without so many if statements?

  window.addEventListener("resize", navPadding);

  function navPadding() {
    let width = window.innerWidth;

    if(width < 1200){
      navDesktop.style.padding = "unset"
    } else

    if(width > 1200 && width < 1300){
      navDesktop.style.padding = "0 19% 0 5.5%"
    } else

    if(width > 1300 && width < 1400){
      navDesktop.style.padding = "0 19% 0 8.5%"
    } else

    if(width > 1400 && width < 1500){
      navDesktop.style.padding = "0 19% 0 11.5%"
    } else

    if(width > 1500 && width < 1600){
      navDesktop.style.padding = "0 19% 0 14%"
    } else

    if(width > 1600 && width < 1700){
      navDesktop.style.padding = "0 19% 0 16%"
    } else

    if(width > 1700 && width < 1800){
      navDesktop.style.padding = "0 19% 0 18%"
    } else

    if(width > 1800 && width < 1900){
      navDesktop.style.padding = "0 19% 0 19%"
    } else

    if(width > 1900 && width < 2000){
      navDesktop.style.padding = "0 20% 0 21%"
    };
  };

  navPadding();

That huge if/else conditional could be converted to for loop with break statement.Assuming 1200 and 2000 as range's lower/upper boundaries.

Padding bottom-left value has the most variation depending on width.That fixed variation can be declared as predefined array:

const paddingSteps = [5.5, 8.5, 11.5, 14, 16, 18, 19, 21];

The final concise implementation:

const paddingSteps = [5.5, 8.5, 11.5, 14, 16, 18, 19, 21];

function navPadding() {
    let width = window.innerWidth;

    if (width < 1200) {
        navDesktop.style.padding = "unset";
        return;
    }
    for (i = 1200; i <= 2000; i += 100) {
        if (width >= i && width <= i + 100) {
            padRight = (width >= 1900) ? 20 : 19;
            navDesktop.style.padding = `0 ${padRight}% 0 ${paddingSteps[i % 1200 / 100]}%`;
            break;
        }
    }
}

navPadding();


Class to get user information I've just begun creating a class to get User information, and I'd like for it to be improved.

The global variable $oConni is the database connection.

class UserInfo {

public $id, $oConni;

public function __construct($id, $oConni) {
    $this->id = $id;
    $this->oConni = $oConni;
}

public function getInfo() {
    $userInfo = array();
    $cSQL = "SELECT name, last_name, email, use_gravatar, pic_profile FROM users WHERE id = ?";
    $stmt = $this->$oConni->prepare($cSQL);
    $stmt->bind_param('i', $this->$id);
    $stmt->execute();
    $stmt->store_result();
    $stmt->bind_result($name, $lastName, $email, $gravatar, $picProfile, $admin);
    if ($stmt->fetch()) {
        $userInfo['name'] = $name;
        $userInfo['lastName'] = $lastName;
        $userInfo['email'] = $email;
        $userInfo['gravatar'] = $gravatar;
        if($gravatar == 1){
            $size = 500;
            $grav_url = "http://www.gravatar.com/avatar/".md5(strtolower(trim($email)))."?s=" . $size;
            $userInfo['picProfile'] = $grav_url;
        }else if(strpos($picProfile, 'http') === 0 || strpos($picProfile, 'https') === 0){
            $userInfo['picProfile'] = $picProfile;
        }else{
            $pic = 'http://www.example.com/'.$picProfile.'?id='.RAND(0,999999);
            $userInfo['picProfile'] = $pic;
        }
    }
    $stmt->close();

    return $userInfo;
}
}
$userInf = new UserInfo($cmntUser, $oConni);
$userData = $userInf->getInfo();
print_r($userData);

Structure

I don't really like returning a magic array. The caller has to guess or read up on the keys, which is annoying. I know that this is often done in PHP, but I think a better approach would be to move the retrieving of information to the constructor, store the result in class fields, and then access those fields with explicit getter (or if you don't want to completely rewrite your class, store the array as field, and add getters for each key of the array).

This could be used like this:

$userInfo = new UserInfo($userId, $dbConnection);

Name: <?php echo htmlentities($userInfo->getName()); ?>

Email: <?php echo htmlentities($userInfo->getEmail()); ?>

[...]

For debug purposes you could still add a toString method with returns a string of all array values.

If you do it like this, not only is your class a lot easier to use (without having to read the documentation and/or knowing of the inner workings of the class), the class now actually is more than a wrapper for a method that could just as well be a static function.

Misc

You bind a result to $admin which never exists.
your fields should be private.
naming: oConni could be pdo or database, cSQL could be query, and grav_url gravatarUrl (you are using camelCase everywhere else).
your indentation and spacing is sometimes off, you can use an IDE to fix this easily.


Convert an array to a nested object var a = ["a", "b", "c", "d"]
var res = {}
var value = "Hello"
for(var i=a.length; i > 0; i-- ){
  var item = a[i-1]
  var temp = {}
  temp[item] = i == a.length ? value : {}
  if(i != a.length){
    Object.keys(res).forEach(r => {
      temp[item][r] = res[r]
    })
  }
  res = temp
}

console.log(res)

The result is :

{
  "a": {
    "b": {
      "c": {
        "d": "Hello"
      }
    }
  }
}

Which is right, exactly what I need. I was just wondering if I'm doing it the right way or is there any better solutions for this?

Thanks in advance.

You are currently mixing declarative and functional style. I suggest succinct functional style:

var keys = ["a", "b", "c", "d"];
var value = "Hello";
var res = keys.reduceRight((value, key) => ({[key]: value}), value);


Is there any way to make this CoffeScript code simpler / smaller I've just started with coffeescript,  and I saw  http://blog.8thlight.com/uncle-bob/2012/04/20/Why-Is-Estimating-So-Hard.html  which has a little programming problem,  so I figured I'd do it in coffeescript (which seemed easy enough to do... )

anyways, I ended up with :-

lineEndings = ( s, len ) ->
  words = s.split ' '
  line = ''
  result = ''
  words.map ( word ) ->
    if line.length == 0
      line = word
    else if line.length + 1 + word.length <= len
      line = line + ' ' + word
    else
      result = result + line + '\n'
      line = word
  result + line

getty = "Four score and seven years ago our fathers brought forth upon this continent a new nation, conceived in liberty and dedicated to the proposition that all men are created equal"
console.log lineEndings getty, 13

I'm looking for any ways I could make this simpler coffeescript.

( I just used nodejs with coffescript installed to test this )

This is how I would write it.

lineEndings = (s, len) ->
  words = s.split ' '
  line = []
  result = []
  for word in words
    if line.length + 1 + word.length <= len
      line = [line..., word]
    else
      result.push line.join ' '
      line = [word]
  lastLine = line.join ' '
  result.push lastLine if lastLine
  result.join "\n"

There's nothing wrong with how you wrote it, though.


Heavily nested anonymous classes for Swing UIs Oftentimes, I write small little dialogs or forms without a UI builder. I usually find myself having a lot of "container" panel variables that just float around. I found this leads to time-wasting experiences.

Where did I put that BorderLayout panel?
Oh right, here it is.
Wait, never mind. That one has a GridLayout.

A while ago, I had the idea to nest widgets in anonymous classes, directly in add calls. This way, a simple layout has fewer variables that get in the way of digesting the code's logic since every component is nicely indented in its own scope.
Pros

Component hierarchy is obvious, as are component attributes like colors and fonts
Code is cleaner, with less dummy container variables
Forces component logic to be encapsulated directly in the component (i.e. not 50 lines up or below the component initialization: saves time)
I found it easier to come back to code written in this style after a couple months and still understand the layout.

Cons

Might bloat generated code size, because all the Swing constants would have to be written to each classfile

Below is a simple (Java 8) frame that shows a button and a label. I realise that this same layout could have been made with fewer panels, but I'm presenting it as an overly explicit example of my approach.
import javax.swing.*;
import java.awt.*;

public class TestFrame extends JFrame {
    public TestFrame() {
        super("A Frame");
        add(new JPanel() {
            {
                setLayout(new BorderLayout());
                setBackground(Color.LIGHT_GRAY);
                add(new JPanel() {
                    {
                        setLayout(new BorderLayout());
                        setOpaque(false);
                        add(new JLabel("A label", JLabel.LEFT) {
                            {
                                setBorder(BorderFactory.createEmptyBorder(0, 10, 0, 10));
                                setFont(getFont().deriveFont(Font.BOLD));
                            }
                        }, BorderLayout.EAST);
                        add(new JButton("Click me!") {
                            int timesClicked = 0;

                            {
                                addActionListener((x) -> {
                                    timesClicked++;
                                    setText(String.format("You've clicked me %d time(s)!", timesClicked));
                                });
                            }
                        }, BorderLayout.CENTER);
                    }
                });
            }
        });
        setSize(new Dimension(640, 480));
        setLocationRelativeTo(null);
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    }

    public static void main(String[] argv) {
        try {
            UIManager.setLookAndFeel(UIManager.getSystemLookAndFeelClassName());
        } catch (ReflectiveOperationException | UnsupportedLookAndFeelException ignored) {
        }
        SwingUtilities.invokeLater(() -> {
            TestFrame test = new TestFrame();
            test.setVisible(true);
        });
    }
}

I (and I'm not by any means an expert on Swing) haven't seen Swing code like this around on StackExchange sites, or really, anywhere I look. I can't have been the only one to think of this approach - is there a reason why an aggressive subclassing approach is not preferred, or have I just not been searching hard enough?
I think the readability benefits far outweigh any costs this approach has.

Anonymous constructors are very confusing for many people. They don't correspond well to people's expectations, and are not common knowledge.

I would recommend against them on principle.

Having said that, I can see how you think this has helped reduce your code boilerplate, but, you need to be consistent about it then. Currently your class is defined as:

public class TestFrame extends JFrame {
    public TestFrame() {
        super("A Frame");
        add(new JPanel() {
            {

when really, that should be:

public class TestFrame extends JFrame {
    {
        setName("A Frame");
        add(new JPanel() {
            {

If you are going to be unconventional, then you at least need to be consistently unconventional.

timesClicked should probably be an AtomicInteger so you can deal with some concurrency in the clicked event - or, rather, so you can read it from a different thread later. Lambdas with side-effects should be avoided.

Having said all of that, you really should use a style that is more conventional just so others can read your code better. The indentation levels on their own are a distraction with the addition of the anonymous blocks.

Oh, there's one other item that needs to be said, with all the anonymous classes there, your stack traces for any exceptions are going to be much harder to read without the code in hand too. You lose all the benefits of named classes. This will hamper your debug time.


Raspberry PI controllable car (code for a 6-year old) I've been trying to show my 6-year old daughter a way to build and create a simple toy car. We've got a 4-motor chassis kit, Raspberry PI and a motozero shield that can operate these 4 motors. We've put a power bank and a separate battery for the motors:

                                              (The green light on the right is the traffic light)

Now, as far as coding goes, I've created a simple Car class with forward, backward, forward_left, forward_right, backward_left and backward_right actions. For the demonstration purposes mostly, I've tried to be explicit and violated the DRY principle a lot.

I've also initialized an endless loop listening for keyboard inputs - F would be for forward, B for backwards and S for stop (have not decided which keys would go for those "left-ish" and "right-ish" actions yet).

from RPi import GPIO
from gpiozero import Motor, OutputDevice

class Car:
    def __init__(self):
        self.front_right = Motor(forward=24, backward=27)
        self.front_left = Motor(forward=6, backward=22)

        self.rear_right = Motor(forward=13, backward=18)
        self.rear_left = Motor(forward=23, backward=16)

        OutputDevice(5).on()
        OutputDevice(17).on()
        OutputDevice(25).on()
        OutputDevice(12).on()

    def forward(self):
        self.front_right.forward()
        self.front_left.backward()

        self.rear_right.backward()
        self.rear_left.backward()

    def backward(self):
        self.front_right.backward()
        self.front_left.forward()

        self.rear_right.forward()
        self.rear_left.forward()

    def backward_right(self):
        self.front_right.backward(1)
        self.front_left.forward(1)

        self.rear_right.forward(1)
        self.rear_left.forward(0.2)

    def backward_left(self):
        self.front_right.backward(1)
        self.front_left.forward(1)

        self.rear_right.forward(0.2)
        self.rear_left.forward(1)

    def forward_left(self):
        self.front_right.forward()
        self.front_left.backward(0.2)

        self.rear_right.backward(1)
        self.rear_left.backward(1)

    def forward_right(self):
        self.front_right.forward(0.2)
        self.front_left.backward(1)

        self.rear_right.backward(1)
        self.rear_left.backward(1)

    def stop(self):
        self.front_right.stop()
        self.front_left.stop()

        self.rear_right.stop()
        self.rear_left.stop()

if __name__ == '__main__':
    GPIO.setwarnings(False)
    GPIO.cleanup()

    commands = {
        "f": "forward",
        "b": "backward",
        "s": "stop"
    }
    try:
        car = Car()

        while True:
            command = raw_input()
            getattr(car, commands[command])()
    finally:
        try:
            GPIO.cleanup()
        except:  # ignore cleanup errors
            pass

(I've also mixed up some motors and pins on the board which led to some motors accepting "backwards" as "forward")

It is, of course, difficult to explain this kind of code to a 6-year old, since she still expects a printer to print something when I show her the Python's print() command, but, how would you change this code to make it more understandable for a kid?

I would also appreciate any points about the code organization and quality since the next step would be to control the car "via Bluetooth".

The implementation of forward is horrible. The only correct way of expressing it is to make all four motors go forward. As an analogy, how would you react if your keyboard manufacturer told you "oh sorry, we made a mistake, you have to type every second letter in uppercase to get a lowercase word"? That would be non-intuitive.

The API you provide must be as clean and simple as possible. You don't want your daughter to learn that "programmers make mistakes and are lazy about fixing them, so we have to live with that".

In which ways is your daughter supposed to change the current program? You should probably add some example code for waiting some time, to allow for composed commands like "forward, wait 1s, right, 1s, backward".

Your code seems to crash with an exception when an unexpected key is pressed. That's unacceptable for a robot, since it might harm people or pets when running uncontrolled (see Asimov's laws).

Using raw_input looks as if the user has to press the Enter key to confirm each action. That's too much of a burden. Controlling the car must be simple and immediate, to prevent accidents. How would you feel if your grown-up street car would ask you for confirmation for each step on the gas or brake?


Giving actual username from Gmail ID If you have a Gmail ID, then you can also receive emails on the same account even if you add extra dots in the username or add '+' and some characters.

Say your Gmail ID is montypython@gmail.com.  You will receive emails on same account even if it is sent to:

monty.python@gmail.com
montypython+python@gmail.com
m.o.nty.p.y.t.hon@gmail.com

Following is the code, which gives me back username removing dots and characters after '+' symbol. Do note that I really don't care if user enters an invalid Gmail ID.

import re

def return_gmail_user(email):
    username = email.split('@')[0]
    domain = email.split('@')[1]
    valid_email_char = '[\w\d\.]+'
    # check if email is of gmail
    if domain != 'gmail.com':
        return False
    # get username
    m = re.match(valid_email_char, username)
    if m:
        username = m.group(0)
        #print username
        return "".join(username.split('.'))
    return False

def main():
    assert('my' == return_gmail_user('my@gmail.com'))
    assert('my' == return_gmail_user('m.y@gmail.com'))
    assert('avinassh' != return_gmail_user('avinassh@reddit.com'))
    assert('avinassh' == return_gmail_user('avinassh+reddit@gmail.com'))
    assert('avinassh' == return_gmail_user('av.ina.s.sh@gmail.com'))
    assert('avinassh' == return_gmail_user('avi.na.ss.h+reddit@gmail.com'))
    assert('avinassh' == return_gmail_user('avi.na.ssh+red+dit@gmail.com'))

if __name__ == '__main__':
    main()

def return_gmail_user(email):

Return isn't a very descriptive word to use as the beginning  of a function name. Call it parse_gmail_user or something along those lines instead.

    username = email.split('@')[0]
    domain = email.split('@')[1]

Read up about sequence unpacking. There's no need to use indices directly, just assign the result of the split operation directly to both variables.

    valid_email_char = '[\w\d\.]+'
    # check if email is of gmail
    if domain != 'gmail.com':

The comment is redundant, it adds nothing to the code — remove it.

        return False

It would be more Pythonic to raise an exception instead of returning False.

    # get username
    m = re.match(valid_email_char, username)

Give m a descriptive name, such as valid_user, making the code more fluent.

    if m:

You could combine the two if statements to fail early if an incorrect value is given for email.

        username = m.group(0)

group retrieves the first group by default, no need to supply 0.

        #print username

This comment is probably a remnant from testing; commented-out code should be removed immediately as it adds clutter and serves only to confuse.

        return "".join(username.split('.'))

replace('.', '') would be a more descriptive option of achieving the above.

    return False

Following my suggestions, you could arrive the below solution.
Note that the semantics are insignificantly different (the match is performed regardless of the validity of the domain), but readability and simplicity is substantially better.

def return_gmail_user(email):
    user, domain = email.split('@')
    valid_user = re.match('[\w\d\.]+', user)
    if domain != 'gmail.com' or not valid_user:
        raise ValueError('invalid gmail address: ' + email)
    return valid_user.group().replace('.', '')


Check if bit is only set once in a vector of int I have a vector of uint16_t and I want to check if there is a bit which is only set in one vector.  I then get its position within the vector as well as from the bit.

Let's say we have:

v[0] = 0100
v[1] = 0110
v[2] = 1001

I would like to know that ind = 1 and val = 2.

In clumsy code this would be like that:

#include <cstdint>
#include <vector>
#include <fstream>

const uint16_t N = 10;

int main() {
    // Create a dataset
    std::vector<uint16_t> vec(N, (1 << 2) + (1 << 3));
    vec[3] = (1 << 5);
    vec[6] = (1 << 7);

    // loop over the values
    for (auto i = 0; i < 16; i++) {
        uint16_t counter = 0,ind;

        // loop over the vector
        for (auto ii = 0; ii < N; ii++) {
            if (vec[ii] & (1 << i)) {
                if (counter++ == 1) { break; }
                ind = ii;
            }
        } // vector loop

        // Do we only have a single value?
        if (counter == 1) {
            printf("%i @ %i\n",i,ind);
        }
    } // value loop
} // main

Is there a better way to do this?  In case it matters using g++ with Ubuntu.

Let me start by pointing out problems in your code:

Wrong type

The type of your global constant N is uint16_t. There are several problems with this:

as you are using cstdint the type should be std::uint16_t (because the C++ header provides them in the namespace std, this note should be applied to all instances of uint16_t in your code)
you are using N to indicate the size of the std::vector but sizes are specified using std::size_t
you are using uint16_t for the variables counter and ind, which are not related to the type. counter just counts, so you would be fine with std::size_t and ind is an index to a vector so it might be an std::size_t as well

Magic number

The loop boundary 16 in your code is related to the size of the type that is stored in the vector and offers possibilities for inconsistencies between the two. You should not rely on that, instead calculate it from the type:

int numberOfBitsPerEntry = sizeof(vec[0]) * CHAR_BIT;

And use that as loop boundary.

Variable names

While N is borderline (as soon as there are more then one containers it becomes ambiguous) the other variable names are not:

i is about as generic as it can get for a loop counter variable. What it actually does is signify the index of the bit you are currently looking at, so how about: bitIndex
ii just tells me that i was already taken and you temporarily forgot what was the next letter in the alphabet. This index actually walks through the std::vector's elements, so you might call it elementIndex (which is still too generic but I don't know what exactly is stored in the vector)
counter has already some information but it still does not inform you what is counted, how about enabledBitsCounter or if you take the notion of arranging your elements above each other so that the bits with the same index form a column: columnBitsCounter (in that light elementIndex might be named rowIndex)
ind (probably short for index) is just another generic name. It signifies the index of the element that is the only one that has a one bit in this column. columnWinnerRowIndex might be a bit too verbose but the general idea should be clear
vec does not say much about the contents. By staying in the table metaphor we might choose rows as the new name

This is C++

You are using printf in C++ where there are better alternatives (at least typesafety wise). Also you haven't included cstdio or stdio.h where printf comes from.

As it is C++ I would pledge for std::cout instead:

std::cout << bitIndex << " @ " << columnWinnerRowIndex << "\n";

Algorithm

Now to the algorithm itself. You need to walk through every column and look at each row so there is not much to change on the loops. (Besides the fact that auto might be overkill for int variables)

However, I find the loop break unnecessary (avoid changing the flow of loops from within the body) and the counter too heavyweight.

Basically there are 3 states for a column:
- 0 one bits
- 1 one bit
- too many one bits

This would fit nicely into an enum:

enum ColumnState {
    NO_ONE_BITS,
    ONE_ONE_BIT,
    TOO_MANY_ONE_BITS
};

Your counter is replaced with this:

    ColumnState columnState = NO_ONE_BITS;
    for(int rowIndex = 0; rowIndex < (int)rows.size() &&
                          columnState != TOO_MANY_ONE_BITS; ++rowIndex) {
        if(rows[rowIndex] & (1 << columnIndex)) {
            switch(columnState) {
            case NO_ONE_BITS:
                columnState = ONE_ONE_BIT;
                columnWinnerRowIndex = rowIndex;
                break;
            case ONE_ONE_BIT:
                columnState = TOO_MANY_ONE_BITS;
                break;
            default:
                assert(!"Invalid state");
            }
        }
    }

On second thought the enum looks too verbose and the main problem I had with the break was the counter++ == 1 part that is a bit mind bending. Reverting to a counter might be more readable:

    int columnBitsCounter = 0;
    for(int rowIndex = 0; rowIndex < (int)rows.size(); ++rowIndex) {
        if(rows[rowIndex] & (1 << columnIndex)) {
            ++columnBitsCounter;
            if(columnBitsCounter > 1) {
                break;
            }
             columnWinnerRowIndex = rowIndex;
        }
    }

Encapsulation

You might want to separate the logic of finding the column winners from the output code and the input. The signature of a function that allows this could be

std::vector<std::size_t> findRowWinners(const std::vector<std::uint16_t> &rows);

Where the result vector gives the winning row for each column or rows.size() if there is no winner in this column.


Multi-thread safe buffer in C++ I am trying to design thread-safe data structure that I cna use as a buffer in my application. Can you please give me comments about this code and what can be improved:

#include <deque>
#include <thread>
#include <mutex>
#include <condition_variable>

template <typename T>
class Buffer
{
public:
    void add(T num)
    {
        while (true)
        {
            std::unique_lock<std::mutex> locker(mu);
            buffer.push_back(num);
            locker.unlock();
            cond.notify_all();
            return;
        }
    }
    T remove()
    {
        while (true)
        {
            std::unique_lock<std::mutex> locker(mu);
            cond.wait(locker, [this](){return buffer.size() > 0;});
            T back = buffer.back();
            buffer.pop_back();
            locker.unlock();
            cond.notify_all();
            return back;
        }
    }
    int size()
    {
        std::unique_lock<std::mutex> locker(mu);
        int s = buffer.size();
        locker.unlock();
        return s;
    }
private:
    std::mutex mu;
    std::condition_variable cond;

    std::deque<T> buffer;
};

Problems with add:

the while(true) is useless
you want to notify_one, not notify_all because you've only added one thing, and so only one thread waiting to remove can remove it.
you should overload for two version which take const T& and T&& (the latter would std::move). This would be optimal in all cases. (analogous to push_back in standard containers)
You may also want to add an emplace to complement the add() versions so you can construct T in-place at the top of the Buffer.
you can use lock_guard instead of unique_lock and scope it with {}s. I'd just say always prefer the lighter weight construct when a heavier weight one isn't strictly needed.

problems with remove:

the while (true) is useless
the cond var lambda can be return !buffer.empty();
the back variable can be std::moved
what happens if you destruct an empty Buffer while a thread is waiting on remove? That case isn't handled at all, and it's a problem
there's no reason to notify_all in remove. Who is it notifying and why?

problems with size:

use a lock_guard unless unique_lock is needed
just do: std::lock_guard<std::mutex> lock{mu}; return buffer.size();


I don't like this:

do
{
    printf("Card number: ");
    scanf("%lld", &no);
}

while(no<10);

That blank line makes it look like the while (); is a separate (possibly infinite) loop.  I recommend writing the while keyword straight after the closing brace, like this:

do {
    printf("Card number: ");
} while (scanf("%lld", &no) != 1);

(I've also changed the test so that we don't access an uninitialised no if the scanf() fails - it's still flawed, though, as any permanent stdin failure will lead to an infinite loop).

It's not clear why we have d_1, d_2, ..., etc. when all we do with them is to copy them into array members - why not just assign to the array members directly?

Instead of dividing no by successively bigger numbers each time, we could modify it as we go, like this:

d_16 = no % 10;
no /= 10;
d_15 = no % 10 * 2;
no /= 10;
d_14 = no % 10;

It's starting to become clear how to transform this into a loop.  The missing part is whether to multiply by 2 at each step; we can tell whether the loop index is even or odd, by considering it modulo 2.

Once we are considering the digits in a loop, we can perform the addition as we go, meaning that we no longer need all those local variables.


Remake of factoring/prime factorization calculator Background info on factoring and prime factorization.

I've remade this program from my first attempt, and I'm sure it can still be improved.  My questions:

These calculations should only be done with positive integers.  Would something like templates help keep this type-safe?  What about throwing an exception?
Is this an effective way to use the typedefs?  Only the classes need them, so it seems better to qualify them with each class rather than a namespace.
Can both calculate()s be better optimized, considering they perform numerous divisions?
I'm not quite liking the call to calculate() in both display()s, even though there are no data member changes.  Is this still okay?  It doesn't quite make sense to perform the calculations in the constructor, and calculate() should still be private.

Factors.h

#ifndef FACTORS_H
#define FACTORS_H

#include <cstdint>
#include <map>

class Factors
{
private:
    typedef std::map<std::uint64_t, std::uint64_t> FactorsList;
    std::uint64_t integer;
    FactorsList calculate() const;

public:
    Factors(std::uint64_t);
    void display() const;
};

#endif

Factors.cpp

#include "Factors.h"
#include <cmath>
#include <iostream>

Factors::Factors(std::uint64_t i) : integer(i) {}

Factors::FactorsList Factors::calculate() const
{
    float sqrtInt = std::sqrt(static_cast<float>(integer));
    FactorsList factors;

    for (std::uint64_t i = 1; i <= sqrtInt; ++i)
    {
        if (integer % i == 0)
        {
            factors[i] = integer / i;
        }
    }

    return factors;
}

void Factors::display() const
{
    FactorsList factors = calculate();

    for (auto iter = factors.cbegin(); iter != factors.cend(); ++iter)
    {
        std::cout << iter->first << " x " << iter->second << "\n";
    }
}

Primes.h

#ifndef PRIMES_H
#define PRIMES_H

#include <cstdint>
#include <map>

class Primes
{
private:
    typedef std::map<std::uint64_t, std::uint64_t> PrimesList;
    std::uint64_t integer;
    PrimesList calculate() const;

public:
    Primes(std::uint64_t);
    void display() const;
};

#endif

Primes.cpp

#include "Primes.h"
#include <iostream>

Primes::Primes(std::uint64_t i) : integer(i) {}

Primes::PrimesList Primes::calculate() const
{
    std::uint64_t intCopy = integer;
    std::uint64_t divisor = 2;
    PrimesList primes;

    while (intCopy % divisor == 0)
    {
        intCopy /= divisor;
        primes[divisor]++;
    }

    for (divisor = 3; intCopy > 1; divisor += 2)
    {
        while (intCopy % divisor == 0)
        {
            intCopy /= divisor;
            primes[divisor]++;
        }
    }

    return primes;
}

void Primes::display() const
{
    PrimesList primes = calculate();

    for (auto iter = primes.cbegin(); iter != primes.cend(); ++iter)
    {
        if (iter != primes.cbegin()) std::cout << " x ";
        std::cout << iter->first;
        if (iter->second > 1) std::cout << '^' << iter->second;
    }
}

Factors::calculate() can be sped up by:

performing two integer adds after each iteration to determine loop termination

std::sqrt()'s overhead, calculations, and casting can be slow
for-loops shouldn't increment towards a floating-point value
incrementing towards an integer value is faster and proper
adding is generally fast and can be done in place of multiplication

starting the loop counter at 2 or 3 (if integer is even or odd respectively)

fewer increments means fewer division operations (division is slow)

This integer add method replaces multiplication as such:

0*0 = 0
1*1 = 1 = 0 + 1
2*2 = 4 = 1 + 3
3*3 = 9 = 4 + 5
4*4 = 16 = 9 + 7
// etc.

Revised function with explanations:

Factors::FactorsList Factors::calculate() const
{
    FactorsList factors;
    std::uint64_t incr = 0;    // will increment by 2 each iteration
    std::uint64_t incrSum = 1; // will accumulate value of 'incr' each iteration

    // if integer is even, i starts at 2
    // if integer is odd, i starts at 3
    std::uint64_t i = (integer % 2 == 0) ? 2 : 3;

    // 'i' is only incremented by 1 for checking each number
    // value of 'incrSum' will determine loop termination
    for (; incrSum <= integer; ++i)
    {
        if (integer % i == 0)
        {
            factors[i] = integer / i;
        }

        incr += 2;
        incrSum += incr;
    }

    return factors;
}


A counterexample in balanced parentheses In this program I had to analyse if the parenthesis given are well balanced. For instance, input (()) is correct and input ())(() is incorrect. I've tested with other similar inputs, so my question is: can my code fail with some weird expression (using parentheses) or will it always work correctly?

public class Main {

public static void main(String[] args) {
    Scanner x = new Scanner(System.in);
    Stack<String> parenthesis = new Stack<>();

    System.out.println("Introduce the length of your expression");
    int l = x.nextInt();
    System.out.println("Introduce your expression");

    x.nextLine();
    for (int i = 0; i < l; i++) {
        System.out.println("Introduce an element");
       String e = x.next();
        parenthesis.push(e);
    }

    analysis(parenthesis);
}

public static void analysis(Stack<String> st) {

    Stack<String> closed = new Stack<>();//only for parentesis like this: )

    int ww = 0;
    while (!st.isEmpty() && ww != 1) {
        String s =st.pop();
        if (0 == s.compareTo("(") && !closed.isEmpty()) {
            closed.pop();
        } else if ((0 == s.compareTo("(")) && closed.isEmpty()) {
            System.out.println("incorrect expression.");
            ww = 1;
        } else {
            closed.push(s);
        }
    }
    if (ww == 1) {
        System.out.println();
    } else if (!closed.isEmpty()) {
        System.out.println("incorrect espression");
    } else {
        System.out.println("correct");
    }

}
  }

May I suggest a simpler solution:
You need only a counter. it is incremented by 1 whenever you encounter an openning parenthesis "(" and is decreased by 1 whenever you encounter a closing parenthesis ")"
The input is invalid if the counter drops below zero or is not zero by the end of scanning.

EDIT
since OP specified they must use a stack, we can follow @JollyJoker's advice: counter increament is implemented by a push operation, counter decrement by a pop one.  counter falling below zero is implemented by attemt to pop when the stack is empty. by the end of scanning the stack has to be empty.


Counting words in files I made a simple program for my study that calculates words in a text files and prints every word and its repeated times in the files.

How can I improve this code?

#include <future>
#include <iostream>
#include <fstream>
#include <unordered_map>
#include <string>
#include <vector>
#include <algorithm>
#include <exception>

typedef std::unordered_map<std::string, std::size_t> Words;

Words wordsInFile(const std::string& fileName)
{
    std::ifstream file(fileName);
    if (!file.is_open())
    {
        throw "Can't open the file" + fileName;
    }

    Words loadFromFile;

    for (std::string word; file >> word;)
    {
        ++loadFromFile[word];
    }

    return loadFromFile;
}

int main(int argc, char *argv[])
{
    std::vector<std::future<Words>> futures;

    for (int i = 1; i < argc; ++i)
    {
        futures.push_back(std::async([=] { return wordsInFile(argv[i]); }));
    }

    Words words;

    for (auto& i : futures)
    {
        const auto results = i.get();

        for (const auto& j : results)
        {
            words[j.first] = j.second;
        }
    }

    std::cout << "Word\tRepeated Times\n-------------------------\n";
    for (const auto& i : words)
        std::cout << i.first << "\t\t" << i.second << '\n';
}

test.txt

File system is huge subject need more work out. file is plain text file
This is some junk words. simple program for words counting in text file. 1 2 3
2 2 2 2
@ % & ^ *
all so good
this test text file

Output:

Word    Repeated Times
-------------------------
File            1
out.            1
1               1
system          1
2               5
is              3
need            1
huge            1
subject         1
work            1
more            1
%               1
file            3
plain           1
text            3
This            1
some            1
junk            1
program         1
words.          1
simple          1
for             1
words           1
counting                1
in              1
file.           1
so              1
3               1
@               1
&               1
^               1
this            1
*               1
all             1
good            1
test            1

I see some things that may help you improve your code.

Simplify your code

You don't need a lambda at all.  Use this instead:

    futures.push_back(std::async(wordsInFile,argv[i]));

Also, instead of if (!file.is_open()) use the more idiomatic if (!file).

Fix the counting

If we have two files which both contain the word fox exactly once then I would expect the result of this program to print fox   2 but in fact, it would falsely claim a count of 1.  The problem is this line:

words[j.first] = j.second;

This overwrites the count instead of accumulating it.  This is what you need instead:

words[j.first] += j.second;

Use reserve to prevent reallocations

This code already knows how many futures are going to be created.  The code can be made a little more efficient by using this:

futures.reserve(argc-1);

With large numbers of files, this eliminates the re-allocation that would be required by dynamically resizing the vector.

Check for exceptions

If any of the file names can't be read.  That is, if it can be opened but not read, as with a directory under Linux, this code will throw an error that isn't caught and so therefore results in a crash.  Since future::get() will throw any error that was stored, it's that part of the code that should check for exceptions.  One alternative would be this:

    Words results;
    try {
        results = i.get();
    }
    catch(std::exception &err)
    {
        std::cerr << "ERROR: " << err.what() << '\n';
    }
    catch(std::string &err)
    {
        std::cerr << "ERROR: " << err << '\n';
    }
    for (const auto& j : results)
    {
        words[j.first] += j.second;
    }

Note that this will simply skip the offending file name but continue to process the rest.

Measure your results

Create a non-parallel version of this code and compare the timing to see if it's faster or slower than this version.  I have found that it's often true that the overhead of the implicit (as in async) or explicit (as in thread) thread creation swamps the savings in time by making things parallel.  The only way to know for sure is to time it.


Active Directory security groups Search application optimization I have a Web API method that returns the  Active Directory security groups for the specific login user. The below code was working fine but it was taking so munch time nearly 45 sec to get the results.

DirectoryEntry root = GetDirectoryEntry()
  using (var groups = root.Children.Find("OU=Sample Security Groups"))
                        {
                            using (var directory = groups.Children.Find("OU=Permissions"))
                            {
                                using (var searcher = new DirectorySearcher(directory))
                                {
                                    searcher.Filter = `filter condition`
                                    var results = searcher.FindAll();
                                    foreach (SearchResult result in results)
                                    {
                                        if (result != null)
                                        {
                                            using (DirectoryEntry group = result.GetDirectoryEntry())
                                            {
                                                items.Add((string)group.Properties["sAMAccountName"].Value);
                                            }
                                        }
                                    }
                                }
                            }
                        }

Update:  In the GetDirectoryEntry method we are creating the connection to the active directory using the username and password.

Can anyone help to optimize the code using Parallel.ForEach or threading etc..

Let us first review the code.

Stacking usings will reduce the level of indentation.
The Filter of the DirectorySearcher can be passed to the constructor
If the expected result of the FindAll() call will be huge, you should consider to return only needed properties, by using this overloaded constructor of the DirectorySearcher class.
The SearchResultCollection returned by DirectorySearcher.FindAll() implements the IDisposable interface hence it should be enclosed in a using as well. The remarks section of the documentation states:

  Due to implementation restrictions, the SearchResultCollection class
  cannot release all of its unmanaged resources when it is garbage
  collected. To prevent a memory leak, you must call the Dispose method
  when the SearchResultCollection object is no longer needed.

Implementing some of this changes could look like so

DirectoryEntry root = GetDirectoryEntry();
using (var groups = root.Children.Find("OU=Sample Security Groups"))
using (var directory = groups.Children.Find("OU=Permissions"))
using (var searcher = new DirectorySearcher(directory, "filter condition"))
using (var results = searcher.FindAll())
{
    foreach (SearchResult result in results)
    {
        if (result != null)
        {
            using (var group = result.GetDirectoryEntry())
            {
                items.Add((string)group.Properties["sAMAccountName"].Value);
            }
        }
    }
}

If you only need to get the SamAccountName's of the groups a specific user is a member of you can use UserPrincipal.FindByIdentity() like so

UserPrincipal user = UserPrincipal.FindByIdentity(new PrincipalContext(ContextType.Domain, "domain", "username", "password"), IdentityType.SamAccountName, "loginUser");

foreach (GroupPrincipal group in user.GetGroups())
{
    Console.Out.WriteLine(group.SamAccountName);
}


Transparent wrapper class for data structure in Python I needed a deque who's maxlen can be set again after it has been initialized. So, since new-style classes in Python are treated as types, I decided to create a new class borrowing from deque to accomplish this. My approach is to have an internal deque as an attribute and when maxlen is set, it replaces the internal deque with a new one, initialized to the old contents and the new maxlen.

I tried subclassing deque but it's too crude, creating a deque that's useless on top with the useful one internally. So I chose to simply have all of the wrapper class's attributes (except special ones) point to the internal deque's. However, due to how new-style classes work, the special  attributes have to be handled manually. Since all I want is to override maxlen's setter, all of this seems inelegant and I'm wondering if there's a cleaner way of accomplishing this.

From what I've read here, it seems I could subclass this class in __new__ to skip overriding the special attributes, but that seems even more hairy than what I already wrote.

This is the result, stripped of extraneous comments (complete code here if you want something runnable, with tests):

# -*- coding: utf-8 -*-

from __future__ import print_function

from collections import deque

class ResizableDeque(object):
    def __init__(self, *args, **kwargs):
        self.internal = deque(*args, **kwargs)

        skip_list = [
            'maxlen'
        ] + [attr for attr in dir(deque) if attr.startswith('__') and
             attr.endswith('__')]

        for attr in dir(deque):
            if attr not in skip_list:
                setattr(self, attr, getattr(self.internal, attr))

    @property
    def maxlen(self):
        return self.internal.maxlen

    @maxlen.setter
    def maxlen(self, value):
        templist = list(self.internal)
        self.internal = deque(templist, value)

    def __str__(self):
        return self.internal.__str__()

    def __repr__(self):
        return self.internal.__repr__()

    def __getitem__(self, value):
        return self.internal.__getitem__(value)

    def __setitem__(self, index, value):
        return self.internal.__setitem__(index, value)

    # these have not been tested
    def __copy__(self):
        return self.internal.__copy__()

    def __delitem__(self, index):
        return self.internal.__delitem__(index)

    def __iadd__(self, other):
        return self.internal.__iadd__(other)

    def __len__(self):
        return self.internal.__len__()

    # not sure if overriding __sizeof__ is wise this way
    def __sizeof__(self):
        return self.__sizeof__() + self.internal.__sizeof__()

    # pretty sure this is ok
    def __format__(self, spec):
        return self.internal.__format__(spec)

Correctness?

I copied your code, and added the following testcase.

if __name__ == "__main__":
    k = ResizableDeque(maxlen=10)
    k.append(12)
    print(k)
    k.maxlen = 10
    k.append(13)
    k += [14]
    print(k)

Expected result:

deque([12], maxlen=10)
deque([12, 13, 14], maxlen=10)

But I got

deque([12], maxlen=10)
deque([12, 14], maxlen=10)

The reason is that you copy over a bunch of items in __init__, but not in the maxlen.setter.

Whitelisting vs blacklisting

In __init__ you copy several items over using a skip_list. Why not explicitly define the items you want to copy over instead?

Actually, why copy at all? Performance would be a proper reason, but then I'd suggest copying just the items that have proven to be necessary to copy (after profiling).


Printing the largest sum from integers in a file I am writing a program for learning purposes that takes an input of a file structured similarly to:

13,22,13,14,31,22, 3, 1,12,10
11, 4,23, 7, 5, 1, 9,33,11,10
40,19,17,23, 2,43,35,21, 4,34
30,25,16,12,11, 9,87,45, 3, 1
1,2,3,4,5,6,7,8,9,10

Which will then read the values from the file, figure out and print the largest sum you can make from the digits on each line which is 50 or less (also cannot be the same number used twice).

I think I've come to a good solution, but there are a few issues. I want to try and separate the code into separate functions. I'm additionally looking for code optimization advice.

#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[]) {
  FILE *fp;
  int hv, lv,line,i, linecount,val[5][10]; //hv + lv = the two highest values on a line, line=sum of hv and lv.
  int j=0;

  // test file succesfully opened
  if((fp=fopen(argv[1], "r")) == NULL){
    printf("Cannot open file.\n");
    exit(1);
  }

  //Store values in 2d array
  for(i=0;!feof(fp);i++){
    while(j<10){
      fscanf(fp, "%d,",&val[i][j++]);
    }
  j=0;
  }
  linecount=i-1; //set linecoutn to No of lines

//test and print result
 for(i=0; i<=linecount; i++){ // for each line
   hv=0, lv=0, line=0; //reset values for each line
   for(j=0;j<10;j++){ // for each value
     for(int a =0; a<10; a++) { //test against all in line
       if(a!=j && (val[i][j]+val[i][a]<=50)){ //if two values arent equal and sum is less than 50
     if((val[i][j]+val[i][a])>line){ //if value is greater than previous value
       hv=val[i][j];
       lv=val[i][a];
       line=hv+lv;
     }
       }
     }
   }
   printf("Line %d: largest pair is %d and %d, with a total of %d\n", i+1, hv, lv, line);
 }

 fclose(fp);
}

Brad, you code compiles cleanly, which is a good sign :-)  Clearly it could be split into functions. An obvious approach would be to split the reading of the values from the processing of those values.

I would take a slightly different approach, but remember there is no one "correct" solution. I would read and process the file
one line at time.  So the pseudo code would be:

    for each line
        read the line data  (function)
        find greatest sum in line  (function)
        store sum if bigger than max value found

This approach avoids the need for a multi-dimensional array and means you
  can handle an arbitrary number of lines.

Note that nested loops are often not desirable, but are sometime necessary.  Finding the greatest sum in the line looks like a case where a nested loop is optimal.  But this double loop should be a function.

Also check the return value from fscanf.  If you do this you can avoid processing a blank line.

A few comments on notation:

Be consistent!!!  This is important although it might seem trivial.  Many people
reading your code and seeing some places where a comma is followed by a
space (preferred) and some where it is not will judge the code badly.  Similarly,
comments should be consistent - follow // by a space.
Defining just one variable per line is usually preferred.
Leave a space after keywords (if, else, for, while etc). Similarly,
leave a space before and after = (or != etc). (these are my preferences, others might differ).
Many people, including me, prefer code and comments to extend to no more than 80
columns.
Send error messages to stderr and on error from system and library calls
that set errno (the global error variable) use perror (which prints to
stderr).
Exit on failure with exit(EXIT_FAILURE);
On completion, return a value from main, normally return EXIT_SUCCESS
Oh and finally, delete the useless comments. Nearly all of your comments are worse than pointless - they add noise, not information.


matplotlib get_legend_handles_labels always return empty I have a problem when adding elements to the same figure.
The problem is with the legend.
At each iteration I add elements and the corresponding legend.
But I want the legend to include all the different elements from all iterations.

The problem is that the function get_legend_handles_labels() returns empty handles & labels lists.

The code is as follows:

    handles, labels = ax.get_legend_handles_labels()
    for key in legendMap.iterkeys():
        if key not in labels:
            handles.append(h[legendMap[key]])
            labels.append(key)

    ax.legend(handles, labels, loc='center left', bbox_to_anchor=(1, 0.5))
    h1,l1 = ax.get_legend_handles_labels()

h1, l1 are empty rather being the same as handles, labels

The following code does the trick, but I believe there should be more elegant solution:

    legend = ax.get_legend()
    labels = [] if legend is None else [str(x._text) for x in legend.texts]
    handles = [] if legend is None else legend.legendHandles
    for key in legendMap.iterkeys():
        if key not in labels:
            handles.append(h[legendMap[key]])
            labels.append(key)

    ax.legend(handles, labels, loc='center left', bbox_to_anchor=(1, 0.5))

Help is much appreciated, thanks !

When you adding curves, bar to your chart specify label, like:

ax.plot(x, y, linewidth=1.75, color='#00ff00', label='curve1')

After that everything works like a charm.


Arduino weather station logger I made a program that uses the Adafruit pressure sensor, and the DHT22 temperature and humidity sensor to log pressure, temperature and humidity every 30 minutes. I left it outside my house overnight yesterday in the hopes that it would have logged all the data overnight but it had logged only once. I'm guessing it drained the whole 9V battery in 30 minutes due to some inefficiency in my program? The program works when the sampling rate is at 2 seconds or something small like that. Please tell me how to make the program more efficient so it can do the logging every 30 minutes successfully.

    #include "DHT.h"
    #define DHTPIN 3
    #define DHTTYPE 22

    DHT dht (DHTPIN,DHTTYPE);

#include <SD.h>
#include <SPI.h>

#include "Wire.h"
#include "Adafruit_BMP085.h"

Adafruit_BMP085 mySensor;
File myData;

int cs = 4;//chipSelect for SD reader/Writer
double tempC;//holds temperature reading (Celcius)
double pressure;//holds pressure reading (Pascals)
double humidity;
double DHTtemp;
double finalTemp;

void setup() {
  // put your setup code here, to run once:
  dht.begin();
  SD.begin(cs); //initialize cardReader/Writer
  mySensor.begin();//initialize pressure sensor
  pinMode(10,OUTPUT);//the SD library requires pin 10 to be reserved as OUTPUT and not to be used.

  myData=SD.open("PTDATA.txt",FILE_WRITE);
  myData.println("Logging start at  8:30PM, logging 30-minute intervals");
  myData.println("Time,Temperature,Pressure,Humidity");
  myData.close();

}

void loop() {
//   put your main code here, to run repeatedly:
  myData=SD.open("RLDATA.txt",FILE_WRITE);

  if(myData)
  {
  tempC=mySensor.readTemperature();//read Temperature
  pressure=mySensor.readPressure();//read Pressure
  humidity=dht.readHumidity();
  DHTtemp=dht.readTemperature();//read temp from DHT Sensor

  myData.print(tempC);
  myData.print(",");
  myData.print(pressure);
  myData.print(",");
  myData.println(humidity);
  myData.close();
  delay(1800000);//30 min delay;

  }

 }

Rather than use the Arduino delay feature, use a watchdog timer to turn the Arduino on/off to take readings.  The TI TPL5010 should serve well. http://www.digikey.com/product-detail/en/texas-instruments/TPL5010DDCT/296-38829-2-ND/5130142


Passing a password to a viewmodel I've got a project I've been working on that connect to a file server and writes some data to a text file. In order to do this I added a textbox for the user to type their username and a passwordbox well for the password. In my models I have an interface called IPassword that looks like the following:

public interface IPassword
{
    SecureString Password { get; }
}

Using MVVM Light I created a RelayCommand and tied it to a button. When the button is clicked it calls the function ExportList (I've removed any code relating to my file server location):

    public void ExportList(object parameter)
    {
        var passwordContainer = parameter as IPassword;
        NetworkCredential credentials;
        string folder = String.Empty;

        if (passwordContainer != null && IsEnabled == true)
        {
            var securePassword = passwordContainer.Password;
            var password = ConvertToUnsecureString(securePassword);

            credentials = new NetworkCredential();
            credentials.UserName = Username;
            credentials.Password = password;
            credentials.Domain = "domain name";

            folder = "folder location";
            WriteToNetworkFolder(folder, credentials);
        }

        else
        {
            folder = "local folder location";
            WriteToLocalFolder(folder);
        }
    }

    private string ConvertToUnsecureString(SecureString password)
    {
        if (password == null)
        {
            return String.Empty;
        }

        IntPtr unmanagedString = IntPtr.Zero;
        try
        {
            unmanagedString = Marshal.SecureStringToGlobalAllocUnicode(password);
            return Marshal.PtrToStringUni(unmanagedString);
        }

        finally
        {
            Marshal.ZeroFreeGlobalAllocUnicode(unmanagedString);
        }
    }

    private void WriteToNetworkFolder(string folder, NetworkCredential credentials)
    {
        // Open the connection to the server
        using (new NetworkConnection(folder, credentials))
        {
            foreach (string location in LocationsPicked)
            {
                string fileName = folder + "\\" + location + ".txt";

                // Write out the lines to each text file
                try
                {
                    using (StreamWriter writer = new StreamWriter(fileName, true))
                    {
                        for (int i = 0; i < address.PhysicalAddresses.Count; i++)
                        {
                            writer.WriteLine(address.PhysicalAddresses[i]);
                        }
                    }
                }

                catch (IOException ioe)
                {
                    Console.WriteLine("The file was not written.");
                    Console.WriteLine(ioe.Message);
                    Console.WriteLine(ioe.StackTrace);
                }
            }

            UpdateStatusBar("Success! Your MAC address(es) were successfully exported!");
        }
    }

The main reason I did it this way was because you cannot use databinding to get the password from a passwordbox due to security reasons. Is this the best way to handle this situation? If not, then how would you recommend getting the password?

Why don't you just use the overloaded constructor of the NetworkCredential class which takes the password as SecureString?

In this way you can remove the ConvertToUnsecureString() method at all and you don't have to read the decrypted value of the password.

It is always recommended to use System.IO.Path.Combine() instead of adding up the path by using string concatenation. In this way you will be safe regarding illegal characters in the path, missing \ etc.


Converting duplicate JS code into a higher order function I have a block of code that is re-used and I want to use functional programming to remove this duplication.
My code takes an array of items, splits the items into batches of 500 and then does some kind of work on them.
In the first function it deletes items from a database:
Delete function:
const deleteDocuments = async (documentReferences) => {
   const batchesOf500 = Math.ceil(documentReferences.length / 500);

   for(let batchNumber = 0; batchNumber < batchesOf500; batchNumber += 1) {
      const batch = getBatchWriter();
      const startingIndex = batchNumber * 500;
      const maxIndex = startingIndex + 500;

      for(let index = startingIndex; index < maxIndex; index += 1) {
         if(index < documentReferences.length) {
            const documentPath = documentReferences[index];
            batch.delete(documentPath);
         }
      }

      await batch.commit();
   }
}

The second function is almost identical but instead of deleting from a database, it writes to the database:
Add function:
const writeToCollection = async (dataArray, collectionRef) => {
   const batchesOf500 = Math.ceil(dataArray.length / 500);

   for(let batchNumber = 0; batchNumber < batchesOf500; batchNumber += 1) {
      const batch = getBatchWriter();
      const startingIndex = batchNumber * 500;
      const maxIndex = startingIndex + 500;

      for(let index = startingIndex; index < maxIndex; index += 1) {
            if(index < dataArray.length) {
               const [key, value] = dataArray[index];
               const doc = getDocFromPath(key);
               batch.set(doc, value);
            }
         }
      }

      await batch.commit();
   }
}

These functions are almost identical, so I have write a higher order function to do most of the leg-work.
Higher order function:
const runFunctionInBatchesOf500 = (func, dataArray) => {
   const batchesOf500 = Math.ceil(dataArray.length / 500);

   for(let batchNumber = 0; batchNumber < batchesOf500; batchNumber += 1) {
      const batch = this.firestore.batch();
      const startingIndex = batchNumber * 500;
      const maxIndex = startingIndex + 500;

      for(let index = startingIndex; index < maxIndex; index += 1) {
         const document = dataArray[index];
         func(document, batch);
      }
   }

   await batch.commit();
}

And to it you can create your own functionality to apply to each document and use it like this:
const write = (document, batch) => {
   const doc = getDocFromPath(key);
   batch.set(doc, value);
};

await runFunctionInBatchesOf500(write, dataArray);

This all works but I think I am missing something. Is this an efficient use of higher order functions? What would a more elegant, FP-style solution be?

From a short review;

Why hardcode the batch length to 500 ?
Why not have the batch length be a nice constant ?
You have even hard coded the length in the function name, which is really unfortunate
batchNumber++ is more canonical than batchNumber += 1
I would have gone for maxIndex = Math.min(startingIndex + 500, dataArray.length); because now you have a lot of calls to func with undefined as a document value
await requires runFunctionInBatchesOf500 to be async (it is missing now)
I would use Array.prototype.slice() to create batches as an array, and then use forEach on each slice/batch
const doc = getDocFromPath(key); <- where does key come from, an evil global?

I personally would be mildly evil by adjusting the Array prototype so that I can keep chaining, FP style;
Array.prototype.mapSlice = function arrrayMapSlice(n){
  //Just return `this` if we get a non-sensical parameter
  if(isNaN(n) || n <= 0){
    return this;
  }
  let start = 0, out = [];
  while(start < this.length){
    out.push(this.slice(start, start+=n));
  }
  return out;
}

async function runBatches(list, f, batchSize){
   batchSize = batchSize || 500;
   list.mapSlice(batchSize).forEach(batch => {
     const firestoreBatch = this.firestore.batch();
     batch.forEach(document => f(document, firestoreBatch ));
   });
   await batch.commit();
}


Installing charts in parallel using go routines I use the following code which works and install helm chart (in the loop) in parallel,
it get a kubeconfig file (to access to k8s) create a helm client and install the charts from the defined location and waits (upgradeAction.Wait = true) that each chart installation will finished successfully.
At first I do it in sequence and now I added a wait.group to be able to reduce the installation time and install it in parallel, do I miss something? error handling etc?
    package main

    import (
        "fmt"
        "os"
        "strings"
        "sync"
    )

    func main() {
      log.Info("tools Installation")

    kcfgFilePath, err := kubeFile(cfg)
    defer os.Remove(kcfgFilePath)
    if err != nil {
        return err
    }
    settings := cli.New()
    actionConfig := new(action.Configuration)
    clientGetter := genericclioptions.NewConfigFlags(false)
    clientGetter.KubeConfig = &kcfgFilePath
    // install all relevant components
    var wg sync.WaitGroup

    for _, chartInstallation := range charts {
        wg.Add(1)
         go installChart(&wg,chartInstallation, releaseName, actionConfig, clientGetter, settings, log)
    }
    return nil
}

func installChart(wg *sync.WaitGroup,chartInstallation Installation, releaseName string, actionConfig *action.Configuration, clientGetter *genericclioptions.ConfigFlags, settings *cli.EnvSettings, log logr.Logger) (error, bool) {
    defer wg.Done()
    chart, err := loader.Load(chartInstallation.Path)
    if err != nil {
        return err, true
    }
    releaseName = releaseName + "-" + chartInstallation.Name
    if err := actionConfig.Init(clientGetter, settings.Namespace(), os.Getenv("HELM_DRIVER"), func(format string, v ...interface{}) {
        r := fmt.Sprintf(format, v)
        log.Info("Helm Installation", "deploy status", r)
    }); err != nil {
        return err, true
    }
    releasePresent := true
    status, err := action.NewStatus(actionConfig).Run(releaseName)
    if err != nil {
        //if errors.Is(err, driver.ErrReleaseNotFound) {
        if strings.Contains(err.Error(), driver.ErrReleaseNotFound.Error()) {
            releasePresent = false
        } else {
            return err, true
        }
    }

    if !releasePresent {
        // install chart
        installAction := action.NewInstall(actionConfig)
        installAction.CreateNamespace = true
        installAction.Namespace = chartInstallation.Namespace
        installAction.ReleaseName = releaseName

        installAction.Wait = true
        // install new
        _, err := installAction.Run(chart, nil)
        if err != nil {
            return err, true
        }
        log.Info("installed: ", "releaseName", releaseName)
    }

    if status != nil {
        if releasePresent && status.Info.Status.String() == release.StatusFailed.String() { // upgrade if broken
            upgradeAction := action.NewUpgrade(actionConfig)
            upgradeAction.Atomic = false
            upgradeAction.CleanupOnFail = true
            upgradeAction.Wait = true
            upgradeAction.ReuseValues = false
            upgradeAction.Recreate = false
            _, err := upgradeAction.Run(releaseName, chart, nil)
            if err != nil {
                return err, true
            }
        }
    }
    return nil, false
}

The code is ignoring the return values from the go routine
The WorkGroup isn't used to block until completion

// 1. Perhaps you'd want to have a separate channel to communicate the errors.
var errCh := make(chan error, len(charts))
for _, chartInstallation := range charts {
    wg.Add(1)
    go func(errCh chan error){
       installed, err := installChart(&wg,chartInstallation, releaseName, actionConfig, clientGetter, settings, log)
       if err != nil {
           errCh <- err
       }
    }(errCh)
}
// 2. Wait for the wait group to complete
wg.Wait()
// 1. Collect the errors
for err := range errCh {
    fmt.Println("Error: ", err)
}


Bit rotations exercise I'm studying C on K&R and I solved exercise 2.08:

  Write a function rightrot(x,n) that returns the value of the integer x rotated to the right by n positions

I've tested my code with some bit patterns and it seems to work, but I'm not sure that this solution covers all possible cases.

What do you think about this code?

unsigned rightrot(unsigned x, int n)
{
    int size;
    unsigned y;

    size = 0;
    y = x;

    while (y != 0) {
    y = y << BYTESIZE;
    size++;
    }
    size = size * BYTESIZE;

    return (x << (size-n)) | (x >> n);
}

Here is the main

#include <stdio.h>

#define BYTESIZE 8

unsigned rightrot(unsigned x, int n);

int main(void)
{
     unsigned x;
     int n;

     x = 0x23acb;
     n = 2;

     printf("%x\n", rightrot(x, n));

     return 0;
}

Isn't it something like this?

#include <limits.h> /* for CHAR_BIT */

unsigned
rotate_right(unsigned x, int n)
{
    int left_shift = ((sizeof x) * CHAR_BIT) - n;
    return x<<left_shift | x>>n;
}


Roughly removing 'duplicate' photos based on filename I quickly wanted a script that'll go through a lot of photos in a folder and only keep one photo per minute (gotta love burst shots on iPhone).

Fortunately, Dropbox names them with a timestamp upon import: I was thus allowed to base this one on the filename (an example filename is '2013-11-06 23.32.19 XXXXXX.jpg').

I was thinking of maybe moving these files to a 'duplicate' folder or something, just didn't have the time to figure it out for now. I only just started Python, and it was definitely fun to have such a tiny assignment done in under 10 minutes :)

Any comments on how I could've done this in a more pretty way is appreciated!

import os
directory = "{pathToPhotos}"

dt_counter = {}
for filename in os.listdir(directory):
    if not filename.endswith('.ini'):
        dt_rounded = filename[:16]
        if dt_rounded in dt_counter.keys():
            dt_counter[dt_rounded] += 1
        else:
            dt_counter[dt_rounded] = 1
        if dt_counter[dt_rounded] > 1:
            os.remove(os.path.join(directory, filename))

Additionally, I was thinking of expanding this due to the many 'duplicate' photos I have (same setting, different photo's) by grouping them in 10 second buckets and creating a GUI or so to show all alternatives and choose one to keep, any tips on where to start with this would also be greatly appreciated!

If you're only dealing with only .jpg files then it will be better to use the glob module:

import glob

for filepath in glob.iglob(os.path.join(directory), '*.jpg'):
   ...

Don't use dict.keys() to check for the existence of a key in dictionary, it is inefficient in Python 2(created a whole list first and then searches for key in that list) and requires an unnecessary call to .keys() in Python 3. Or even better use collections.Counter to keep counts of objects:

from collections import Counter

dt_counter = Counter()

for filepath in glob.iglob(os.path.join(directory), '*.jpg'):
    dt_rounded = os.path.split(filepath)[1][:16]
    dt_counter[dt_rounded] += 1
    if dt_counter[dt_rounded] > 1:
         # if `directory` is actually a `pathToPhotos` then you
         # don't need to call os.path.join anymore
         os.remove(filepath)


Spreading out model across multiple classes a good idea? Background

My project performs software application management; it covers in-app purchasing, product provisioning, account management, etc. I've recently been tasked to add a cost reporting component.

Code

On the code level, I've spread out my model into a few classes that represent categories or interest areas, each requiring specific dependencies. Here's a rough outline:

models/base.class.php

namespace Models;

class Base
{
    protected $db; // PDO

    public function __construct(PDO $db)
    {
    }

    // helper functions such as data mappers for one or multiple rows
}

models/ordermodel.class.php

namespace Models;

class OrderModel extends Base
{
    protected $gateway; // payment gateway to handle purchases

    public function __construct(PDO $db, Gateway $gateway)
    {
        parent::__construct($db);
        $this->gateway = $gateway;
    }

    // methods for purchase, refund, subscription cancellation, etc.
    public function updateInstallDetails(...);

    public function refundPayment(...);
}

models/provisionmodel.class.php

namespace Models;

class ProvisionModel extends Base
{
    protected $account; // user account to "anchor" queries against

    public function __construct(PDO $db, Entities\Account $account)
    {
        parent::__construct($db);
        $this->account = $account;
    }

    // this method gets called when a software update took place
    // it's called (indirectly) from a controller class
    private function updateInstallDetails(...);
}

The individual classes are accessed via a factory:

class Model
{
    private static $repository = array();

    /**
     * Generic instance loader
     *
     * @param $name string
     * @param $fn callback generates the object instance
     */
    private static function getModelInstance($name, $fn)
    {
        if (!isset(self::$repository[$name])) {
            self::$repository[$name] = $fn();
        }
        return self::$repository[$name];
    }

    public static function getOrderModel($gateway = null)
    {
        // TODO pass payment gateway object
        return self::getModelInstance(__FUNCTION__, function() use ($gateway) {
            if ($gateway) {
                $gateway = GatewayFactory::load($gateway);
            }
            return new Models\OrderModel(SiteConfig::getDefaultDatabase(), $gateway);
        });
    }
}

Problem

The cost reporting component cross-cuts two main areas of the model, i.e. purchases, refunds and provisioning (shown above). Purchases and refunds of any product are recorded for royalty payments and provisioning keeps track of software updates (between versions there may be added or removed royalties).

My possible solutions

So I have a few options:

Make both my classes aware of the reporting unit; this means extending the constructor and what not.
Apply Decorator pattern to "intercept" method calls.
Apply ObserverMediator pattern; each class will emit "interesting" events and the reporting unit will listen for them and make the appropriate changes to the database tables.

Questions

Does it make sense to spread out my model into separate classes in the first place? Would it make more sense to make one big class that encompasses all my application logic?

ObserverMediator pattern has its pros and cons; the advantage is that you can easily decouple units from each other, but the downside is that it's indeed decoupled. Are there other cons?

Is there another pattern I could apply to crack this? Is there some other major issue with my code?

Model classes

Model classes should not have any hard dependency in their constructors especially not a PDO or similar stuff. Model can not have any business rule they are containing only data and that's all. Ofcourse if you have some mapping between model classes (Author <--> Books) then some of the relation informations have to be in the model (Mediator!) classes but that's all.

Model classes should not have any knowledge about their storage type. It can be anything not just az SQL database handled with PDO. They can not have self managed CRUD methods (if they does then they become Active Record(s) which is a really bad thing (~God object), antipattern) (maybe factory methods are okay but these methods also can not have anything heavy (query database through a horrible static global accessible thing) inside).

CRUD

Mapped entities should have some kind of top level storage (like in .NET an ObjectContext) which is dealing the storage problem; handling SQL connections and providing transaction capabilities with an Unit of Work (Commit, Rollback) solution. The top level storage can have for example mapped SQL tables for querying from the underlaying storage, inserting into it, deleting from it. Other thing is that this top level storage is the responsible to track the changes in the entities. Of course we have to put some stuff in every model in the system (Mediator #2!) to handle this session but the main logic still in a state manager not in the models.

Repository

Repositories are tricky. With powerful ORM systems we can forget them if the business logic doesn't require another application level. What should a repository contain? Add, Delete, Query methods but no Save() or anything else (not even named query methods like: GetUserByEmailAddress($email)). You can use DAO classes for named queries which are operating one or more repository or on the entire object tree. Do not forget: not every entity has a repository! Only aggregate roots can have repository, for example: an Order model can have a repo but an OrderDetail can not becouse it belongs to an Order model and can not exists whithout it.

Static

In your example your are using in static context object which are containing state (your repository stuff) information which is bad. You will introduce a global state whith them which will bring more harm to your code then good (global hyperspace is inreliable, anyone can modify it).


Operating sublists efficiently In my Java program, I need to operate on the sublists a of ArrayList or LinkedList often, like removing a sublist, comparing two sublists.

For ArrayList or LinkedList, I didn't find any good APIs to do these. My current implementation is as below. Basically, it takes an input like:

u1234 u1236 u1236 u2def u1236 u1236 u2def

and outputs the following:

u1234 ( ( u1236 ) * u2def ) *

Core function:

ArrayList<String> toRegex(ArrayList<String> tokenArray)
{
    /* check different length of continuous duplication */
    for(int len=1; len<=tokenArray.size()/2; len++)
    {
        boolean match = false;
        /* given a length, scan for duplication */
        for(int i=0; i<tokenArray.size()-(2*len-1); i++)
        {
            ArrayList<String> first = new ArrayList<String>();
            ArrayList<String> second = new ArrayList<String>();
            for(int j=i; j<i+len; j++)
                first.add(tokenArray.get(j));
            for(int j=i+len; j<i+2*len; j++)
                second.add(tokenArray.get(j));

            if(isSpecial(first)) {       /* skip the cases */
                continue;
            }

            while(isIdentical(first, second))
            {
                match = true;
                for(int j=i+len; j<i+2*len; j++)
                    tokenArray.remove(i+len);
                if(i+2*len > tokenArray.size())
                    break;
                second.clear();
                for(int j=i+len; j<i+2*len; j++)
                    second.add(tokenArray.get(j));
            }

            if(match == true) {
                tokenArray.add(i, "(");
                tokenArray.add(i+1+len, ")");
                tokenArray.add(i+2+len, "*");
                i = i+3+len;
                match = false;
            }
       }
    }

    return tokenArray;
}

Currently, the performance is not good. Could you find the bad design or inappropriate usage of data structures/APIs? What are the good common ways of operating sublists?

Yes, this doesn't look pretty. Consider converting the list to sets or using RegEx.

ArrayList<String> A = new ArrayList<String>();
A.add("Z");
A.add("Z");
A.add("C");
A.add("X");
A.add("Z");

Set<String> set = new HashSet<String>(A);

for(String temp:set)
    System.out.println(temp);

Sublists:

List<String> subList = alist.subList(2, 4);

Use retainAll to get intersections:

listOne.retainAll(listTwo) ;
boolean areEqualNotSamePosition = listeOne.size();


Using HikariCP and JDBC to store data for a Minecraft plugin I'm creating a plugin for the game Minecraft, and with it I'm using HikariCP and MySQL to store data between server restarts. I've created working code but I'm wanting to improve it.

While the code works fine, I feel there's a simpler way to structure it. I appreciate any suggestions.

HikariFacade.java

public class HikariFacade {

private final HikariConfig hikariConfig;
private final HikariDataSource hikariDataSource;

public HikariFacade(HikariConfig hikariConfig) {
    this.hikariConfig = hikariConfig;
    hikariDataSource = new HikariDataSource(hikariConfig);
}

/**
 * Get a connection from the DataSource
 *
 * @return A connection, or null if one couldn't be obtained
 */
public Connection getConnection() {
    Connection connection = null;
    try {
        connection = hikariDataSource.getConnection();
    } catch (SQLException e) {
        e.printStackTrace();
    }
    return connection;
}

/**
 * Create a PreparedStatement
 *
 * @param sql The SQL for the PreparedStatment
 * @return A prepared statement
 * @throws SQLException
 */
public PreparedStatement createPreparedStatement(String sql) throws SQLException {
    return getConnection().prepareStatement(sql);
}

/**
 * Run an update, then close the resources
 *
 * @param ps Run a PreparedStatement as an update
 * @return The number of rows effected
 * @throws SQLException
 */
public int update(PreparedStatement ps) throws SQLException {
    int i = ps.executeUpdate();
    close(ps.getConnection(), ps, null);
    return i;
}

/**
 * Run a query, then close the resources
 *
 * @param ps Run a PreparedStatement as a query
 * @return The ResultSet from the query
 * @throws SQLException
 */
public ResultSet query(PreparedStatement ps) throws SQLException {
    ResultSet rs = ps.executeQuery();
    close(ps.getConnection(), ps, rs);
    return rs;
}

/**
 * Close a set of database related objects
 *
 * @param connection A Connection to close
 * @param statement  A Statement to close
 * @param resultSet  A ResultSet to close
 */
public static void close(Connection connection, Statement statement, ResultSet resultSet) {
    try {
        if (connection != null) {
            connection.close();
        }
        if (statement != null) {
            statement.close();
        }
        if (resultSet != null) {
            resultSet.close();
        }
    } catch (SQLException e) {
        e.printStackTrace();
    }
}

public HikariDataSource getHikariDataSource() {
    return hikariDataSource;
}
}

DatabaseManager.java

public abstract class DatabaseManager {

protected HikariFacade hikariFacade;

public DatabaseManager(HikariFacade hikariFacade) {
    this.hikariFacade = hikariFacade;
}

}

TownDatabaseManager.java

public class TownDatabaseManager extends DatabaseManager {

public TownDatabaseManager(HikariFacade hikariFacade) {
    super(hikariFacade);
}

public int create(Town town) {
    String sql = "INSERT INTO towns VALUES (?);";
    try {
        PreparedStatement ps = hikariFacade.createPreparedStatement(sql);
        ps.setString(1, String.valueOf(town.getUuid()));
        return hikariFacade.update(ps);
    } catch (SQLException e) {
        e.printStackTrace();
    }
    return 0;
}

public List<Town> loadTowns() {
    String sql = "SELECT * FROM towns;";
    try {
        PreparedStatement ps = hikariFacade.createPreparedStatement(sql);
        ResultSet rs = hikariFacade.query(ps);
        List<Town> towns = new ArrayList<>();
        while (rs.next()) {
            UUID townUuid = UUID.fromString(rs.getString("town_uuid"));
            towns.add(new Town(townUuid));
        }
        return towns;
    } catch (SQLException e) {
        e.printStackTrace();
    }
    return null;
}

}

Here's how this is all used in the end:

Main.java

private void testDb() {
    Town newTown = new Town(UUID.randomUUID());

    int i = new TownDatabaseManager(getHikariFacade()).create(newTown);

    new TownDatabaseManager(getHikariFacade()).loadTowns();

}

Something which occurs to me is that catching the exception and not rethrowing is a very bad practice. Printing the stack trace is helpful, it tells the consumer of your API that something went wrong, but it is inflexible (it cannot be redirected to another IO device) and, more importantly the failure itself is swallowed. The program keeps on running after an exception, but there is no fallback behavior and the situation seems critical. Consider rethrowing the exception or, better yet, removing the try...catch construct entirely.