Proper usage of OOP PHP I am new to the OOP world I have been reading as much as I can about it and have never been more confused. I understand that its great for organizing code and making it more maintainable, etc. I have written some OOP code but I am unsure if its proper, it works fine though.

I am confused about public private functions along with extends and constructors. I haven't used them yet in my code and do not see where I would use them in a real life example. The more I read and try to understand it the more confused I get. I have included my code and was wondering if someone could point out mistakes, improvements, proper usage, organization. I feel it would help me better understand if someone with a lot of experience was to look at my code and give me pointers.

class userFunctions {

  const SALT_LENGTH = 9;

  //Retrieves encrypted password from database returns in variable $salt
  public function retrievePassword($conn,$username) {
    try{
      $stmt = $conn->prepare('SELECT `password` FROM `users` WHERE `userName`= :userName');
      $stmt->bindValue(':userName', $username);
      $stmt->execute();
      $salt = $stmt->fetchColumn();

        } catch (PDOException $e){
            echo 'Connection failed: ' . $e->getMessage();
        }
            return $salt;
    }

  //Generate an encrypted password method
  public function generateHash($password, $salt = null) {
    if ($salt === null) {
      $salt = substr(md5(uniqid(rand(), true)), 0, self::SALT_LENGTH);
    } else {
      $salt = substr($salt, 0, SALT_LENGTH);
    }
    return $salt . sha1($salt . $password);
 }

//Check database for duplicate username
  public function userCheck($conn,$userName) {
    try{
      $stmt = $conn->prepare('SELECT COUNT(*) FROM `users` WHERE `userName` LIKE CONCAT("%",:userName)');
      $stmt->bindValue(':userName', $userName);
      $stmt->execute();
      $count = $stmt->fetchColumn();
      return $count;
    } catch (PDOException $e){
      echo 'Connection failed: ' . $e->getMessage();
    }
  }

  //Add user to database
  public function Register($conn,$userName,$encryptedPass) {
    try{
      $stmt = $conn->prepare('INSERT INTO users (userName, password) VALUES (:userName, :password)');
      $stmt->execute(array(':userName' => strip_tags($userName), ':password' => $encryptedPass));
    }  catch (PDOException $e){
      echo 'Connection failed: ' . $e->getMessage();
    }
  }
}

Right, your object is, like Rene said, just a container of functions. Though, in theory, that's what objects are for (group functionality together), the way you went about this, you're actually using classes as modules. It's not what you call SOLID code.

Edit:
Perhaps you might want to scroll down first, because throughout the answer I've used rather messy examples to explain things, but at the bottom I've added a really simple example that might help you to understand the access modifiers and inheritance better.

Anyhow, I'm not going into details on how you should refactor this to be SOLID compliant, that's part of the learning curve. The simple fact of the matter is that, yes OOP is confusing at first, and at first you might not even need constructors, access modifiers (public, protected or private), let alone destructors. Especially if you haven't come to terms with inheritance. But let me explain a few things that, hopefully, will clarify one or two things.

Class methods share state:
That's just a fancy way of saying that class methods have access to variables, without having them passed as arguments. Of course, not just any variable will do: it's the properties of the class that are accessible. In your snippet, you're passing $conn to various methods, like you would if they were functions. The difference is, that classes can be assigned properties, too.

class RefactoredFunctions
{
    private $conn = null;
    public function setConn($conn)
    {
        $this->conn = $conn;
    }
    public function retrievePassword($userNAme)
    {
        //your code here, but replace $conn with $this->conn
    }
}

Now you can call the retrievePassword method, without having to pass the $conn param over and over again. Though, this code would fail, if $conn wasn't set prior to calling retrievePassword. How can we fix that?
Enter constructors. A constructor is a method, like any other, except for the fact that it can't be called manually. It's called automatically, when an instance of the class is created. You could define a constructor that expects an argument, and pass the connection to it, ensuring the $conn property will be set:

public function __constructor($conn)
{
    $this->conn = $conn;
}
//using this class:
$instance = new RefactoredFunctions($conn);
$instance->retrievePassword('userFoo');//conn is set through constructor!

That's all a constructor does (9/10 times): setting properties your class requires to work properly. These properties are sometimes called the dependencies of that class. The class depends on them.
Now, an argument that is called $conn is clearly supposed to be a DB connection in your case, but how do we make sure nobody passes a string? Simple: we use type-hinting, another benefit of OOP:

public function __construct(PDO $conn)
{
    $this->conn = $conn;
}
//usage
$instance= new RefactoredFunctions(new PDO());//works fine
$error = new RefactoredFunctions('foobar');//fatal error string is not PDO instance

This makes your code more failsafe. Use type-hints to avoid bugs. That's an order!
Now, all this time, I've been using $this->conn which is a property declared as private. Why? Simple. I've used type-hints to ensure the value of $this->conn is guaranteed to be an instance of PDO, but if I would have declared public $conn, I'm able to access the property (and thus assign it a new value) simply bypassing the setter/constructor:

//assume public $conn
$instance= new RefactoredFunctions(new PDO());//works fine
$instance->retrievePassword('userFoo');//still OK
$instance->conn = 'reassign conn';
$instance->retrievePassword('userFoo');//ERROR!

Now, with private properties:

$instance= new RefactoredFunctions(new PDO());//works fine
$instance->conn = 'reassign conn';//ERROR!!
$instance->retrievePassword('userFoo');//will never get this far

What's the benefit of this? Simple: the bug is easy to find. Imagine if I would've reassinged the conn property by accident, and then call a method 50 lines further down the script, or pass an instance to another class (defined in another file), and call the method there. I'd get an error, with a file & line number that contains the call, not the cause (reassignment statement).

OK, you got this far, and this is still making sense? On to protected... Where does that tie in?
Well, the more objects you have, chances are some of them share functionality. Rather than writing the same methods several times, we extend classes from each other. So far, we have:

class RefactoredFunctions
{
    private $conn = null;
    public function __construct(PDO $conn)
    {
        $this->conn = $conn;
    }
    public function setConn(PDO $conn)
    {
        $this->conn = $conn;
        return $this;
    }
    public function retrievePassword($userNAme)
    {
        //your code here, but replace $conn with $this->conn
    }
}

Now, suppose I wanted a DB class per table (quite common). I have this base class, that holds a connection instance, and some methods to query a DB. What if I moved the constructor, $conn property and setConnection method to a generic class, and create child classes per table? that would save me 2 methods/class to write. If I have 10 tables, that's 20 methods. Of course I'll do that:

class BaseTable
{
    private $conn = null;
    private $tblName = null;//to hold the table their linked to

    public function __construct(PDO $conn)
    {
        $this->conn = $conn;
    }

    public function setConn(PDO $conn)
    {
        $this->conn = $conn;
        return $this;
    }

    public function setTable($tblName)
    {//set table name for each instance
        $this->tblName = $tblName;
        return $this;
    }
}
//a child
class UserTbl extends BaseTable
{
    public function getUsers()
    {
        $this->conn->prepare('SELECT * FROM '.$this->table);
        //etc...
    }
}
//usage:
$users = new UserTbl(new PDO());
$users->getUsers();

Ok, so I didn't have to write those properties and methods, but the code above won't work! Why? because private means the properties/methods are only visible to the class in which they were defined, in this case BaseTable, not the child class. protected means that properties/methods can be accessed in both the class that defined them, and its children. So, would you make the $conn property protected? Of course NOT.
There's a number of reasons why you wouldn't do this, but I might go into those in a future edit. ATM, we have some more important things to focus on.

Have you noticed how the connection is guaranteed to be available thanks to the constructor, but the tablename isn't? yet, we're using the table name in our queries. How do we fix that? You have 3 options:

Make $tblName protected and assign it a value in the child's definition, and remove the setTable method
override the parent constructor (less secure)
A combination of the first two

So, if in the base class, we now have protected $tblName = null; you can write this, in the child:

protected $tblName = 'users';

Job done. The second approach would look like this:

//in child
public function __construct(PDO $connection, $tblName)
{
    $this->tblName = $tblName;
    parent::__construct($connection);//explicit call required
}

To combine the two would require using abstract, final and all that, here's an example of this, but I'm not going into all that now.

All this time, we still have to deal with the private $conn being invisible to the child classes. How would you deal with that? The answer to that question is surprizingly simple:

//in BaseTable
protected function getConnection()
{
    return $this->conn;
}

This protected method is visible to the children, and returns the connection, so all we have to do now is write:

$this->getConnection()->prepare('SELECT ...');

instead of $this->conn. There we are, problem solved, you should be able to get these classes working now.
Note that there's a lot of things left to be done. For example the getUsers methods is nothing but a select * from tbl query. We could just as well have written that in our BaseTable class, but using another name and access modifier:

protected function getAll()
{//protected: it's for internal use only
    $res = $this->conn('SELECT * FROM '.$this->tblName);
    return $res->fetchAll();
}

Then, in each child class:

//UsersTable:
public function getUsers()
{
    return $this->getAll();
}
//LogsTable
public function getLogs()
{
    return $this->getAll();
}

You could even add arguments in the child methods, and process the return value of the getAll method accordingly:

protected function getAll()
{
    return $this->conn->query('SELECT * FROM '.$this->table);
}
//UsersTable:
public function getUsers($asArray = false)
{
    $asArray = $asArray ? PDO::FETCH_ASSOC : PDO::FETCH_OBJ;
    return $this->getAll()->fetchAll($asArray);
}

One last thought on inheritance: Assume the classes BasteTable and UserTable extends BaseTable. When using type-hinting, this is good to know:

$base = new BaseTable();
$user = new UserTable();
if ($base instanceof BaseTable) echo 'Yes';//will echo, of course
if ($user instanceof UserTable) echo 'Duh';//of course, this is true

That's as you'd expect it, but:

if ($base instanceof UserTable) echo 'WTF';//You won't see this
if ($user instanceof BaseTable) echo 'Yes';//will echo!!

So all instances of UserTable are instances of BaseTable, too, but not all instances of BaseTable are instances of UserTable. Now this might seem odd, at first but think of it like this:

class Room
{
    public $door;//can be opened from outside
    protected $window;//can only be opened from within ANY room, all rooms have to access this
}
class Bathroom extends Room
{
    private $mirror;//this room has a mirror, can only be accessed in a bathroom
}
class Bedroom extends Room
{
    private $bed;//this one has a bed, can only be used when inside a bedroom
}

Now you see: both a bedroom and a bathroom are rooms, but a bedroom is no bathroom, hence:

$bath = new Bathroom;//instanceof Room, not instanceof Bedroom
$bed = new Bedroom;//instanceof Room, not instanceof Bathroom
$genericRoom = new Room;//instanceof Room
//not specified which, so no mirror or bed, so not instance of any child

I hope this clears one or two things up for you. If you have a follow up question, don't hesitate to ask. I'm not going to add any more info, as this is quite a lot to deal with already


php inside html indent style This is actually a question that bugged me for 8 years or so, but just swept it under the rug.

I have a project (big, old, zend 1, etc.) that contains a decent (not high but large enough) amount of logic in views. This implies existence of conditionals there. It does not have any template system (like smarty/twig) so php is used for that.

Now... my problem is how to indent (since current indentation just hurts my eyes).

Currently is like this:

  <?php if ($this->product->getType() == Product_Types::BOOK) {
       $bookAuthor = $this->product->getAuthor();
  ?>

<?php echo $bookAuthor; ?>

  <?php } ?>

Basically the indentation is correct, but totally useless since I can't easily see that Some output is actually inside the if statement.

Personally I prefer this approach (and used it for years, but now I decided I need a second opinion):

  <?php
     if ($this->product->getType() == Product_Types::BOOK) {
        $bookAuthor = $this->product->getAuthor();
        ?>

              <?php echo $bookAuthor; ?>

        <?php
     }
  ?>

This is not actually that correct since the <?php ?> tags don't open/close on same space. It basically creates an "pseodo-block" ?> <?php. BUT it helps me see exactly what's where. You can see from first glance that the div belongs in the if... and I think that is actually the purpose of indenting.

How do you guys think is better? (other variants are encouraged)

The second one is definitely better. In the first one, it's really hard to see that the div is actually inside the if.

Personally, I would build the string completely in PHP, and then echo it, instead of switching in and out of the HTML context. This will avoid the opening and closing PHP tags being littered around your code:

$html = "";
if ($this->product->getType() == Product_Types::BOOK) {
    $html += "<div>" . $this->product->getAuthor() . "</div>";
}
echo $html;


Find available ports on localhost I'm writing code to find the available ports on 127.0.0.1. I'd like it to be faster, but it's functional now.

def find_open_ports():
    for port in xrange(1,8081):
        sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        res = sock.connect_ex(('localhost', port))
        if res == 0:
            yield port
        sock.close()

and it is called with available_ports = list(find_open_ports()).

There is no error handling because this will be running server-side. If localhost can't be found, there are bigger problems than failing a port scan.

Two comments:

The existing code only checks for free ports under 8081.  The full range is 0-65535.
You're only checking the IPv4 definition of 'localhost'.  We live in the future! IPv6 calls localhost ::1.  At first blush, this would seem to not matter - something's either listening or not, right? - but it turns out that firewalls sometimes have different rulesets for IPv4 vs IPv6, so one may get through but the other not.  You didn't specify what this code was for, so it's just a caveat to keep in mind.


Manipulate XML files in c# I just finished my working code, but still want to improve it.

I want to transform this input:

<item>asdf</item>
<item>asdf</item>
<item>asdf</item>

to this output:

<string name="x1">asdf</string>
<string name="x2">asdf</string>
<string name="x3">asdf</string>

My current code:

   static void Main(string[] args)
        {
            String content;

            using (StreamReader reader = new StreamReader("arrays2.xml"))
            {
                content = reader.ReadToEnd();
            }

            int counter = 0;
            int startIndex = 0;
            while ((startIndex = content.IndexOf("<item>", startIndex)) != -1)
            {
                counter++;
                content = content.Substring(0, startIndex) + "<string name=\"rule" + counter + "\">" + content.Substring(startIndex + 6);
            }

            content = content.Replace("</item>", "</string>");

            using (StreamWriter writer = new StreamWriter("arrays2_formatted.xml"))
            {
                writer.Write(content);
            }
            Console.WriteLine(content);

        }

I think the bottleneck is in the content assignment within the loop, as a lot of String instance are created and thrown away.

However, is there a completely different way to do this efficiently?

Don't parse it through string manipulation, use the XML libraries in the framework.  I'd recommend LINQ to XML.  Parse it and make your changes.

var xmlStr = @"<root>
<item>asdf</item>
<item>asdf</item>
<item>asdf</item>
</root>";

var doc = XDocument.Parse(xmlStr);         // parse from string
//var doc = XDocument.Load("arrays2.xml"); // or load from file
var items = doc.Descendants("item");       // find all item elements
var index = 1;
foreach (var item in items.ToList())  // ToList() required since we're modifying the document
{
    var name = "x" + index++;
    var nameAttr = new XAttribute("name", name);           // create the name attribute
    var value = (string)item;                              // read the value as a string
    var newNode = new XElement("string", nameAttr, value); // create the new element
    item.ReplaceWith(newNode);                             // replace the current one
}
//doc.Save("arrays2_formatted.xml"); // save the changes to the file


Simple command line todo-list Introduction

I've written a simple command line todo-list in Haskell.

The full code can be found here. However, given people's time constraints, I have selected three verbose functions for review.

Function One

program :: StateT TodoList IO ()
program = do
    StateT $ \xs -> do
        (choice,_) <- runStateT requestChoice xs
        (_,xs') <- runStateT (parseChoice choice) xs
        runStateT printList xs'
        runStateT program xs'

main = runStateT program []

Questions:

Am I repeating myself too much by extracting the monads using runStateT?
Is there a better way to structure the main program?

Function Two

parseChoice :: Text -> StateT TodoList IO ()
parseChoice choice
    | first == "add" = addItem $ TodoItem second third
    | first == "remove" = removeItem $ parseInt second
    | first == "save" = saveList second
    | first == "load" = loadList second
    | first == "quit" = StateT $ \_ -> exitSuccess
    | otherwise = invalidChoice first
    where   args =  split (=='|') (toLower choice)
        first = head args
        second = if length args > 1 then args !! 1 else ""
        third = if length args > 2 then args !! 2 else ""

Questions:

Am I using too many guards?

Function Three

removeItem :: Maybe Int -> StateT TodoList IO ()
removeItem mn = StateT remove
    where remove = \xs -> case mn of
        Just n -> if n <= length xs - 1
            then return ((),removeAt n xs)
            else do
                S.putStrLn "Index too large"
                return ((),xs)
        Nothing -> do
            S.putStrLn "Invalid index entered"
            return ((),xs)

General Questions

Which parts are un-idiomatic?
Which parts are superfluous?
Should I decompose these longer functions into smaller functions that do less?
How else can I improve this code?

It seems like you may not quite grasp the interplay between do-notation and monad transformer stacks. Take a look at how I've rewritten program here to leverage the actual machinery of StateT. The version you wrote is needlessly verbose due to your manually plumbing the state around!

program :: StateT TodoList IO ()
program = do
    choice <- requestChoice
    parseChoice choice
    printList
    program

parseChoice can be cleaned up by favoring pattern matching over guards. Whenever you see a wall of guards that depend only on Eq, consider pattern matching instead.

parseChoice :: Text -> StateT TodoList IO ()
parseChoice choice =
    case split (== '|') (toLower choice) of
        ["add", date, message] -> addItem $ TodoItem date message
        ["remove", index]      -> removeItem $ parseInt index
        ["save", file]         -> saveList file
        ["load", file]         -> loadList file
        ["quit"]               -> lift exitSuccess
        (invalid:_)            -> invalidChoice invalid

I think you can probably guess what can change about removeItem after reading my other changes now, so before reading this next code block try rewriting it on your own.

removeItem :: Maybe Int -> StateT TodoList IO ()
removeItem Nothing  = lift $ putStrLn "Invalid index entered"
removeItem (Just n) = modify (removeAt n)


Exporting site and address data These two methods do very similar things. Is it possible to condense these somehow?  This is using the Entity Framework.

public void ExportSiteData (FLEX_INV_EXP_SITE siteData)
{
    var uniqueSite = from e in _context.FLEX_INV_EXP_SITE
                     where e.SITE_ID == siteData.SITE_ID
                     select e;
    var count = uniqueSite.Count();

    if (count != 0) return;
    _context.FLEX_INV_EXP_SITE.AddObject(siteData);
    _context.SaveChanges();
}

public void ExportAddressData (FLEX_INV_EXP_ADDRESS addressData)
{
    var uniqueSite = from e in _context.FLEX_INV_EXP_ADDRESS
                     where e.SITE_ID == addressData.SITE_ID
                     select e;
    var count = uniqueSite.Count();

    if (count != 0) return;
    _context.FLEX_INV_EXP_ADDRESS.AddObject(addressData);
    _context.SaveChanges();
}

It seems the only two unique things about the blocks of code are the Tables that you are searching/adding to and how the SITE_IDs are obtained (from different types).  You could just make your method accept two additional parameters, the table that is being searched and a lambda which selects the SITE_ID.  You'll just have to rewrite some expressions.

public static void ExportData<TEntity, TSite>(
    ObjectSet<TEntity> table,
    Expression<Func<TEntity, TSite>> siteIdSelector,
    TEntity data)
    where TEntity : class
{
    var condition = GetCondition(siteIdSelector, data);
    var isInTable = table.Where(condition).Any();
    if (!isInTable)
    {
        table.AddObject(data);
        table.Context.SaveChanges();
    }
}

private static Expression<Func<TEntity, bool>> GetCondition<TEntity, TKey>(
    Expression<Func<TEntity, TKey>> keySelector,
    TEntity data)
    where TEntity : class
{
    var entity = Expression.Parameter(typeof(TEntity), "entity");
    var entityKey = BindParameter(keySelector, entity);
    var dataKey = BindParameter(keySelector, Expression.Constant(data));
    var body = Expression.Equal(entityKey , dataKey);
    return Expression.Lambda<Func<TEntity, bool>>(body, entity);
}

private static Expression BindParameter<TEntity, TKey>(
    Expression<Func<TEntity, TKey>> keySelector,
    Expression entity)
    where TEntity : class
{
    return new SubstitutionVisitor
    {
        OldExpr = keySelector.Parameters.Single(),
        NewExpr = entity,
    }.Visit(keySelector.Body);
}

public class SubstitutionVisitor : ExpressionVisitor
{
    public Expression OldExpr { get; set; }
    public Expression NewExpr { get; set; }

    public override Expression Visit(Expression node)
    {
        return (node == OldExpr) ? NewExpr : base.Visit(node);
    }
}

Then to use it, you could do something like this:

ExportData(_context.FLEX_INV_EXP_SITE, entity => entity.SITE_ID, siteData);
ExportData(_context.FLEX_INV_EXP_ADDRESS, entity => entity.SITE_ID, addressData);


Implementing a singly linked list in C++ I'm really just looking for some advice on my current code and with moving forward. As you can see, I have a pointer to my student list and data (which just haven't been made yet).

#include<iostream>
#include<cstdio>
#include<cstdlib>

using namespace std;

class List{ //Building the base of the program, being the nodes, and initializing the functions of the list
    private:
        typedef struct node {
            int student_number;
            int class_number;
            struct node* classpointer;
            struct node* studentpointer;
        }* nodePtr; //condensing node structure to simply "nodePtr"

        nodePtr head;
        nodePtr curr;
        nodePtr temp;

    //functions that I will be using
    public:
        List();
        void addNode();
        void PrintList();
};
//initialize the list pointers
List::List() {
    head = NULL;
    curr = NULL;
    temp = NULL;
}
//function to add a node to the linked list
void List::addNode() {
    nodePtr n = new node;
    n->classpointer = NULL;

    cout << "What value would you like to add?" << endl;
        int x;
    cin >> x;
    n->class_number = x;

    if(head != NULL) {
        curr = head;
        while (curr->classpointer != NULL) {
            curr = curr->classpointer;
        }
        curr->classpointer = n;
    }
    else {
        head = n;
    }
}

void List::PrintList() {
    curr = head;
    while(curr != NULL) {
        cout << " " << endl;
        cout << curr->class_number << endl;
        curr = curr->classpointer;
    }
}

int menu() {
    int input;
    List List;
    while (input != 3) {
        cout << " " << endl;
        cout << "Press '1' to input a node" << endl;
        cout << "Press '2' to view the list of nodes" << endl;
        cout << "Press '3' to exit" << endl;
        cout << " " << endl;
        cin >> input;
        if (input == 1) {
        List.addNode();
        }
        else if (input == 2) {
        List.PrintList();
        }
        else if (input == 3) {
            return 0;
        }
        else {
        cout <<"That is an invalid key" << endl;
        }
    }
}

int main() {
    int i;
    for (i=0; i < 500; i++) {
        menu();
    }
}

Unnecessary member variables

You have a member variable curr that has no purpose as a member variable, because it is only used as a temporary pointer to iterate through your list.  You should make curr simply be a local variable in the functions it is used in.

The member variable temp is never used and should be deleted.

\$O(n)\$ add

Your addNode function takes linear time because it has to find the end of the list.  You should either add to the beginning of the list, or keep a tail pointer so that you can quickly add to the tail.


Storing fiscal year DateTime We have conception as "fiscal year" and have calculations based on start/end date (April 1, March 31).

My question is: what is better, store end date time as April 1 or March 31?
Now I have next code:

public DateTime GetFiscalYearStart(int fiscalYear)
{
    return new DateTime(fiscalYear, 4, 1);
}

public DateTime GetFiscalYearEnd(int fiscalYear)
{
    return new DateTime(++fiscalYear, 4, 1).AddTicks(-1);
}

It doesn't really matter.

  My question is: what is better, store end date time as April 1 or March 31?

This is completely up to you based on what you want:

How do you prefer to check your date bounds?

Inclusive? if(myDate <= myFiscalYear.GetFiscalYearEnd())
Exclusive? if(myDate < myFiscalYear.GetFiscalYearEnd())

Is this end date shown to the user? Which date do they prefer seeing?

On a technical level, it doesn't matter which you pick. All that matters is that you handle the value consistently. If you start mixing < and <= checks, you're going to run into problems and bugs regardless of whether you define your date inclusive or exclusive.

Taking it a bit further.

However, as I pointed out, the inconsistent use of < and <= is going to be a possible cause for bugs down the line. Your developers might mistakenly use one instead of the other.

The issue here is that external code will handle the comparison. However, if you wrap this in a class of your own, you can ensure that the evaluation only occurs in one place, which dramatically lowers the chance of developer error.

A simple example class:

public class FiscalYear
{
    private int fiscalYear;

    public DateTime Start => new DateTime(fiscalYear, 4, 1);
    public DateTime End   => new DateTime(fiscalYear+1, 4, 1).AddTicks(-1);

    public FiscalYear(int year)
    {
        this.fiscalYear = year;
    }

    public bool Contains(DateTime date)
    {
        return date >= this.Start && date <= this.End;
    }
}

  Note
  There are other possible variations on this. You could calculate the dates in the constructor, or you could calculate the end date based on the start date. How you define the values is completely up to you.

  Edit remark:
  It's still possible for developers to implement their own evalutation based on the Start/End properties, but that is then considered developer error, they should rely on the logic that already exists instead of reinventing the wheel.
  What you could also do is set Start and End to private. This way, you prevent external code from doing their own evaluation, they can only do new FiscalYear(2018).Contains(myDate), but then you do lose out on using FiscalYear as an easy way for your external callers to find out what the start and end date of a fiscal year are. That's up to you to decide.

External code can then rely on the existing Contains() method; which means that the developers of the external code no longer need to know whether they should use < or <=.


Simplify 'if' condition How can I simplify this code?

  if (directory.Exists)
  {
    smallFileNames = directory.GetFiles("*.csv").Select(i => i.FullName).ToList();
    if(smallFileNames.Count == 0)
      smallFileNames = DivideIntoFiles(fileName);
  }
  else
    smallFileNames = DivideIntoFiles(fileName);

I confused by identical lines...

Jumping in after the horse has bolted with a very minor different approach (using Any rather than Count and null).

IEnumerable<string> GetSmallFileNames(Directory directory, string fileName, string filter)
{
   var smallFileNames = new List<string>();

   if (directory.Exists)
   {
       smallFileNames = directory.GetFiles(filter).Select(i => i.FullName);
   }

   return smallFileNames.Any() ? smallFileNames : DivideIntoFiles(fileName);
}

Then used such as

IEnumerable<string> smallFileNames = GetSmallFileNames(directory, fileName, "*.csv");


URL resolver for Users, Lessons, and Series My Resolver.resolve class returns a hash of objects and I would like to return a specific object if it exists. Is there a more concise way to write this?

def resolve_url(url)
    # Resolver library now returns a hash of objects
    # Using this helper method to preserve the old functionality of returning a single object
    # Order of old return types is Lesson -> Series -> User
    objects = Resolver.resolve(url, :only => [User, Lesson, Series])
    if objects[Lesson]
      objects[Lesson]
    elsif objects[Series]
      objects[Series]
    elsif objects[User]
      objects[User]
    else
      nil
    end
  end
end

I might as well post this then:

def resolve_url(url)
    # Resolver library now returns a hash of objects
    # Using this helper method to preserve the old functionality of returning a single object
    # Order of old return types is Lesson -> Series -> User
    only = [Lesson, Series, User]
    objects = Resolver.resolve(url, :only => only)
    objects.values_at(*only).compact[0]
  end
end

So first get only the necessary fields (reusing the list from the resolve call), remove nils with compact and finally return the first value; if it's empty you still get nil back.

I've also removed my comment since you've updated your post.  FWIW you could also use something like objects[Lesson] or objects[Series] or Objects[User] instead.


Function to return the sum of multiples within a range Project Euler problem 1 says:

  Find the sum of all the multiples of 3 or 5 below 1000.

I have written a program that will return the sum of the multiples of any list of positive integers. So for example if I wanted to know the sum of every integer that was a multiple of 3 and 5 that is less than 10 I would add 3,5,6, and 9 to get 23.

My function can do this for any number of positive integers, between any range that starts from 1.

# Function to return sum of multiples that are within a certain range
def MultiplesSum(multiple, end, start=1):
    mSum = 0
    naturals = range(start, end)
    offset = -start
    if start == 0:
      raise ZeroDivisionError('Cannot start with the number 0')
    for num in multiple:
      for i in naturals:
        if i % num == 0 and naturals[i+offset] != 0:
          # print mSum,"+", i,"=",i+mSum
          mSum += i
          naturals[i+offset] = 0
        i += 1
    return mSum

problem1 = MultiplesSum([3,5,2], 16)
print problem1
# prints 88

Simpler implementation

You don't need naturals at all, and offset either.
You could make the outer loop iterate over the range(start, end),
and for each value check if it can be divided by any one of nums,
and add to the sum.

msum = 0
for i in range(start, end):
    for num in nums:
        if i % num == 0:
            msum += i
            break
return msum

API

The interface of your function is not intuitive:

The range start parameter goes after the range end parameter. That's not intuitive. Consider how Python's range works:

With one arg, the first arg is the end of the range
With two args, the first arg is the start, the second is the end of the range. It's natural that way

I suggest to follow that example
The first arg is called "multiple", singular. It's not a multiple. It's a collection of numbers. So nums would be a better name, and it would make it easier for readers to understand how to use this function

Input validation

In this part:

if start == 0:
  raise ZeroDivisionError('Cannot start with the number 0')

The zero division error is inappropriate. You did not divide by zero,
and it won't make sense for callers to catch this exception and handle it.
In other languages this should be an IllegalArgumentException, in Python ValueError would be more appropriate.
The real problem is not division by zero, but inappropriate use of the function.

But then, why is start=0 inappropriate? I see nothing wrong with that.
What would be wrong is having 0 in nums.
That's what you should validate instead.

Coding style

The recommended practice is to use snake_case for function names, not CamelCase. See more coding style recommendations in PEP8.


Extracting Pixels From Image Byte[] I have a requirement where I need to extract pixels of a given rectangle within an image byte[].

More specifically I have an image where I have narrowed an area that I wish to return and keep and the remainder discard. In my case I'm performing face recognition in real time so I don't want to store unnecessary data but obviously this could be useful elsewhere.

public static byte[] ExtractPixelsFromImage(this byte[] pixels, int top, int left, int width, int height, int horizontalResolution)
{
    List<byte> result = new List<byte>();

    int bytesPerPixel = 4;
    int rowLength = horizontalResolution * bytesPerPixel;
    int rowLeftPixelStart = (rowLength * top) + (left * bytesPerPixel);

    for (int i = 0; i < height; i++)
    {
        int currentPixelStart = (rowLength * i) + rowLeftPixelStart;
        result.AddRange(pixels.Skip(currentPixelStart).Take(width * bytesPerPixel));
    }

    return result.ToArray();

}

This method has been responsible for the vast amount of CPU time my application is using and I'm hoping to improve this.

The obvious suggestion is to remove the LINQ, but beyond this, is there anything else that can be improved?

At a quick glance it would appear that your code is likely getting slowed down by the use of Linq as well as conversion between List<byte> and byte[]. Only profiling and determining a reasonable amount of time for this code to execute is going to tell you the real answer, but here are some things you can try:

Don't use Skip and Take. Both of these methods operate on IEnumerable, meaning Skip may iterate from 0-currentPixelStart despite the fact that the array can be accessed by index directly. It's possible that this optimization is done by these extension methods, but some quick searching seems to indicate it is not (and you don't want to rely on it's implementation sticking that way).
Don't use List<byte>. For starters, one of the benefits of using a List is that it can dynamically grow based on elements that are added. In most cases it does pretty well managing the overhead of resizing the internal collection, but if you need the absolute best performance you can just use a byte array. You can already calculate how big the resulting byte array will be, so just allocate the proper size from the start. This also means avoiding the result.ToArray() call since result will already be an array.
Make bytesPerPixel a const. This one isn't likely to gain you anything noticeable in the performance department, but other methods could make use of this value and right now it's hidden away in this method.
Don't recalculate the new row size on every height iteration (ie: move width * bytesPerPixel outside the loop since it won't change).


Find the closest unvisited visible home I'm writing a 2d top down game where I perform a lot of "closest" searches between one point and all the rest of the points.

In this example, "me" wants to search for the closest visible house (Home) that I haven't visited yet, and if it is not found, return null.

protected Home getClosestUnvisitedVisibleHome(Person me, final int VISIBLE_ZONE) {
    List<Home> visitedHomes = me.getVisitedHomes();
    Map<Home, Integer> homeDistances = new HashMap<>();
    List<Home> allHomes = getHomes();
    allHomes.stream().parallel().forEach(home -> homeDistances.put(home, range(me, home)));
    List<Home> unvisitedVisibleHomes = allHomes.stream().parallel().filter(
            home -> !visitedHomes.contains(home) && homeDistances.get(home) <= VISIBLE_ZONE
    ).collect(Collectors.toList());
    unvisitedVisibleHomes.sort((home1, home2) -> homeDistances.get(home1).compareTo(homeDistances.get(home2)));
    try {
        return unvisitedVisibleHomes.get(0);
    } catch (IndexOutOfBoundsException e) {
        return null;
    }
}

protected int range(VisibleEntity visibleEntity, VisibleEntity me) {
    return (int)Math.sqrt(Math.pow((visibleEntity.xy[0] - me.xy[0]), 2) + Math.pow((visibleEntity.xy[1] - me.xy[1]), 2));
}

I'm looking for suggestions on how to optimize this particular function and or any suggestions of its content.

I don't see why you use a HashMap (or any Map) at all.

What I'd do is:

Get all the homes
Filter all the homes to only get those which are unvisited
Get the home that is closest

The home that you get will either be visible, or it won't be visible. If it is not visible, there won't be any other visible homes either.

Combining this with @Vogel's suggestion of using Set the code would go something like this:

Set<Home> visitedHomes = me.getVisitedHomes();
Set<Home> allHomes = getHomes();
Optional<Home> result = allHomes.stream().parallel()
    .filter(home -> !visitedHomes.contains(home))
    .min(Comparator.comparingInt(home -> range(me, home)))
    .map(result -> range(me, result) <= VISIBLE_ZONE ? result : null);

And then you can choose between either returning the optional:

return result;

or returning null if there is no match:

return result.orElse(null);


Determine whether a checkbox is checked or unchecked I have a jQuery "checkbox checked/unchecked" function that works well. This is a checkbox for turning a particular URL parameter on or off.  But I believe this code could be written a lot tighter. Does anyone have any suggestions?

$('#mapControl').live('click', function(){
var thisUrl = $(location).attr('href');
if($(this).is(':checked')) {
    var lastFour = thisUrl.substr(thisUrl.length - 4);
    var param;
    if (lastFour == 'com/') {param='?mapControl=true'} else {param='&mapControl=true'}
    thisUrl=thisUrl+param;
} else {
    $('#urlParam').val(thisUrl);
    if (thisUrl.indexOf('?mapControl=true') >= 0){
        thisUrl=thisUrl.replace('?mapControl=true','');
    } else if (thisUrl.indexOf('&mapControl=true') >= 0){
        thisUrl=thisUrl.replace('&mapControl=true','');
    }
}
$('#urlParam').val(thisUrl);
});

I was given some suggestions (because i erroneously posted on StackOverflow) and was asked to post my question here. So with the feedback I got - I ended up with this code.

    function getParam() {
    var n;
    var myUrl = window.location.href;
    var lastFour = myUrl.substr(myUrl.length - 4);
    if (lastFour == 'com/') {n='?'} else {n='&'}
    return n;
}

$('#mapControl').on('click', function(){
    var thisUrl = window.location.href;
    var urlParamObj = $('#urlParam');
    if(this.checked === true) {
        var lastFour = thisUrl.substr(thisUrl.length - 4);
        var param = getParam();
        thisUrl=thisUrl+param+'mapControl=true';
    } else {
        urlParamObj.val(thisUrl);
        thisUrl=thisUrl.replace('?mapControl=true','').replace('&mapControl=true','');
    }
    urlParamObj.val(thisUrl);
});

I think this is the tightest it could be and I can use the getParam function for other checkboxes.

This is tighter I think:

function addParameter( url , name , value )
{
  var justHostname = ( location.href == ( location.protocol + "//" + location.host + "/" ),
      separator = justHostname?"?":"&"
  return url + separator + encodeURIComponent(name) + "=" + encodeURIComponent(value);
}

$('#mapControl').on('click', function(){
    var url = window.location.href,
        urlParamObj = $('#urlParam');
    if(this.checked)
    {
      url = addParameter( url , 'mapControl' , 'true' )
    } else {
      url = url.replace('?mapControl=true&','?')
               .replace('?mapControl=true','')
               .replace('&mapControl=true','');
    }
    urlParamObj.val(thisUrl);
});

I did not test this, but you should catch the drift.

If it were me though, I would not re-invent the wheel and use the code from the answer here :
https://stackoverflow.com/questions/6953944/how-to-add-parameters-to-a-url-that-already-contains-other-parameters-and-maybe


polymorphism with object ArrayList I've an ArrayList of Object that could contain String, Date and Long. The Object list is nearly mandatory. (I obtain this list through reflection, reading all the member variables of an unknown class.)

    ArrayList <Object> values = new ArrayList<Object>();

I build dynamically a query for Alfresco cmis from it:

        addObjectToQueryStatement(qs, i+1, values.get(i));

I came only with this idea:

private void addObjectToQueryStatement(QueryStatement qs, int parameterIndex, Object value)
{
    if (value instanceof String)
    {
        qs.setString(parameterIndex, (String) value);
    }
    else if (value instanceof Date)
    {
        qs.setDateTime(parameterIndex, (Date)value);
    }
    else if (value instanceof Long)
    {
        qs.setNumber(parameterIndex, (Long)value);
    }
}

Does it exist a better and more elegant implementation?

This is missing the point of polymorphism. Its great strength is that you don't need to know or care about the type. With your code, you still need to check type using the instanceof operator.

One way to resolve this is using a variation on the command pattern. First, define a common interface (I used an abstract class) that all three implementations can inherit from. Let call it QueryParameter:

abstract class QueryParameter {
    protected final int index;

    public QueryParameter(int index) {
        this.index = index;
    }

    public abstract void addTo(QueryStatement queryStatement);

}

Now you define three implementations. Here is the String version:

class StringParameter extends QueryParameter {
    private final String value;

    public StringParameter(String value, int index) {
        super(index);
        this.value = value;
    }

    @Override
    public void addTo(QueryStatement queryStatement) {
        queryStatement.setString(index, value);
    }

}

Now, you may construct an arbitrary number of parameters and save them in a collection. Once you've got your QueryStatement ready, simply call the addTo() methods of every parameter in no particular order and you're done!

final Set<QueryParameter> params = new HashSet<>();
//Add bunch of parameters.
params.stream().forEach((param) -> param.addTo(queryStatement));

Of course there are many ways to go about doing this, my example is not necessarily the best fit for you.


Roulette game in C++ I'm kind of new to C++ and was just wondering if anyone could give me tips on how I could make my code more efficient (I added some comments to the code to help you understand it better).

Is it a good idea to use switch cases? Is there a way I can use more functions/arrays/pointers?

Source code

Here is some of the code:

case 1:
        //Asks for number to place bet on
        cout << endl << "Please choose a number to place your bet on: ";
        cin >> betNumber;

        //Checks if number is valid (between 1 and 36)
        while (betNumber < 1 || betNumber > 36) {
            cout << endl << "You must choose a valid number between 1 and 36, inclusive!" << endl;
            cout << "Please choose a number to place your bet on: ";
            cin >> betNumber;
        }

        //Asks for amount to bet on that number
        cout << endl << "How much would you like to bet on the number " << betNumber << "? $";
        cin >> betAmount;

        //Checks if minimum amount is $1 and if the player has enough money in their account
        while (betAmount < 1 || betAmount > bankAccount) {
            cout << endl << "You have $" << bankAccount << " in your bank account: $";
            cin >> betAmount;
        }

        //Seeds random number to the generator
        srand(time(0));

        //Generates a random number
        randomNumber = 1 + (rand() % 36);

        cout << endl << "The ball landed on the number " << randomNumber << ".";

        //Checks if player won or lost their bet
        if (betNumber == randomNumber) {
            bankAccount = win(betAmount, betOdds, bankAccount);
            cout << endl << "Congratulations, you won! You now have $" << bankAccount << " in your account." << endl;
        } else {
            bankAccount = lose(betAmount, bankAccount);
            cout << endl << "Bad luck, you lost! You now have $" << bankAccount << " in your account." << endl;
        }

        break;

Since there aren't any loops in your code that aren't waiting for the user to provide some input, it's probably way premature to talk about efficiency. Is your code measurably slow? Does it leave you hanging? If not, unless you have an unusual need, save questions on efficiency for later.

So let's talk about some more important things: readability and maintainability. Here are some tips on ways to improve the readability and maintainability of your code. These aren't hard rules that you should never break (especially the one on comments); they are guidelines on ways to make your life easier that you should learn to bend when the guideline makes things awkward instead.

I hope this helps, even though it may be a lot to take in all at once. Feel free to ask follow-up questions and get other opinions.

Avoid Redundancy

Redundancy can show up in many forms. One example of it is in your declaration of arrays, using code like int betType[11] = {35, 17, 8, 11, 5, 2, 1, 1, 2, 1, 1};. Unless there's something very special about the number 11, there's no reason to call it out. Instead just say int betType[] = {35, 17, 8, 11, 5, 2, 1, 1, 2, 1, 1}; which will automatically determine the size of the array for you.

When you later check your bounds with while (betChoice < 1 || betChoice > 11) {, you can instead use a calculated size (_countof(betType) or sizeof(betType)/sizeof(betType[0])), or even use a std::vector<int> instead of an int[], and check against the vector's size().

This will help you avoid magic numbers that don't mean much later. After all, if someone asks you what's special about 11, would you think it's the number of available bet types? But if they asked what betTypes.size() meant, it would be easy to answer.

Another way redundancy shows up is in large blocks of repeating code. For instance, case 1 and case 2 have almost the same code. In fact I had to read it a couple times to find the part that was different. Sometimes this can be best handled by refactoring similar parts of code into functions, and passing parameters to them that control how they differ. Sometimes it's better just to extract the parts that are identical into simpler functions, and use them. I'll touch on this more below, but I certainly don't have the answer.

Avoid Obfuscation

In the code commented Displays a large dollar sign, there are a lot of casts from int to char so that cout prints the value as a character. But the characters in question are not that unusual. Just use the actual character you want to show, for example replacing

cout << endl << "       " << (char)36 << (char)36 << (char)36 << (char)36 << (char)36;

with

cout << endl << "       $$$$$"

This will not only be easier to type or update, it will be easier to read.

Avoid Comments

This recommendation is somewhat controversial, but it begins to target your question about functions. Instead of commenting what a line of code does, comment how a block of code does something unusual. When you're first starting out, everything seems unusual, but eventually you will see patterns and only need to comment on things that are not common patterns.

But then, instead of commenting what a block of code does, give it a name instead by putting it in a function. For example, you have several cases where you ask how much the user wants to bet on a number, then loop until they enter a valid number. You could extract this loop into a helper function like this:

int getBetAmount(int bankAccount)
{
    int betAmount;
    cin >> betAmount;
    while (betAmount < 1 || betAmount > bankAccount)
    {
        cout << endl << "You have $" << bankAccount << " in your bank account: $";
        cin >> betAmount;
    }
    return betAmount;
}

int _tmain() {
    : : :
    case 1:
        : : :
        cout << endl << "How much would you like to bet on the number" << betNumber << "? $";
        betAmount = getBetAmount(bankAccount);
        : : :
    : : :
    case 2:
        : : :
        cout << endl << "How much would you like to bet on the numbers" << betNumber << " and " << betNumber + 3 << "? $";
        betAmount = getBetAmount(bankAccount);
        : : :
}

Find some other code that doesn't change much and extract that into functions as well. For example, the code commented Checks if player won or lost their bet, I see creating a function you'd call like this:

case 1:
    : : :
    bankAccout = awardWinnings(betNumber == randomNumber, betAmount, betOdds, bankAccount);
    break;
case 2:
    : : :
    bankAccount = awardWinnings(betNumber == randomNumber || betNumber + 3 == randomNumber, betAmount, betOdds, bankAccount);
    break;

After you make these changes, ideally the parts that are different will start to stand out, and the parts that are the same will have good names that tell you what they do even if they don't have a comment. And then you can more easily avoid incorrect comments like case 2's Check if number is valid (between 1 and 36) that actually checks for 33.

You can also avoid comments by naming constants. Instead of starting with int bankAccount = 500 and then 500 lines later referencing 500 to figure out your overall winnings, perhaps declare const int StartingBankAccount = 500; and use the name instead of the number in both places. If you decide to change the initial account wealth, this also helps ensure your ending summary remains correct.

Avoid Bad Dice

While this is a toy program, and a person is unlikely to play long enough for it to matter, rand() % max is a flawed approach to generating random numbers. It's flawed in ways too subtle for me to explain (I understand it, but not well enough to explain it). However Stephan T. Lavavej knows it much better and explains it in a video called rand() Considered Harmful; watch it and use the approach he recommends if you want a more uniformly distributed random number.


Chain-of-Responsibility in C++11 using smart pointers I have implemented the CoR pattern in c++11 using smart_ptr for association between chain of receivers. The client owns all the receiver objects(which can be created and destroyed at runtime) and the receiver object maintain weak_ptr for subsequent request handler.

// ChainOfResponsibility.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"

#include <memory>
#include <tuple>
#include <string>
#include <iostream>
#include <vector>

// Event, sent from sender to many receivers. The first parameter decides who should handle.
using Event = std::tuple<int, std::string>;

//Abstract class which manages the link to the subsequent classes.
class Handler {
public:
    Handler(std::shared_ptr<Handler> successor = nullptr) :m_successor(successor) {}
    virtual void handle(Event e) = 0;
    virtual  ~Handler() {}
protected:
    std::weak_ptr<Handler> m_successor;
};

//clients need to implement handle function
class Agent : public Handler {
public:
    Agent(std::shared_ptr<Handler> successor = nullptr):Handler(successor) {}
    void handle(Event e) override {
        if (std::get<0>(e) == 1) {
            std::cout << "Agent handled the request: " + std::get<1>(e) << std::endl;
        }
        else {
            try {
                std::shared_ptr<Handler> ptr(m_successor);
                ptr->handle(e);
            }
            catch(std::bad_weak_ptr) {
                std::cout << "No successor handler" << std::endl;
            }
        }

    }

    virtual  ~Agent() override {}
};

class Supervisor : public Handler {
public:
    Supervisor(std::shared_ptr<Handler> successor = nullptr) :Handler(successor) {}
    void handle(Event e) override {
        if (std::get<0>(e) == 2) {
            std::cout << "Supervisor handled the request: " + std::get<1>(e) << std::endl;
        }
        else {
            try {
                std::shared_ptr<Handler> ptr(m_successor);
                ptr->handle(e);
            }
            catch (std::bad_weak_ptr) {
                std::cout << "No successor handler" << std::endl;
            }
        }

    }
    virtual  ~Supervisor() override {}
};

class Boss : public Handler {
public:
    Boss(std::shared_ptr<Handler> successor = nullptr) :Handler(successor) {}
    void handle(Event e) override {
        if (std::get<0>(e) == 3) {
            std::cout << "Boss handled the request: " + std::get<1>(e) << std::endl;
        }
        else {
            try {
                std::shared_ptr<Handler> ptr(m_successor);
                ptr->handle(e);
            }
            catch (std::bad_weak_ptr) {
                std::cout << "No successor handler" << std::endl;
            }
        }

    }
    virtual  ~Boss() override {}
};

int main() {

    //three receivers owned by the client
    auto boss = std::make_shared<Boss>();
    auto supervisor = std::make_shared<Supervisor>(boss);
    auto agent = std::make_shared<Agent>(supervisor);

    //events
    std::vector<Event> events;
    events.push_back(std::make_tuple(1, "Technical support"));
    events.push_back(std::make_tuple(2, "Billing query"));
    events.push_back(std::make_tuple(1, "Product information query"));
    events.push_back(std::make_tuple(5, "Police issue"));
    events.push_back(std::make_tuple(3, "Enterprise client request"));

    for (auto e : events) {
        agent->handle(e);
    }

    //receivers may be released at run time.
    boss.reset();

    for (auto e : events) {
        agent->handle(e);
    }

    return 0;
}

Please review this and suggest me the improvemnt.

Assumption: Performance is not a primary concern.

Please suggest me an elegant implementation if you have seen somewhere else.

First nit: boss and supervisor strike me as basically synonymous. So it's kind of confusing that you're using both of them as identifiers in your program. Are these identifiers supposed to be meaningful, or are they just placeholders like Alice or Bob?

Also a nit: If your destructor doesn't do anything, don't write it.

virtual  ~Boss() override {}

is just a waste of characters. (The definition of ~Handler() is important because you have to hang the virtual specifier on it; but the definition of ~Boss() is perfectly redundant.)

//receivers may be released at run time.
boss.reset();

I think I don't get it. If this line had instead read supervisor.reset(), wouldn't boss have become unreachable, as far as handling requests was concerned? (Because requests go first to agent; then to agent->m_successor if it's non-null; but supervisor is gone, so handling just stops there.)

This seems like a bug in your design.

void handle(Event e) override {
    // ...
            ptr->handle(e);

This function call makes a copy of Event e, which might be expensive since Event contains an arbitrarily long std::string. You could fix this in either of two ways: either by changing the function signature to

void handle(const Event& e)

(throughout the whole hierarchy, of course), or else by changing the function call to

            ptr->handle(std::move(e));  // move, don't copy

I know you said "performance isn't a concern", but any time you're using exception handling for control flow you should really reconsider your design. Instead of throwing and catching bad_weak_ptr, I'd recommend

    if (std::get<0>(e) == 1) {
        std::cout << "Agent handled the request: " + std::get<1>(e) << std::endl;
    } else if (auto ptr = m_successor.lock()) {
        ptr->handle(e);
    } else {
        std::cout << "No successor handler" << std::endl;
    }

As for how your list of handlers should be defined: almost certainly you should just use a list of handlers (i.e. std::list<Handler> — or std::list<std::weak_ptr<Handler>> if you really want to keep your original design's ability to have handlers drop out of the list at random).

I don't think that a Handler ought to have any idea of its parent; unless... do you think that the parent of a given handler is part of its "identity"? For example, in my example above, do you think it does actually make sense that if an Agent's Supervisor dies, the Agent would have no way of contacting his late Supervisor's Boss, so certain requests would go unhandled? If that really is a feature-not-a-bug, then okay.

Lastly, even if everything else about this design were good, it's unsettling how easy it would be for someone to write a Handler class that just dropped messages on the floor.

class Stockboy : public Handler {
public:
    Stockboy(std::shared_ptr<Handler> successor = nullptr) :Handler(successor) {}
    void handle(Event e) override {
        if (std::get<0>(e) == 7) {
            std::cout << "Stockboy handled the request: " + std::get<1>(e) << std::endl;
        }
    }
};

IMO, it would be better to take the responsibility for forwarding unhandled messages and move that responsibility right out of the Handler class. Give it to a Dispatcher class whose job is to find the right Handler and call it. The Dispatcher might hold a mapping of ints to handlers so that it could find the right recipient quickly; or it might just query each handler in turn until some handler reports success.


Building blocks of Life I'm learning Clojure, and decided to write a Conway's Game of Life clone as my starting project. I ended up dicking around for a bit before diving in, and came up with a few functions that I'd like looked over. Mainly, I'm concerned about writing them more concisely and idiomatically.

I'm planning on using a 1D vector to represent a 2D field. The typical equation to get the index of a vector corresponding to an (x,y) coordinate is y * width + x. Here are my first attempts:

; A 2D point representing a coordinate, or any pair of numbers
(defrecord Point [x y])

; Represents the Game of Life world.
; cells is a vector representing a 2D matrix of cells
; dims is the dimensions of the world as a Point
(defrecord Enviro [cells dims])

(defn index-of [width x y]
  (+ (* y width) x))

(defn enviro-index-of [enviro x y]
  (let [width (:x (:dims enviro))]
    (index-of width x y)))

(defn enviro-index-of-2 [enviro x y]
  (let [width (-> enviro :dims :x)]
    (index-of width x y)))

index-of is straight forward. My issue is the 2 convenience functions to get an index by supplying the Enviro instead of the width directly. I tried deconstructing the record directly in the argument list, but it complained that it didn't recognize the keyword record keys. The work-around was to just "navigate" the records manually, and bind "width" in a let. My first attempt was kind of naïve; using the accessor functions to get the Enviro, then the dimensions Point. Then I remembered the thread macro! For my second attempt, I used -> to find the width. It seems to be much cleaner, but is this as idiomatic as it gets?

Next, I decided to try writing a function that returns a vector containing all the points surrounding a given point. I can then check the cell at each neighboring point to see whether or not the center point should be dead or alive:

(defn generate-neighbor-points
  ([cx cy r]
   (let [start-x (- cx r)
         end-x (+ cx r 1) ; Adding 1 so it's inclusive
         start-y (- cy r)
         end-y (+ cy r 1)]
     (for [y (range start-y end-y)
           x (range start-x end-x)
           :when (and (not= x cx) (not= y cy))] ; Should actually be `or` instead of `and`
       (Point. x y))))

  ([center-point r]
   (let [cx (:x center-point)
         cy (:y center-point)]
     (generate-neighbor-points cx cy r))))

There's a couple things that I'm not happy about here:

The fact that this generates a list of cells, just so it can be checked. I might see if I can just make this a higher-order function that takes a callback that's executed on each Point surrounding the cell. That might be thinking too javascript-y though.
The fact that this requires the creation of 2 "range"s, which are really full lists. This seems to be the best way of "iterating" a range of numbers and returning a list. I'm open for suggestions on how to make this more efficient and idiomatic though.

There's a lot that you can streamline here. For your Point record:

; A 2D point representing a coordinate, or any pair of numbers
(defrecord Point [x y])

If you were only going to use a Point to represent a 2D Cartesian coordinate pair, this record may have some value, but not very much. The problem is the "or any pair of numbers" part. If you want to represent a pair of numbers in Clojure, just use a vector:

[4 2]

Vectors are very easy to destructure:

(defn prettify [point]
  (let [[x y] point]
    (str "(" x ", " y ")")))

(prettify [4 2])
;=> "(4, 2)"

Or if you want to just get the first or second element in a pair, you can do that as well:

(first [4 2])
;=> 4

(second [4 2])
;=> 2

You're creating a fair bit of extra work (e.g. the index-of functions) for yourself by representing your 2D world as a 1D vector. Using a 2D vector instead would eliminate the need for all three of those functions, as well as the Enviro record itself:

(def enviro (vec (repeatedly 10 (fn [] (vec (repeatedly 10 (fn [] (rand-int 10))))))))

(pprint enviro)
;; [[3 7 6 2 9 8 5 7 0 2]
;;  [0 6 4 7 1 7 0 7 1 9]
;;  [1 8 0 5 6 9 7 7 8 3]
;;  [7 9 0 8 4 3 3 7 8 6]
;;  [8 9 3 5 1 4 6 5 0 2]
;;  [6 2 3 2 9 4 8 0 3 1]
;;  [0 5 7 7 7 9 0 0 9 5]
;;  [3 8 9 8 2 0 6 3 1 9]
;;  [9 7 4 1 5 8 0 5 1 2]
;;  [9 9 1 9 4 6 3 1 8 2]]
;=> nil

(get-in enviro [4 2])
;=> 3

Or, if your matrix is sparse, you could use a map from points to values instead:

(def enviro (into {} (repeatedly 10 (fn [] [[(rand-int 10) (rand-int 10)] (rand-int 10)]))))

(pprint enviro)
;; {[2 8] 7,
;;  [5 4] 2,
;;  [4 2] 6,
;;  [7 8] 1,
;;  [9 6] 5,
;;  [1 7] 2,
;;  [2 6] 3,
;;  [6 0] 5,
;;  [3 5] 4,
;;  [0 1] 5}
;=> nil

(get enviro [4 2])
;=> 6

If you just use a plain hash map for this, you'll lose some of the spatial query capabilities of a vector, but you could get some of those back by using a quadtree or some other spatially indexed data structure. I'll leave it up to you to decide how to go about doing that in Clojure if you want to take that route.

Your generate-neighbor-points function looks pretty good. Since range and for return lazy sequences, your implementation doesn't have any real performance issues. If I were to write it myself, though, I would probably be a bit more terse:

(defn neighbors [point radius]
  (let [[xs ys] (map #(range (- % radius) (+ % radius 1)) point)]
    (for [y ys x xs :when (not= point [x y])]
      [x y])))

There's nothing wrong with just returning the sequence of neighbor points and then leaving it up to the caller to decide what to do with them. I wouldn't recommend complecting the "find all the neighbor points" logic and the "do something with each of those neighbor points" logic into the same function, because I think it would make more sense to put that second bit in a separate function:

(defn update-all [enviro points f]
  (reduce #(update-in %1 %2 f) enviro points))

Example (using the 2D vector enviro example above):

(pprint (update-all enviro (neighbors [4 3] 2) #(mod % 2)))
;; [[3 7 6 2 9 8 5 7 0 2]
;;  [0 6 4 7 1 7 0 7 1 9]
;;  [1 0 0 1 0 1 7 7 8 3]
;;  [7 1 0 0 0 1 3 7 8 6]
;;  [8 1 1 5 1 0 6 5 0 2]
;;  [6 0 1 0 1 0 8 0 3 1]
;;  [0 1 1 1 1 1 0 0 9 5]
;;  [3 8 9 8 2 0 6 3 1 9]
;;  [9 7 4 1 5 8 0 5 1 2]
;;  [9 9 1 9 4 6 3 1 8 2]]
;=> nil


Reading Values From Enviromental Monitor XML Feed I have written the below code to handle accessing and parsing the data presented in XML format from our server room environmental monitor.

The plan is to call this class from a controller/view handling the "dashboard" on our Intranet page.

Is there any way I could optimize the class? should the methods really be static or would it be better to create an instance of the class and call the methods that way?

public class EnviromentialMonitorInterface
{
    private static XmlDocument LoadInXML()
    {
        string environmentalMonitorUrl = "http://192.168.6.95/data.xml";
        //string environmentalMonitorUrl = ConfigurationManager.AppSettings["WhiteListedDomainNames"];

        XmlDocument xmlDoc = new XmlDocument();
        xmlDoc.Load(environmentalMonitorUrl);

        return xmlDoc;
    }

    public static double GetTemperature()
    {
        var extraction = LoadInXML().SelectSingleNode("//field[@key='TempC']").Attributes.GetNamedItem("value").Value;

        double value = Convert.ToDouble(extraction);

        return value;
    }

    public static int GetHumidity()
    {
        var extraction = LoadInXML().SelectSingleNode("//field[@key='Humidity']").Attributes.GetNamedItem("value").Value;

        int value = Convert.ToInt32(extraction);

        return value;
    }
}

Naming/spacing:

Don't use Interface in a class name, it might be confusing. Just make it EnviromentialMonitor. A name like extraction doesn't say much, try xmlTemperature and xmlHumidity for example.
Be consistent when using var:

  Use implicit typing for local variables when the type of the variable is obvious from the right side of the assignment, or when the precise type is not important.

With extraction you correctly used var, but you should also have done it for value in both methods since you know what the type is from the right side of that assignment.

I don't like the linebreaks you put everywhere, it makes the code harder to read. What I usually do is block a section withy declarations and then block a section with methods, for example:

var a = 1;
var b = 2;
var items = new List<int>();

items.Add(a);
items.Add(b);

Double code:

Both the GetTemperature and GetHumidity method have a very similar structure. You could refactor the XML-handling in a separate method which you call from the other two. Both would return a double then.

public static double GetTemperature()
{
    return GetValueFromXml("//field[@key='TempC']");
}

public static double GetHumidity()
{
    return GetValueFromXml("//field[@key='Humidity']")
}

private static double GetValueFromXml(string field)
{
    var xmlValue = LoadInXML().SelectSingleNode(field).Attributes.GetNamedItem("value").Value;
    retrun Convert.ToDouble(xmlValue);
}

IP address:

Perhaps the IP won't change that fast, or at all for that matter but when you want to write good code, you make it independent. Pass the IP address as a parameter to the method or get it from a config/resource file like the line that is commented out in your question.

Static vs. Instance:

Since this is kind of a utility class and you're nowhere creating instances and/or modyfying properties/fields in the class, you can leave it static.

Also, static methods can be a little bit faster than instance methods. More reading on this from MSDN: CA1822: Mark members as static. Basically:

  Members that do not access instance data or call instance methods can be marked as static (Shared in Visual Basic). After you mark the methods as static, the compiler will emit nonvirtual call sites to these members. Emitting nonvirtual call sites will prevent a check at runtime for each call that makes sure that the current object pointer is non-null. This can achieve a measurable performance gain for performance-sensitive code. In some cases, the failure to access the current object instance represents a correctness issue.


Review of my PHP framework, vervPHP My goal with vervPHP was to create a framework I could create applications in very quickly, without any excessive configuration or over the top framework.

Here's a link to the github, to see more: https://github.com/verv/vervPHP

And now, for some code examples to show how awesome (I think) it is:

index.php:

// Start the session:
session_start();

// Include the framework base:
include_once('framework/verv.php');

// Start the application:
$verv = new verv();

framework/verv.php

class verv {

    // Variables:
    public static $db;
    public static $url;
    public static $rq = array();
    public static $lang = array();
    public static $page = array();
    public static $config = array();
    public static $commands = array();
    public static $framework = array();

    /**
     * Constructor. Do not call manually, unless your module extends verv.
     *
     * @return null
     * @since 1.0
     */
    public function __construct() {
        // Parse the config:
        self::$config = parse_ini_file('config.ini', true);

        // Setup Language:
        self::setLang();

        // Connect to the database if required:
        self::dbConnect();

        // Prepare the request:
        self::setRQ();

        // Load the remaining framework:
        self::loadFramework();

        // Load the appropriate module:
        self::loadModule(self::getCommand(0));

        // Load the template:
        self::loadTemplate();

        // And finally, render the page:
        self::renderPage();
    }

    // More functions follow...

What I'm hoping from a peer review:

How can I improve my code (generally speaking)?
Are there any glaring security holes that I should fix?
I'm wanting to make this open source (for all the help I recieved on SO while making it) so I need to add an open source license to it - one that allows modification (preferrably with attribution) and allows for commercial development.

And yes, I am in the process of improving the actual documentation - it's pretty shocking (particularly the iCommand chaining) at the moment ;)

This is procedural code trying to pretend that it is OO.  Just stick a namespace around it and write it like the procedural code that it is:

framework/verv.php

namespace verv;

function setLang() { /* Implementation */ }
function dbConnect() { /* Implementation */ }

function main()
{
    setLang();
    dbConnect();
    // etc
}

Usage

verv\main();

Advantages

No more static and self littering the code.
No loss in usual OO benefits like data hiding and encapsulation, while reducing keystrokes required for implementation.


Python Optimizing/Speeding up the retrieval of values in a large string I have a string containing around 1 million sets of float/int coords, i.e.:

    '5.06433685685, 0.32574574576, 2.3467345584, 1,,,'

They are all in one large string and only the first 3 values are used. The are separated by 3 commas. The code i'm using right now to read through them and assign them to a list is below:

    def getValues(s):
        output = []
        while s:
            v1, v2, v3, _, _, _, s = s.split(',', 6)
            output.append("%s %s %s" % (v1.strip(), v2.strip(), v3.strip()))
        return output
    coords = getValues(tempString)

The end results need to be a list of the three floats in each set separated by a space, i.e.:

    ['5.06433685685 0.32574574576 2.3467345584']

The code I defined above does work for what I want, it just takes much longer than I would like. Does anyone have any opinions or ways I could speed it up? I would like to stay away from external modules/modules i have to download.

Btw I don't think it's my computer slowing it down, 32gb ram and 350mb/s write speed, the program that creates the original string does it in about 20 secs, while my code above gets that same string, extracts the 3 values in around 30mins to an hour.

P.s. Using python 2.6 if it matters

EDIT:
Tried replacing the while loop with a for loop as I did some reading and it stated for loops were faster, it might have shaved off an extra minute but still slow, new code:

    def getValues(s, ids):
        output = []
        for x in range(len(ids)):
            v1, v2, v3, _, _, _, s = s.split(',', 6)
            output.append("%s %s %s" % (v1.strip(), v2.strip(), v3.strip()))
        return output


Pig Latin Translator in C++ I made this Pig Latin translator in C++ and I was wondering what I could improve and what is good about it.

I used the stringstream class to get the individual words.

#include <iostream>
#include <vector>
#include <sstream>
std::string translate(std::string word){
   if(word.length() > 1){
       word += word[0];
       word[0] -= word[0];
       word += "ay";
   }
   return word;
}
int main(){
    std::vector<std::string> v;
    std::string str = "Hello the world has ended";
    std::stringstream stringStream(str);
    for(int i = 0; i < str.length(); i++){
        std::string currentWord;
        stringStream >> currentWord;
        v.push_back(currentWord);
    }

for(int i = 0; i < v.size(); i++){
    std::cout << translate(v[i]) + " ";
}

std::cout << std::endl;

}

Couple of issues:

This appends the first letter of the word onto the end.

       word += word[0];

Since I don't know what "PigLatin" is I can't tell if this is correct. A comment about what is the expected translation is really required so we can understand if this is correct behavior.

This sets the first letter to 0.

       word[0] -= word[0];

Note: NOT the character 0 but the number zero which is an unprintable character (there are no glyphs that represent the number 0). Note the character 0 is represented by the number 48 (in ASCII or UTF-8).

Did you want to remove the first character from the string?

word = word.substr(1); // Gets the substring from 1 to the end.
                       // Assigns it to the variable word.

Adds the string "ay" to the end of word.

       word += "ay";

Sure. Sounds reasonable. But need to understand the expected behavior/

You have a string. Which you are converting into a sequence of words and storing in v.

    std::vector<std::string> v;

But the only thing you do with v is loop over it and get each word to call translate() with. Why not remove the middle man and not use the container at all.

Here You are creaging a loop as long as the string. But each iteration you are taking a word from the string.

    for(int i = 0; i < str.length(); i++){

So this will have a lot of blank words on the end. A better way is to read words from the stream until there are no more words in the stream.

    while (stringStream >> currentWord) {
        // Do stuff with currentWord
    }

Avoid the use of std::endl.

std::cout << std::endl;

Its probably not doing any harm here. But its a bad habit. The difference between \n and std::endl is that std::endl also flushes the buffer. Manually forcing a buffer flush is usually incorrect. The buffers will auto flush at the optimal time. Program inserted flushing is usually wrong and is a major cause of eniffciency in C++ code.

I would refactor the inner loop like this:

    while (stringStream >> currentWord) {
        std::cout << translate(std::move(currentWord)) + " ";
    }
    std::cout << "\n";

Notice the std::move() this moves an object to the function (saves a copy). So at the translate() needs to be changed slightly.

std::string translate(std::string&& word)
{                                ^^  bind a moved parameter.
   // stuff.
   return word;
}


Sparse 3D binning/histogram function This is a function written in Python I wrote to process galaxy catalogs for cosmology research, however it is more generally applicable than just my pipeline.

The goal is to take a Python list of coordinates in 3D, optionally with associated weights, define a cubic grid over the coordinates which divides the points into a variable number of "bins" or cells, and then computes the total number of points in each bin (or the sum of the weights). These sums are then associated to either the coordinates or indices of the bin.

Obviously there are existing functions in Python libraries which do this, namely numpy.histogram, scipy.stats.histogram, and less obviously numpy.unique. The reason I didn't use those is that I had to process huge catalogs of \${\sim}10^6\$ galaxies or more, and I wanted to make fairly small bins. The histogram functions store empty bins in memory, so I would often run out of memory trying to store huge numpy arrays of mostly zeros. numpy.unique avoids this, but it cannot handle summing weights instead of just counts.

So I created this function, abusing using the defaultdict subclass of the native Python dictionary to gain the summing feature. I found it to be both fast enough and that it solved my memory problems, but I am interested in improving it.

from collections import defaultdict

"""Accepts a python list of 3D spatial points, e.g. [[x1,y1,z1],...],
optionally with weights e.g. [[x1,x2,x3,w1],...], and returns the sparse
histogram (i.e. no empty bins) with bins of resolution (spacing) given by
res.
The weights option allows you to chose to histogram over counts
instead of weights (equivalent to all weights being 1).
The bin_index option lets you return the points with their bin indices
(the integers representing how many bins in each direction to walk to
find the specified bin) rather than centerpoint coordinates."""
def sparse_hist(points, res, weights=True, bin_index=False):
    def _binindex(point):
        point = point[:3]
        bi = [int(x//res) for x in point]
        bi = tuple(bi)
        return bi

    def _bincenter(point):
        point = point[:3]
        bc = [(x//res+0.5)*res for x in point]
        bc = tuple(bc)
        return bc

    if not bin_index:
        if weights:
            pointlist = [(_bincenter(x), x[3]) for x in points]
        else:
            pointlist = [(_bincenter(x), 1) for x in points]
    else:
        if weights:
            pointlist = [(_binindex(x), x[3]) for x in points]
        else:
            pointlist = [(_binindex(x), 1) for x in points]

    pointdict = defaultdict(list)
    for k,v in pointlist:
        pointdict[k].append(v)

    for key,val in pointdict.items():
        val = sum(val)
        pointdict.update({key:val})

    return pointdict

Some suggestions:

Passing the code through at least one linter such as flake8 or pycodestyle will make for more idiomatic and therefore readable code.
Do you have any automated tests? If so, do they test all four combinations of weights and bin_index?
Do you really need weights and bin_index to be defaulted? I'm generally suspicious of code which defaults parameter values without a very clear reason.
Functions are first class citizens in Python, so rather than your nested if statements you can decide which method to call and what the second parameter should be with two unnested if statements:

if bin_index:
    binning_function = _bincenter
else:
    …

if weights:
    …

Naming is very important. I can't easily follow your code because things are named for what they are and not for what they contain or what they are used for. For example, pointdict tells me nothing about its contents and res could be short for “result”, “response” or something else entirely.
Reusing variables is generally discouraged - they make the information flow confusing, and they should be optimized away by the interpreter.
I don't remember the precedence rules other than for simple arithmetic by heart, so I'd enforce it using parentheses in expressions like x//res+0.5.

Rather than using a defaultdict(list) you can just pointdict.get(key, []) if you know the dict is sparse. This is purposely not a suggestion because whether it's appropriate or not depends on the programming environment, but it is an option.


Hash table in Java with arrays and linear probing I did some manual tests by instantiating the class and it seems to be working okay. I wanted some feedback on the following:

Is the code well structured? I notice I have a lot of redundancy. The search and delete methods are almost the same.
I use a string to flag deleted keys until they are replaced, is there a better way to do this. Another boolean array maybe?

public class HashTable {
    //known limitation: This will break if the key value is set to none because I use none as the identifier string for deleted keys.
        private int capacity;
        String values[];
        String keys[];
        public HashTable(int capacity){
            this.capacity = capacity;
            values = new String[this.capacity];
            keys = new String[this.capacity];
        }

        public int hash(String key){
            int sum = 0;
            for (int i=0; i<key.length();i++){
                sum+=key.charAt(i);
            }
            return sum%capacity;
        }

        public void add(String key, String value){
            int keyhash;
            int i = 0;
            while(i!=capacity-1){
                keyhash = (hash(key)+i)%capacity;
                System.out.println("Try "+ i);
                if(values[keyhash]==null || values[keyhash].equals("none")) { //add delete flag to this condition after implementing delete.
                    values[keyhash] = value;
                    keys[keyhash] = key;
                    break;
                }

                if(values[keyhash]!=null && keys[keyhash].equals(key)){
                    values[keyhash]=value;
                    break;
                }
                i++;
            }
        if(i==capacity-1)
            System.out.println("Table appears to be full,unable to insert value");
        else
            System.out.println("Value inserted successfully.");
        }

        public String get(String key){
            int i = 0;
            int keyhash = (hash(key)+i)%capacity;
            while(values[keyhash]!=null && i<capacity){
                //System.out.println("Try " +i + " of finding the key.");
                if(keys[keyhash].equals(key))
                    return values[keyhash];
                i++;
                keyhash = (hash(key)+i)%capacity;
            }
        return null;
        }

        public void remove(String key){
            int i = 0;
            int keyhash = (hash(key)+i)%capacity;
            while(keys[keyhash]!=null && i<capacity) {
                if (keys[keyhash].equals(key)) {
                    keys[keyhash] = "none";
                    values[keyhash] = "none";
                    return;
                }
                i++;
                keyhash = (hash(key)+i)%capacity;
            }
            System.out.println("Key does not exist in table");
        }

        public void getHashedValues(){
            System.out.println();
            for(int i = 0; i<values.length;i++){
                System.out.print(" "+ values[i]);
            }
            System.out.println();
        }

    }

I formatted your code and added a main method to test the functionality of your HashTable.

I removed all System calls from the HashTable class and replaced the important text displays with Exceptions.  Generally, utility classes don't write to System.out or System.err.

I modified your getHashedValues method to return the values.

I'd change your use of i to index, but that's a minor point.

All in all, good work.

public class HashTableTestbed {

    public static void main(String[] args) {
        HashTableTestbed test = new HashTableTestbed();
        HashTable hashTable = test.new HashTable(3);

        hashTable.add("alpha", "zeta");
        hashTable.add("beta", "theta");
        hashTable.add("gamma", "tau");
        System.out.println(hashTable.get("alpha"));
        System.out.println(hashTable.get("beta"));
        System.out.println(hashTable.get("gamma"));

        hashTable.remove("beta");
        System.out.println(hashTable.get("beta"));

        hashTable.add("beta", "theta");
        System.out.println(hashTable.get("beta"));
    }

    public class HashTable {
        // known limitation: This will break if the key value
        // is set to none because I
        // use none as the identifier string for deleted keys.
        private int capacity;
        String values[];
        String keys[];

        public HashTable(int capacity) {
            this.capacity = capacity;
            values = new String[this.capacity];
            keys = new String[this.capacity];
        }

        public int hash(String key) {
            int sum = 0;
            for (int i = 0; i < key.length(); i++) {
                sum += key.charAt(i);
            }
            return sum % capacity;
        }

        public void add(String key, String value) {
            int keyhash;
            int i = 0;
            while (i != capacity - 1) {
                keyhash = (hash(key) + i) % capacity;
                if (values[keyhash] == null ||
                        values[keyhash].equals("none")) {
                    // add delete flag to this condition
                    // after implementing delete.
                    values[keyhash] = value;
                    keys[keyhash] = key;
                    break;
                }

                if (values[keyhash] != null &&
                        keys[keyhash].equals(key)) {
                    values[keyhash] = value;
                    break;
                }
                i++;
            }
            if (i == capacity - 1) {
                String text = "Table appears to be full, "
                        + "unable to insert value";
                throw new ArrayIndexOutOfBoundsException(text);
            }
        }

        public String get(String key) {
            int i = 0;
            int keyhash = (hash(key) + i) % capacity;
            while (values[keyhash] != null && i < capacity) {
                if (keys[keyhash].equals(key))
                    return values[keyhash];
                i++;
                keyhash = (hash(key) + i) % capacity;
            }
            return null;
        }

        public void remove(String key) {
            int i = 0;
            int keyhash = (hash(key) + i) % capacity;
            while (keys[keyhash] != null && i < capacity) {
                if (keys[keyhash].equals(key)) {
                    keys[keyhash] = "none";
                    values[keyhash] = "none";
                    return;
                }
                i++;
                keyhash = (hash(key) + i) % capacity;
            }
            String text = "Key does not exist in table";
            throw new IllegalArgumentException(text);
        }

        public String[] getHashedValues() {
            return values;
        }

    }

}


Instantiating objects in a MVC in PHP I am working on converting a PHP application to MVC, and have a couple questions!

1) I have a main Model object, that I require a database connection for, as well as a User object that uses a database object also. Is it a bad practice to create a new database object for each of these classes, like the following?

or should I create a $databaseObject variable on the index.php page and pass that same variable in as a construct parameter for both User and Model? What is the difference?

Is there anything else that sticks out as something that I am doing wrong?

class Model {

    function __construct() {
        $this->user = new User();
        $this->db = new Database( DB_TYPE, DB_HOST, DB_NAME, DB_USER, DB_PASS );
    }
}

class User {

    public function getUserId(){
        return $this->_userid;
    }

    public function __construct( ){
        $this->db = new Database( DB_TYPE, DB_HOST, DB_NAME, DB_USER, DB_PASS );
        $this->_userid = 1;
    }
}

Thanks in advance for the help!

In general, using the "new" keyword in any class is bad practice, because you're creating an implicit dependency. Instead, you should follow the "ask, don't look" design philosophy and require that the database object be passed to the class, rather than letting the class fetch it. Dependency injection is a common solve to this problem, and I'd recommend looking at how PIMPLE is used. Here's a good article: "Dependency Injection with Pimple".

Many people, like deceze, will deride you (rightfully so) for doing things like this, because it's one of the easiest (and probably most common) things you can do to kill the possibility of testing your code. If you were to write a unit test for this model, you'd have a database object being constructed with each test. Since the whole point of unit tests is to run code in isolation (to verify the tested code's accuracy), it would be impossible to test this model without a database.

On that note, the JavaScript guru, Misko Hevery, published a really superb article on writing testable code: "Top 10 things which make your code hard to test". While this is specifically in the context of code testing, it's also very good general programming information.


Tic Tac Toe in Python 3 Learning to code with python this is what i came up with. But looking at the code i am sure there are ways to improve. I would really appreciate a constructive review and some guidelines.

from itertools import cycle

game = [[0, 0, 0],
        [0, 0, 0],
        [0, 0, 0]]

player = [1, 2]

current_player = cycle(player)

def diagonal_win():
    if game[0][0] == game[1][1] == game[2][2] != 0:
        print('Winner')

    elif game[0][2] == game[1][1] == game[2][0] != 0:
        print('Winner')

def horizontal_win():
    for row in game:
        if row.count(row[0]) == len(row) and row[0] != 0:
            print('Winner')

def vertical_win():
    for col in range(len(game)):
        see = []
        for row in game:
            see.append(row[col])
        if see.count(see[0]) == len(see) and see[0] != 0:
            print('Winner')

def pick_player(current_player):
    if current_player == 1:
        p1rinput = input('Player 1 Row: ')
        p1cinput = input('Player 1 Column: ')
        if p1rinput.isdigit() and p1cinput.isdigit():
            p1rinput = int(p1rinput)
            p1cinput = int(p1cinput)
            print('Player 1: ')
            if game[p1rinput - 1][p1cinput - 1] == 0:
                game[p1rinput - 1][p1cinput - 1] = 1
            else:
                print('You cheated !!!Skipping turn...')
            return p1rinput, p1cinput

    if current_player == 2:
        p2rinput = input('Player 2 row: ')
        p2cinput = input('Player 2 column: ')
        if p2cinput.isdigit() and p2rinput.isdigit():
            p2cinput = int(p2cinput)
            p2rinput = int(p2rinput)
            print('Player 2: ')

            if game[p2rinput - 1][p2cinput - 1] == 0:
                game[p2rinput - 1][p2cinput - 1] = 2
            else:
                print('You cheated !!!Skipping turn...')

            return p2rinput, p2cinput

def gameplay():
    for items in game:
        print(items)
    while True:
        if vertical_win():
            break
        elif horizontal_win():
            break
        elif diagonal_win():
            break

        else:
            pick_player(next(cycle(current_player)))
            for items in game:
                print(items)

gameplay()

Displaying the grid

Personally, I would move displaying the grid into a separate function, such as display_grid:

def display_grid() -> None:
    for row in game:
        print(row)

This removes the need to keep having to write the loop over and over.

Picking the player

Another personal preference is that I would have the user input both the row and column as the input, and just split what they inputed to get the values. Removes the need to have multiple input calls, and makes the game a little easier to play.

def pick_player(current_player: int) -> (int, int):
    row, col = input(f"Player {current_player} Turn!\nEnter row and column of choice: ").split()
    if row.isdigit() and col.isdigit():
        row, col = int(row), int(col)
        if game[row - 1][col - 1] == 0:
            game[row - 1][col - 1] = current_player
        else:
            print("Position already taken! Skipping turn!")
        return row, col

Checking the board

Instead of having three separate functions to check the board, you should group all the conditionals into one function. Now, I did a bit of fixing and this function, check_win, checks all the ways the game can be won.

def check_win():
    return any([
        game[0][0] == game[1][1] == game[2][2] != 0,
        game[0][2] == game[1][1] == game[2][0] != 0,
        any([row.count(row[0]) == len(row) and row[0] != 0 for row in game]),
        any([len(set([game[i][j] for i in range(3)])) == 1 and game[0][j] != 0 for j in range(3)])
    ])

You can use any to determine if any of the elements in an iterable are True. The last line in this functions does the vertical checking. Spent a good deal trying to make this one line, and it looks pretty good. Basically, the set call removes all duplicates and retains the single values. For example, set([1, 1, 2]) results in {1, 2}.

With this information, if the length of the set is one, and the single value isn't 0, then a winner has been found. That's all that line is doing.


Helper for DropDownLists with extension method Is the following helper idiomatic ASP.NET MVC? All the built-in DropDownListFor helpers only accept IEnumerable<SelectListItem>. If the Model doesn't have this list, but an IEnumerable<SomeEntity>, avoiding this conversion in the Razor template seems to require some more of its helpers.

namespace CompanyName.ProjectName.PresentationLayer.Helpers
{
    // ...documentation omitted...
    public static MvcHtmlString DropDownListFor<TModel, TElem>(
        this HtmlHelper<TModel> htmlHelper,
        Expression<Func<TModel, string>> expression,
        IEnumerable<TElem> elements,
        Func<TElem, string> valueFunc,
        Func<TElem, string> textFunc
    )
    {
        ModelMetadata metadata =
            ModelMetadata.FromLambdaExpression(expression, htmlHelper.ViewData);

        IEnumerable<SelectListItem> items =
            elements.Select(elem => new SelectListItem()
            {
                Value = valueFunc(elem),
                Text = textFunc(elem),
                Selected = elem.Equals(metadata.Model)
            });

        return htmlHelper.DropDownListFor<TModel, string>(expression, items);
    }
}

The native MVC solution is to pass your collection into the SelectList class.

@Html.DropDownListFor(m => m.Item, new SelectList(Model.MyList, "ValueProperty", "TextProperty"))

This would make your helper method unnecessary.

However, the downside to the native approach is that there is no compile-time safety on the Text/Value properties, so you may still want to have a helper method similar to what you've written for this added benefit.

The DropDownListFor methods handle which item is selected for you, so you don't need to concern yourself with populating the Selected property for your SelectListItems.

Your extension method is not idiomatic to DropDownListFor<TModel, TProperty> because you're not using a generic type for the given property. You would at least want to keep TProperty as a generic type, and add a third generic type to your extension method for the collection type. This would change your method signature to DropDownListFor<TModel, TProperty, TElem> and your second parameter to Expression<Func<TModel, TProperty>> expression.

However, your approach has one major design issue: you lose out on all the overloads that exist for DropDownListFor. While you could reimplement all of them, it would be tackling the wrong problem.

You essentially want an elegant way to create a SelectList, so that's what your extension method should do. A Linq extension that works similarly to the ToDictionary extension method would look like the following:

public static IEnumerable<SelectListItem> ToSelectList<T>(this IEnumerable<T> source, Func<T, string> valueFunc, Func<T, string> textFunc)
{
    return source.Select(x => new SelectListItem
    {
        Value = valueFunc(x),
        Text = textFunc(x)
    });
}

Then you can use the native MVC DropDownListFor on any collection easily:

@Html.DropDownListFor(m => m.Item, Model.MyList.ToSelectList(x => x.ValueProperty, x => x.TextProperty))
@Html.DropDownListFor(m => m.Item, Model.MyList.ToSelectList(x => x.ValueProperty, x => x.TextProperty), "Choose...", new { @class = "form-control dropdown" })