Representing Objects as strings in VBA I am working an a more meaningful way to print objects in VBA.  The desired result should look something like this.

Console.PrintLine List.Create(1, 2, 3)
List(1, 2, 3)

The concept is that objects implementing an IPrintable interface are printed as their ToString property.  Primitive data types are printed as is

Console.PrintLine "a"
a

And other objects are represented as "TypeName(&ObjPtr)"

Console.PrintLine New Collection
Collection(&150653720)

But the issue that is gunking up my design is that I want nested objects to represent themselves

Console.PrintLine List.Create(1, List.Create(), New Collection)
List(1, List(), Collection(&150653384))

My solution is to use an auxiliary helper methods, located in a standard module called cast, which in a way provides a default implementation of IPrintable

Public Function ToString(ByVal x As Variant) As String

    Dim result As String

    If TypeOf x Is IPrintable Then
        result = x.ToString
    ElseIf IsObject(x) Then
        result = DefaultObjectToString(x)
    Else
        result = CStr(x)
    End If

    ToString = result

End Function
Private Function DefaultObjectToString(ByVal x As Object) As String

    DefaultObjectToString = ObjectToString(x, cast.CArray(Array("&" & ObjPtr(x))))

End Function
Public Function ObjectToString(ByVal o As Object, ByRef members() As Variant, _
        Optional ByVal delim As String = ", ") As String

    Dim stringMembers() As String
    If LBound(members) <= UBound(members) Then
        ReDim stringMembers(LBound(members) To UBound(members))
    End If

    Dim i As Long
    For i = LBound(members) To UBound(members)
        stringMembers(i) = ToString(members(i))
    Next i

    ObjectToString = TypeName(o) & "(" & Join(stringMembers, delim) & ")"

End Function

I would want this to be located in the IPrintable class but that must be empty to use it as an interface.

Now any method that is printing the objects can just use cast.ToString like console does

Public Sub PrintLine(Optional ByVal x As Variant)

    If IsMissing(x) Then
        Debug.Print vbNullString
    Else
        Debug.Print cast.ToString(x)
    End If

End Sub

Also here is how List implements IPrintable (ToArray is just an array version of the list.)

Public Property Get ToString(Optional delim As String = ", ") As String

    ToString = cast.ObjectToString(Me, ToArray, delim)

End Property
Public Property Get IPrintable_ToString() As String

    IPrintable_ToString = ToString()

End Property

If I had a BinaryTree object it would look like this.

Public Property Get ToString() As String

    ToString = cast.ObjectToString(Me, Array(LeftTree, RightTree), ", ")

End Property

I don't see anything ugly about your Cast module. If it was .Net I would use a Static (Shared) type class, and a standard *.bas module is roughly equivalent in VB6. It seems to have been the right move to me.

What I do question is the usefulness of defaulting to the object's pointer as a string representation of it. Since VB6 supports default properties, I would take advantage of it. It's not exactly pretty, but perhaps a more useful representation of an object.

Private Function TryCastToString(x As Variant) As String
    On Error GoTo ErrHandler

    TryCastToString = CStr(x)

    Exit Function

ErrHandler:
    Const MethodNotSupportedError As Integer = 438

    If Err.Number = MethodNotSupportedError Then
        TryCastToString = "&" & ObjPtr(x)
    Else
        Err.Raise Err.Number, Err.Source, Err.Description, Err.HelpFile, Err.HelpContext
    End If
End Function

Which would change this function:

Private Function DefaultObjectToString(ByVal x As Object) As String

    DefaultObjectToString = ObjectToString(x, cast.CArray(Array("&" & ObjPtr(x))))

End Function

Into this:

Private Function DefaultObjectToString(ByVal x As Object) As String

    DefaultObjectToString = ObjectToString(x, cast.CArray(Array(TryCastToString(x))))

End Function

This provides a much nicer string back. Consider a workbook with a value of "Hello!" in cell A1 and the following code.

Public Sub test()
    Debug.Print Cast.ToString(ThisWorkbook)
    Debug.Print Cast.ToString(Range("A1"))
End Sub

This would result in the following:

Workbook(&2349672)
Range(Hello!)


Counting the total number of characters in an std::set I have a std::set<std::string> and I need to know the total number of characters (sum of all strings length):

static size_t SumLength(size_t value, const std::string &str)
{
    return str.length() + value;
}

size_t TotalLength(std::set<std::string> &stringSet)
{
   return std::accumulate(stringSet.begin(), stringSet.end(), 0, SumLength);
}

What do you think of this approach?

You should make the parameter to TotalLength const, so you can use it on constant sets. Since TotalLength does not modify its argument, there is no reason not to use a const reference.

Also this is a small thing, but I'd change the name of SumLength to AddLength as Sum to me suggests that it operates on multiple strings.

Other than that your code looks perfectly fine and straight forward.


Calculating price after sales commission and discount The code already works, but I just want tips for improving.

import javax.swing.JOptionPane;
import java.text.DecimalFormat;
public class Calculator
{
    public static void main(String[] args)
    {
        double initialPrice = 0.00;
        double commissionRate = 0;
        double discountRate = 0;
        initialPrice = Double.parseDouble(JOptionPane.showInputDialog(null, "What is the initial price total of the sale?"));
        commissionRate = Double.parseDouble(JOptionPane.showInputDialog(null, "What is the percentage amount of the sales commission? \nFor example for 20%, type 20."));
        discountRate = Double.parseDouble(JOptionPane.showInputDialog(null, "What is the customer's discount rate? \nFor example for a 15% discount, type 15."));
        JOptionPane.showMessageDialog(null, "The final cost to the customer is \n$" + computeFinalPrice(initialPrice, commissionRate, discountRate) + ".");
    }
    public static String computeFinalPrice (double initialPrice, double commissionRate, double discountRate)
    {
        double finalPrice;
        DecimalFormat d = new DecimalFormat("0.00");
        finalPrice = (initialPrice*(commissionRate+100)*.01)*((100-discountRate)*.01);
        return (d.format(finalPrice));
    }

Just a quick note: Don't use floating point variables where you may need exact results:

Effective Java, 2nd Edition, Item 48: Avoid float and double if exact answers are required
Why not use Double or Float to represent currency?

Fortunately Java has BigDecimal.


Group duplicate files (part 2) This is a continued discussion (from here => Group duplicate files) with new code and new thoughts/questions (see special question part for details for new questions), and I decide to make a new post.

Here is my source code which group duplicate files together, written in Python 2.7. Any advice on smarter ideas to group duplicate files together more efficient, it will be great. Any general advice on code bugs and code style are also appreciated.

Problem statement:

I am given a list of files e.g. ["1.txt", "2.txt", "3.txt", "4.txt", "5.txt", "6.txt"]. I want to group all files which have the exact same content. Suppose in this example, file "1.txt", "2.txt", "3.txt" are the same, file "4.txt", "5.txt", "6.txt" have common header, but "4.txt", "6.txt" are exactly the same whole content. Then, the output should be two groups "1.txt", "2.txt", "3.txt" and "4.txt", "6.txt".

My major idea:

To avoid reading full content for each file, I generate a hash code for a file header (in my example, I define the file header to be the first 10 bytes of a file)
If more than 1 (>=2) files have the same header, I will read the full content and see if they are the exact same whole content -- I still generate hashing for whole content since if multiple files are the same hashing for header, there are many different potential duplication sub-groups, using hashing is easy to group true equal content file together.

Special question

I find below implementation still needs to read whole file content for any potential duplicate files. Wondering if any better ideas for improvement in terms of performance perspective?

Source code in Python 2.7

from collections import defaultdict

def read_whole_file(filename):
    with open(filename) as f:
        return hash(f.read())
def read_file_header(filename, header_length):
    with open(filename) as f:
        return hash(f.read(header_length))
def group_duplicate_files(filenames):
    header_buf = defaultdict(list)
    whole_buf = defaultdict(list)
    for f in filenames:
        header_buf[read_file_header(f,10)].append(f)
    for files in header_buf.values():
        if len(files) == 1:
            continue
        for f in files:
            whole_buf[read_whole_file(f)].append(f)
    return whole_buf.values()
if __name__ == "__main__":
    files = ["1.txt", "2.txt", "3.txt", "4.txt", "5.txt", "6.txt"]
    print group_duplicate_files(files)

To formalize my comments above as an answer (even though it will be an incomplete one as to possible optimizations):

You can check other "significant" parts of the files before hashing the whole thing. For example, as you've done, not only the first 10 characters but then the final 10 (even a middle 10 afterwards in case they also share identical closing headers).
To do the above, and as a bonus to optimize the I/O aspect, you can avoid re-opening each file, which is quite slow. Instead, keep them open and read as necessary, or even read a large chunk and then close, but don't reopen.
For style commentary, since you requested it in version 1 of this thread:

Add a little whitespace between functions and for loop blocks, and generally between any self-contained block of 3-4 lines or more.
You presumably named the variable files because you'd already used filenames, but files actually refers to groups of filenames. What's more, the variable name doesn't indicate what's special about those "files". One suggestion might be collision_set.

Speaking of sets, for a minor improvement, you could store the values in sets instead of appending to lists.


Binary string to integer and integer to binary string I am working through K&R for review and I thought that I would write some of my own routines for conversion of binary strings to int and int to binary string. I realize this is reinvetning the wheel; I am just doing this for fun. Also, I am attempting to follow this coding style CodingStyle.txt. Any and all critiques are welcomed.

#include <mybin.h>
#include <limits.h>
#include <errno.h>

/*
Parameters:
s   - String with a maximum of 63 binary digits
Return:
    - Decimal value of the binary string pointed to by s
Error:
    - Negative returned on error check errno
*/
signed long binstr2int(char *s)
{
    signed long rc;
    for (rc = 0; '\0' != *s; s++) {
        if (rc > (LONG_MAX/2)) {
            errno = ERANGE;
            return -1;
        } else if ('1' == *s) {
            rc = (rc * 2) + 1;
        } else if ('0' == *s) {
            rc *= 2;
        } else {
            errno = EINVAL;
            return -1;
        }
    }
    return rc;
}

/*
Parameters:
num - The number to convert to a binary string
s   - Pointer to a memory region to return the string to
len - Size in bytes of the region pointed to by s
Return:
    - Pointer to the beginning of the string
Error:
    - NULL returned on error check errno
*/
char *int2binstr(unsigned long num, char *s, int len)
{
    if (len < 2) {
        errno = ERANGE;
        *s = '\0';
        return s;
    } else if (0 == num) {
        s[--len] = '\0';
        s[--len] = '0';
        return s + len;
    } else {
        for (s[--len] = '\0'; 0 != num; num >>= 1) {
            if (0 == len) {
                errno = ERANGE;
                *s = '\0';
                return s;
            } else if (num & 1) {
                s[--len] = '1';
            } else {
                s[--len] = '0';
            }
        }
    }
    return s + len;
}

Edit 1:
Okay, here is the revised code. Most of the suggestions I agree with. I did retain filling the buffer from end to beginning because it seems like a logical flow to me and requires one less variable to deal with.

The do while suggestion was spot on. The for loop was unusual and the do while consumed one of the conditionals making the code easier to read.
The names of the functions have been revised so they are not misleading
The functions now handle unsigned long ints
ul2binstr now returns NULL on error. So this needs to be checked.
Also the string parameter was in binstr2ul was made const
The len parameter was changed to size_t type

/*
Parameters:
s   - String with a maximum of log2(ULONG_MAX) binary characters
num - Memory address to store the result in
Return:
    - Status integer
Error:
    - Negative returned on error check errno
*/
int binstr2ul(const char *s, unsigned long *num)
{
    unsigned long rc;
    for (rc = 0; '\0' != *s; s++) {
        if (rc > (ULONG_MAX/2)) {
            errno = ERANGE;
            return -1;
        } else if ('1' == *s) {
            rc = (rc * 2) + 1;
        } else if ('0' == *s) {
            rc *= 2;
        } else {
            errno = EINVAL;
            return -1;
        }
    }
    *num = rc;
    return 0;
}

/*
Parameters:
num     - The number to convert to a binary string
s       - Pointer to a memory region to return the string to
len     - Size in bytes of the region pointed to by s
Return:
        - Pointer to the beginning of the string
Error:
        - NULL returned on error check errno
*/
char *ul2binstr(unsigned long num, char *s, size_t len)
{
    if (0 == len) {
        errno = EINVAL;
        return NULL;
    } else {
        s[--len] = '\0';
    }

    do {
        if (0 == len) {
            errno = ERANGE;
            return NULL;
        } else {
            s[--len] = ((num & 1) ? '1' : '0');
        }
    } while ((num >>= 1) != 0);

    return s + len;
}

It makes sense for the two functions to be symmetrical - ie that you can call
them in turn to swap between integer and string representations.  So I would
make binstr2int produce an unsigned long of full range returned through a
parameter and have the function return the status (0/-1):

int binstr2int(const char *s, unsigned long &num);

Note that s can be const.

In int2binstr I think the characters should be moved to the beginning of the
string - this is what all other C string functions do (or at least I'm
ignorant of any that do not).  Also, your code is a bit awkward in
that it treats 0 separately from all other numbers.  This is a case where a
do {...} while loop makes sense as it can handle the zero case as any other
number and test for zero once it has done it:

char* int2binstr(unsigned long num, char *s, size_t size)
{
    long len = (long) size - 1;
    do {
        if (len <= 0) {
            errno = ERANGE;
            return NULL;
        }
        s[--len] = (num & 1) ? '1' : '0';
    } while ((num >>= 1) != 0);

    long n = (long) size - len - 1;
    memmove(s, s + len, n);
    s[n] = '\0';
    return s;
}

Here you can see that the tests for a short input string and for
num == 0 are handled naturally.  Also I moved the string into place after
creating it.


PHP flatfile storage So, I made a tiny (~2 KB, <100 lines) library for simple flat-file data storage.

How it works is, you define a file to be used, and then you can add/read/modify/delete objects as "keys" to/from the file.

All the code is on github, but I'll put it here too:

varDx.php:

<?php
namespace varDX;

class cDX {
    private $dataFile;

    public function def($filename){
        $this->dataFile = $filename;
    }

    public function write($varName, $varVal){
        if(file_exists($this->dataFile)){
            $foundLine = $this->check($varName);
        } else {
            $foundLine = false;
        }

        if(!$foundLine){
            $writeData = $varName.'__=__'.urlencode(serialize($varVal)).PHP_EOL;
            file_put_contents($this->dataFile, $writeData, FILE_APPEND);
        } else {
            return "ERR_DX_KEY_ALREADY_EXISTS";
        }
    }

    public function read($varName){
        if(file_exists($this->dataFile)){
            foreach(file($this->dataFile) as $line) {
                if(strpos($line, $varName) !== false) {
                    list(, $new_str) = explode("__=__", $line);
                    $foundLine = true;
                }
            }
            if($foundLine){
                $val = rtrim($new_str);
                return unserialize(urldecode($val));
            } else {
                return "ERR_DX_KEY_NOT_FOUND";
            }
        } else {
            return "ERR_DX_FILE_DOES_NOT_EXIST";
        }
    }

    public function del($varName){
        if(file_exists($this->dataFile)){
            $f = $this->dataFile;
            $term = $varName.'__=__';
            $arr = file($f);
            foreach ($arr as $key=> $line) {
                if(stristr($line,$term)!== false){unset($arr[$key]);break;}
            }
            //reindexing array
            $arr = array_values($arr);
            //writing to file
            file_put_contents($f, implode($arr));
        } else {
            return "ERR_DX_FILE_DOES_NOT_EXIST";
        }
    }

    public function modify($varName, $varVal){
        if(file_exists($this->dataFile)){
            if($this->check($varName)){
                $this->del($varName);
            }
        }
        $writeData = $varName.'__=__'.urlencode(serialize($varVal)).PHP_EOL;
        file_put_contents($this->dataFile, $writeData, FILE_APPEND);

    }

    public function check($varName){
        if(file_exists($this->dataFile)){
            foreach(file($this->dataFile) as $line) {
                if(stripos($line, $varName.'__=__') === 0){
                    return true;
                }
            }
            return false;
        } else {
            return "ERR_DX_FILE_DOES_NOT_EXIST";
        }
    }
}

Usage:

<?php
require 'varDx.php';
$dx = new \varDx\cDX; //create object
$dx->def('file1.txt'); //define data file

$a = "this is a string";
$dx->write('val1', $a); //write key to file

$dx->modify('val1', "this is another string"); //modify value of key
echo $dx->read('val1'); //read value of key

if($dx->check('val1')){ //check if key exists
    del('val1'); //delete key
}

File storage:

All keys are stored in this format:

keyname__=__urlencode(serialize(value_of_key))

There's more info on the functions in the README on the github page. I'm wondering if I can make this more efficient when dealing with files, and if there's anything else that I'm doing wrong?

Assuming this code is solely for learning purpose, there are several main issues

to be honest, a function that returns ERR_DX_FILE_DOES_NOT_EXIST when I request an object looks a bit weird. I don't even know how to use it. Should I wrap every call to a function in a condition that checks the returned value? Or what else I am supposed to do with this string? Consider using errors/exceptions in case of error.
a race condition issue. you can have this file malformed if two parallel processes would try to write at the same time. Consider use file locks
an obvious memory issue - the bigger the file is, the more RAM does it take to process. PHP can let you to read a file line by line without memory overhead
such a clumsy format as keyname__=__urlencode(serialize(value_of_key)) doesn't seem to be very reliable. For such a toy storage I would rather use json format, to ease the search/key access.


Function to get top k words in a given String This is a two-part job:

to identify a word
to find the top k words

I've tried to use regex to split but it's taking 10x more time when string is really long. The top k strings are found using a priority queue. I've created a min priority queue for first k element, if the frequency of the next word is greater than the min element. I then remove min and insert the new word.

 public static List<String> getTopStrings(String input, int k)
        {
            if(k<0)
                throw new IllegalArgumentException("Value k cannot be negative for top k elements: k= "+k);
            List<String> list = new ArrayList<String>();
            if(input==null || input.isEmpty() || k==0)
            {
                return list;
            }
            Set<Character> separators = new HashSet<Character>();
            separators.add(' ');
            separators.add(',');
            separators.add('.');
            separators.add(';');
            separators.add(':');
            separators.add('"');
            separators.add('(');
            separators.add(')');
            separators.add('-');
            separators.add('/');
            separators.add('\\');
            separators.add('\'');
            separators.add('?');
            separators.add('\n');
            separators.add('\r');
            separators.add('!');
            separators.add('|');
            separators.add('~');
            separators.add('\'');
            separators.add('[');
            separators.add(']');
            separators.add('{');
            separators.add('}');
            separators.add('&');
            separators.add('%');
            separators.add('$');
            separators.add('#');
            separators.add('@');
            separators.add('*');
            separators.add('=');
            separators.add('+');
            separators.add('>');
            separators.add('<');

            final Map<String, Integer> wordMap = new HashMap<String, Integer>();
            int count, wordStart=0;
            for(int i=0;i<input.length();i++)
            {
                if(separators.contains(input.charAt(i)))
                {
                    addWord(input, wordMap,wordStart,i);
                    wordStart=i+1;
                }
            }
            addWord(input,wordMap,wordStart,input.length());
            List<String> keySet = new ArrayList<String>();
            keySet.addAll(wordMap.keySet());
            if(keySet.size()<=k){
                return keySet;
            }

            PriorityQueue<String> minHeap = new PriorityQueue<String>(k, new Comparator<String>() {
                @Override
                public int compare(String o1, String o2) {
                    return (wordMap.get(o1)-wordMap.get(o2));
                }
            });

            for(int i=0;i<k;i++)
            {
                minHeap.add(keySet.get(i));
            }
            for(int i=k;i<keySet.size();i++)
            {
                String key = keySet.get(i);
                count = wordMap.get(key);
                if(count>wordMap.get(minHeap.peek()))
                {
                    minHeap.poll();
                    minHeap.add(key);
                }
            }
            list.addAll(minHeap);
            return list;
        }
        public static void addWord(String input, Map<String, Integer> wordMap, int startIndex, int endIndex)
        {
            int count = 0;
            String word = input.substring(startIndex, endIndex).toLowerCase();
            if(word!=null && !word.isEmpty())
            {
                if(wordMap.containsKey(word))
                {
                    count = wordMap.get(word);
                }
                wordMap.put(word, count+1);
            }
        }

Initialize Set with values

Your method of initializing the hashset with values takes quite a bit of space. I would create a static field:

public static final String[] SEPARATORS = new String[] { " ", ",", ".", ";" };

And then use it like this:

public static final Set<String> separatorsSet = new HashSet<String>(Arrays.asList(SEPARATORS));

Extract code to methods

Your getTopStrings method currently does three things: extract words from string, count amount of duplicate words, sort the result of that by frequency. I would at least create separate methods for the first two and the third functionality.

Simplify Sorting

Your sorting mechanism using a queue seems overly complicated. If you are using Java 8, it could be reduced to three lines (or one really long one):

    // sort by value, reversed
    Stream<Entry<String, Integer>> sorted = wordMap.entrySet().stream().sorted(Map.Entry.comparingByValue((x,y) -> (y < x) ? -1 : ((x == y) ? 0 : 1))); // alternatively: use reversed and cast
    // flatten map to list:
    List<String> collected = sorted.map(entry -> entry.getKey()).collect(Collectors.toCollection(ArrayList::new));
    // top k:
    List<String> topK = collected.subList(0, k);

Misc

in Java, it is customary to place curly brackets on the same line as the opening statements.
use curly brackets even for one line statements for improved readability and to avoid future bugs.
use more spaces, eg around ==, <, +, after ;, etc for increased readability.
declare variables in as small a scope as possible to increase readability.


Does this one-line text-based terrain generator go against standards? This isn't really much of an actual project, just something I threw together in about 5-10 minutes. I just want to know if it goes with the current Python standards, and if anything should be changed or fixed.

print(''.join([__import__('random').choice(['\033[7;1;32m  \033[0m','\033[7;32m  \033[0m','\033[7;1;33m  \033[0m','\033[7;34m  \033[0m']) for world in range(40)])); __import__('time').sleep(0.5)

Can anything be improved? What needs to be fixed?

What does THE standard say about imports :

  Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.

What else could easily be improved in your code ?

_ is a usual name for variable whose value is not going to be used.
you do not need to create a list : you can use generator expressions by getting rid of brackets.
you can get rid of the duplicated prefix and suffix in the different choices.
in Python 2.*, you do not need parenthesis to call print.

Your code would become :

from random import choice
from time import sleep
print ''.join('\033[7;' + choice(['1;32','32','1;33','34']) + 'm  \033[0m' for _ in range(40))
sleep(0.5)


Use of filter and drop shadow The following code will be used for educational purposes. It is to simply demonstrate how to apply a drop-shadow using a CSS3 filter. Any feedback on areas where I have not used best practices would be great.

JSBin

HTML:

<!DOCTYPE HTML>

    <title>Drop Shadow</title>
    <link rel="stylesheet" type="text/css" href="drop-shadow.css">

CSS:

    body{
            background: #ccc;
            margin: 0;
    }
    h1{
        font-size: 5em;
        font-family: Georgia;
    }
    .container{
        padding: 20px 50px;
    }
    .one img{
        -webkit-filter: drop-shadow(0 0 30px black);
    }
    .two{
        background: black;
    }
    .two img{
        -webkit-filter: drop-shadow(-15px 0px 20px white);
    }

Your code it's perfectly fine

I would just change, to stress the fact that the images are the same:

.container img {
    width: 250px;
    height: 250px;
    content: url("http://iancottam.co.uk/img/footy.png");
}

and then

jsbin


Stack implementation, array-based My understanding is that Stack possesses LIFO property. And, based upon that I have written the following program:

// This is a sample program to demonstrate push and pop
// functionality in Stack in Java.

package collections;

public class Stack_Array {

    private static final int capacity = 3;
    int[] arr = new int[capacity];
    int top = -1;

    public void push(int pushedElement) {
        if(top >= capacity) {
            System.out.println("StackOverflow !!");
        } else {
            top++;
            arr[top] = pushedElement;
        }
    }

    public void pop() {
        if(top>=0) {
            top--;
        } else {
            System.out.println("Stack underflow !!");
        }
    }

    public void printStack() {
        System.out.println("Elements in Stack are: ");
        for(int i = top; i >=0; i--) {
            System.out.println(arr[i]);
        }
    }

    public static void main(String[] args) {

        Stack_Array s1 = new Stack_Array();
        s1.push(23);
        s1.push(21);
        s1.push(14);

        s1.pop();
        s1.printStack();
    }

}

Please let me know if it is the correct approach.
Or, is there a better way to implement the same

I like the consistency of your braces and indentation - well done!

The name Stack_Array should be StackArray, since as member names are camelCase, type names ought to be PascalCase, not Upper_Snake_Case.

You'll want to implement a constructor so that the capacity of the object can be parameterized, and then the calling code can look like this:

StackArray foo = new StackArray(42);
foo.push(bar);

The underlying capacity static field would need to be changed a bit though; you'll want it to be an instance field, so that each object can have its own independent value. I'd keep it final though, because it's not a value you want to be able to modify during the lifetime of an instance - beyond the constructor, nothing should be allowed to modify that value.

You'll want to throw an appropriate exception in exceptional situations:

if(top >= capacity) {
    System.out.println("StackOverflow !!");

A Stack is a data structure; other code will rely on its correct behavior, and expect a number of errors to occur in certain specific situations - for example when the caller attempts to push an element into an instance that already contains as many items as it can hold.

That said, I'd prefer a Stack implementation that simply increases its capacity as items are pushed. In any case, printing "StackOverflow !!" isn't appropriate. Making the method return a bool that the calling code can evaluate to determine whether the push/pull succeeded or not, would be a better alternative if you really want a fixed capacity.

Printing to the console isn't the job of a data structure, printStack should be replaced by a method that returns a new data structure that contains a copy of the internal array. Such a toArray method would leave it up to the calling code to decide whether they want to print the items to the console, or dump them into a log file, or whatever.

Your implementation is pretty bare-bones, you'll want to look at the java.util.Stack<E> members and possibly dig into generics. The C# System.Collections.Generics.Stack<T> class could also be an inspiration.

The pop method would be expected to return the popped item. A peek method is missing, to "peek" at the next item without popping it out of the stack.

Other observations

Between push and pop, the two have an if conditional, but one is no-op in the positive branch, while the other is no-op in the negative branch (/else block). It would be better to decide whether you test for the error condition [to throw an error and fail early] or if you test for the success condition [and throw an error in the else block] - it's usually better to fail early and reduce the indentation level:

public void foo() {
    if ({error condition}) {
        throw new SomeException();
    }
    doSomething();
}

Watch the spacing on either side of comparison operators:

for(int i = top; i >=0; i--) {

Give it the same whitespace as for the = assignment operator:

for(int i = top; i >= 0; i--) {


Recorder for keyboard and mouse events I'm building an event recorder. So far the code looks clean, but I feel like I'm missing something that can make this code even better.

Is composition a better solution? If yes, how can I do it properly in this context. Do you know a better way to share the Recorder across all other components? Will it be better to initialize the recorder in app.js and then pass it to the other components (mouse, keyboard)?

Mouse.js and Keyboard.js look pretty much the same, except they record different event types. Is there a way to avoid repetition (stop and record method)? Just tell me what you think about the logic.

This needs to be flexible because I have no idea how many events I will need to record in the future.

app.js

export default (params) => {
  const container = document.getElementById(params.container),
        recorder = Recorder('session'),
        keyboard = Keyboard(),
        mouse = Mouse(container);

  const startRecording = () => {
    recorder.record('Start recording');
    keyboard.record();
    mouse.record();
  }

  const stopRecording = () => {
    keyboard.stop();
    mouse.stop();
    recorder.record('Stop recording');
  }

  const init = () => {
    document.getElementById('start').onclick = (e) => {
      e.preventDefault();
      startRecording();
    };
    document.getElementById('stop').onclick = (e) => {
      e.preventDefault();
      stopRecording();
    };
  }

  return {
    init
  }
}

recorder.js

import Data from '../db/eventsLog';
import moment from 'moment';

export default (type) => {
  const record = (input) => {
    const t = moment().unix();
    Data[type].push({t, event: input});
  }

  return {
    record
  }
}

keyboard.js

import Recorder from './recorder';

export default () => {
  const recorder = Recorder('keyboard');

  const initRecorder = (recording) => {
    document.onkeydown = (e) => {
      if (recording) {
        recorder.record(e);
      }
    };
  };

  const record = () => initRecorder(true);

  const stop = () => initRecorder(false);

  return {
    record,
    stop
  }
}

mouse.js

import Recorder from './recorder';

export default (container) => {
  const recorder = Recorder('mouse');

  const startRecording = (recording, e) => {
    if (recording) {
      recorder.record(e);
    }
  };

  const initRecorder = (recording) => {
    // Mouse mouve
    container.onmousemove = (e) => startRecording(recording, e);

    // Mouse Double click
    container.ondblclick = (e) => startRecording(recording, e);

    // Mouse Click
    container.onclick = (e) => startRecording(recording, e);
  };

  const record = () => initRecorder(true);

  const stop = () => initRecorder(false);

  return {
    record,
    stop
  }
}

I was going to put this as a comment, but could not show formatted code, so this will have to be an answer.

If you ever want to adapt your script to records "all" events, you could use something like this:

var Events = Object.keys(window).reduce(function(events, prop){
        if( prop.substring(0,2) === 'on' ){
            events.push(prop.slice(2));
        }
        return events;
    }, []);

    Events.forEach(function(event){
        window.addEventListener(event, function(e){
            console.log(event + " fired");
        })
    })

You would definitely want to debounce your event recordings, otherwise you will eat up a lot of the event loop.

There are other events, in the browser, but these are almost all of them, for a complete list checkout MDN Events and MDN Event - Web APIs for more details.


Flowers Challenge on HackerRank I worked through the Flowers challenge on hackerrank.com and would like to get feedback on solution and algorithm implementation.  It was suggested by hackerrank to review greedy algorithm to solve this challenge.  As far as I understand this is an implementation of greedy algorithm.

  You and your \$K−1\$ friends want to buy \$N\$ flowers. Flower number \$i\$ has cost \$c_i\$. Unfortunately the seller does not want just one customer to buy a lot of flowers, so he tries to change the price of flowers for customers who have already bought some flowers. More precisely, if a customer has already bought \$x\$ flowers, he should pay \$(x+1) \times c_i\$ dollars to buy flower number \$i\$.

  You and your \$K−1\$ friends want to buy all \$N\$ flowers in such a way that you spend the least amount of money. You can buy the flowers in any order.

  Input:

  The first line of input contains two integers \$N\$ and \$K\$ (\$K≤N\$). The next line contains \$N\$ space separated positive integers \$c_1,c_2,\dots,c_N\$.

  Output:

  Print the minimum amount of money you (and your friends) have to pay in order to buy all \$N\$ flowers.

import java.io.*;
import java.util.*;

public class Flowers {

    public static void main(String[] args) {
        /* Enter your code here. Read input from STDIN. Print output to STDOUT. Your class should be named Solution. */
        Scanner sc = new Scanner(System.in);
        ArrayList <Integer> flowerPriceList = new ArrayList<Integer>();
        int numFlowers = sc.nextInt();
        int numFriends = sc.nextInt();
        for(int i = 0; i<numFlowers;i++){
            flowerPriceList.add(sc.nextInt());
        }
        // Sort the ArrayList in reverse order to start bying flowers from most expensive
        Collections.sort(flowerPriceList,Collections.reverseOrder());
        int flowersBought = 0;
        int friendNum = 0;
        int total = 0;
        for(int price:flowerPriceList){
            //itterate throught all the flower prices and calculate the total
            total +=(flowersBought+1)*price;
            friendNum++;
            if(friendNum == numFriends){
                //if all friends bought flowers reset the friend counter and restart the cycle
                friendNum = 0;
                flowersBought++;
            }
        }
        System.out.println(total);
    }
}

Class name and general comments

Your class is named Flowers but there is a comment here that says:

/* (...) Your class should be named Solution. */

Although I think that Flowers is a much better name, personally.

Generally, I understood your code better than the assignment description, so well done there.

Array instead of ArrayList

Instead of using an ArrayList <Integer>, that by the way would be better to declare as List<Integer> (use interface for declaration, implementation for initialization), you could use an int[] instead, as you know the exact size of the list. There's no need to do any dynamical resizing of the list, so a int[] would be just fine.

Small spacing nitpick

A little nitpick would be to use a bit more spacing, I am a bit allergic to lines like this:

for(int i = 0; i<numFlowers;i++){

better would be:

for (int i = 0; i < numFlowers; i++) {

And also:

total +=(flowersBought+1)*price;

becomes:

total += (flowersBought + 1) * price;

Closing statement

Again, your code was very readable and your approach is good, and indeed it is a greedy algorithm

As said on Hackerrank:

  A greedy algorithm is an algorithm that follows the problem-solving heuristic of making the locally optimal choice at each stage with the hope of finding a global optimum.

This is indeed what your code does. It always picks a way to buy the flower in the cheapest way.


Shortest path from U to V using at most k nodes I'm trying to solve a problem that asks for a shortest path between two cities with at most k nodes.

The first line of input has the number of test cases. On the second line 3 integers are given, which are respectively, the number of nodes, the number of edges and the maximum number of nodes in the shortest path. After that all edges are described with 3 integers representing 2 connected cities and a weight of travel from city 1 to city 2.

For the output just print the shortest path. If there is no shortest path print '-1'

I'm using a \$O(k * n ^ 2)\$ Floyd-Warshall solution. Is there a better solution? This solution is exceeding the time limit.

#include <stdio.h>
#include <string.h>

#define MAX ((unsigned int) -1)

int main(){
    unsigned int number_of_cases;
    scanf("%u",&number_of_cases);

    for (int i = 0; i < number_of_cases; ++i) {
        unsigned int number_of_nodes,number_of_edges,limit_of_nodes;
        scanf("%u %u %u",&number_of_nodes,&number_of_edges,&limit_of_nodes);

        unsigned int g[number_of_nodes][number_of_nodes];
        memset(g, -1, sizeof(g));

        for (int j = 0; j < number_of_edges; ++j) {
            unsigned int u,v,w;
            scanf("%u %u %u",&u,&v,&w);
            g[u][v] = w < g[u][v] ? w : g[u][v]; //since there is no negative weight... we can choose the minimum one to the adjacency list and ignore the others.
        }

        /*
            In the k'th run the floyd-warshall algorithm gives the shortest path with k edges.
            If i want a shortest path with number of nodes = limit_of_nodes . I just need limit_of_nodes - 1 edges.
         */
        --limit_of_nodes;
        for (int k=0 ; k < limit_of_nodes ; k++)
            for (int i=0 ; i < number_of_nodes ; i++)
                for (int j=0 ; j < number_of_nodes ; j++)
                    if (g[i][k] !=  MAX && g[k][j] != MAX && g[i][k] + g[k][j] < g[i][j])
                        g[i][j] = g[i][k] + g[k][j];

        if(g[0][number_of_nodes-1] == MAX )
            puts("-1");
        else
            printf("%u\n", g[0][number_of_nodes-1]);
    }

    return 0;
}

Some observations:

The number of nodes of a solution can't be less than 2 (since the initial and final nodes are in the solution).
There are no negative numbers on the input.
There can be more than 1 edge between 2 cities. (Is this relevant ?)

Input sample:

3
5 5 3
0 1 2
0 2 1
1 4 3
2 3 1
3 4 2
3 2 2
0 1 1
1 2 1
3 3 2
0 1 1
1 2 1
0 2 3

Output sample:

5
-1
3

A complete description of the problem is here

Algorithm flawed

Your comment here is wrong:

  In the k'th run the floyd-warshall algorithm gives the shortest path with k edges.

In the k'th run, the algorithm gives the shortest path from each node to each other node using intermediate nodes from 0..k.

As an example, suppose you had 10 vertices and wanted to find the shortest path between vertices 0 and 9, with max nodes = 2.  If the shortest path was 0 -> 8 -> 9, you would need to run 9 iterations for k before you would find that path.  Unfortunately, by that time you would have found paths using more than 2 nodes.

Here is an example input for the above case demonstrating the problem:

1
10 3 3
0 8 1
8 9 1
0 9 999

Output: 999

Corrected algorithm

Your idea about iterating k times was good, but you need to compute the correct thing on each loop.  What you should be computing is the shortest path from node 0 to each other node using k or fewer nodes in the path.  To do this, you can still use the same three loops, but like this:

    unsigned int new_g0[number_of_nodes];
    for (int k=2 ; k < limit_of_nodes ; k++) {
        memcpy(new_g0, g, sizeof(new_g0));
        for (int i=1 ; i < number_of_nodes ; i++) {
            for (int j=1 ; j < number_of_nodes ; j++) {
                if (g[0][i] != MAX && g[i][j] != MAX &&
                        g[0][i] + g[i][j] < new_g0[j]) {
                    new_g0[j] = g[0][i] + g[i][j];
                }
            }
        }
        memcpy(g, new_g0, sizeof(new_g0));
    }

The new_g0 array is something I added to avoid modifying the matrix when the loop is still computing the current iteration.  If you modified g[0][j] directly, it could influence another computation in the same loop, causing you to find a path with k+1 nodes instead of k nodes.

The k loop starts at 2 because the original matrix already has the lengths of paths with 2 nodes (the start and end nodes).

While this algorithm is now correct, it has the same run time as your original algorithm, \$O(k*n^2)\$, so it doesn't solve your problem of exceeding the time limit.

Other algorithms

If the number of edges is small compared to the number of vertices, you might be better off using a search algorithm such as a depth first search where you 1) limit the depth and 2) keep track of the best distance so far so you can backtrack immediately whenever you exceed the best distance.

Dijkstra's algorithm is good for finding the shortest path between two nodes, but unfortunately, it is not easy to modify it to also have the constraint that there be at most k nodes in the path.


Pagination and Controller setup Below is some code that I've been working to simplify, however I feel there is more that can be done. Please note that I did clean up the articles.search method.

class HomeController < ApplicationController
    @@articles_per_page = 6
     def index
        @paged_articles = articles.search(:user_id=> @@user_id, :page=>'1', :per_page=>@@articles_per_page)
        @tags = get_tags
        @next_page_url = url_for(:controller =>:home, :action => :page, :page_id=>  2)
        @page_title = "Recent articles"
     end

    def tags
      @paged_articles = articles.search(:page=>'1',:tags=>params[:tag_name], :per_page=>@@articles_per_page)
       @tags = get_tags
       @next_page_url = url_for(:controller =>:home, :action => :page, :tag_name => params[:tag_name], :page_id=>  2)
       @page_title = "Search articles by tags"
       render :index
     end

    def about
       @page_title = "About"
    end

   private
    def get_tags
         articles.getTags 'param1', 'param2'
    end
   end

I simplified it this way

class HomeController < ApplicationController
  @@articles_per_page = 6
  @@first_page_articles_options = {:page=>'1', :per_page=>@@articles_per_page}
  @@second_page_url_options = {:controller =>:home, :action => :page, :page_id=>  2}

  def index
    @paged_articles = articles.search(@@first_page_articles_options)
    @tags = get_tags
    @next_page_url = url_for(@@second_page_url_options)
    @page_title = "Recent articles"
  end

  def tags
    @@first_page_articles_options[:tags]  = params[:tag_name]
    @paged_articles = articles.search(@@first_page_articles_options)
    @tags = get_tags

    @@second_page_url_options[:tag_name]  = params[:tag_name]
    @next_page_url = url_for(@@second_page_url_options)
    @page_title = "Search articles by tags"
    render :index
  end
   def about
      @page_title = "About"
  end

  private
  def get_tags
         articles.getTags 'param1', 'param2'
  end
end

So what do you think? Is there any way to make it more better and cleaner?

You can move urls and titles to helpers and articles search to separate method

class HomeController < ApplicationController
  # this methods can be used in view template
  helper_method :next_page_url, :page_title

  @@articles_per_page = 6
  @@first_page_articles_options = {:page=>'1', :per_page=>@@articles_per_page}
  @@second_page_url_options = {:controller =>:home, :action => :page, :page_id=>  2}

  def index
    @paged_articles = articles.search(@@first_page_articles_options)
    @tags = get_tags
  end

  def tags
    @paged_articles = load_articles
    @tags = get_tags

    render :index
  end
   def about
      @page_title = "About"
  end

  private

    def load_articles
      ...
    end

  ...

  # helpers

  def next_page_url
    if filter_by_tags?
      url_for(@@second_page_url_options)
    else
      @@second_page_url_options[:tag_name] = params[:tag_name]
      url_for(@@second_page_url_options)
    end
  end

  def page_title
    if filter_by_tags?
      "Recent articles"
    else
      "Search articles by tags"
    end
  end

  def filter_by_tags?
    params[:tag_name].present?
  end
end


"Pass the Pigs" game I was wondering if anyone could give a critique of my code. We are supposed to be using inheritance and OOP principles. All the functions but the PlayerSet class were given, so really I just need someone to critique that class.

The full code is here.

class PlayerSet:
    '''A group of players to play
    the Pass The Pigs game.'''

    def __init__(self,playerCount):
        '''PlayerSet(int) -> PlayerSet
        Contstructs a group of players,
        given how many there are and asks the
        name of each player.'''
        self.playerCount = playerCount # initialize count of players attribute
        self.playersList = [] # initialize list of players attrbute
        for i in range(1,self.playerCount+1): # for every i between 1 and plyerCount
            name = input('Player ' + str(i) + ', please enter your name:') # ask player i's name
            player = Player(name) # create a player with that name
            self.playersList.append(player) # add the player to the list of players
        self.currentPlayer = self.playersList[0] # initialize current player attribute

    def __str__(self):
        '''print(PlayerSet) -> str
        Returns all of the player's
        name and score.'''
        string='' # will contain output
        for player in self.playersList: # for every player in the list of players
            string+= player.get_name() + ' has ' + str(player.get_score()) + ' points \n' # add a string saying the player's name and current score
        return string # return the string

    def get_curr_name(self):
        '''PlayerSet.get_curr_name() -> str
        Returns the name of the current player.'''
        return self.currentPlayer.get_name() # return the name of the player

    def go_to_next_player(self):
        '''PlayerSet.go_to_next_player()
        Moves the current player to the next
        player.'''
        index = 0 # will contain index of the current player
        for i in range(len(self.playersList)): # for every index in list of players
            if self.playersList[i] == self.currentPlayer: # if that index is correct
                index = i # assign index to the desired index

        index += 1 # add 1 to the desired index, we want to move to next player
        index %= self.playerCount # if the number of the player goes over, take the residue mod playerCount
        self.currentPlayer=self.playersList[index] # make the current player equal to the new index of the list of players

    def is_curr_winner(self):
        '''PlayerSet.is_curr_winner() -> bool
        Returns if the current player has won.'''
        if self.currentPlayer.get_score() >= 100: # if the current player has won
            return True
        else: # otherwise
            return False

    def take_turn(self):
        '''PlayerSet.take_turn()
        Takes the player's turn.'''
        self.currentPlayer.play_turn() # use player's method - play_turn() which plays a turn

    def play_game(self):
        '''PlayerSet.play_game()
        Plays the real Pass The Pigs game! Woohoo!'''
        while self.is_curr_winner() == False: # until a player has won
            print(self) # print the status of the players
            print(self.get_curr_name() + ", it's your turn") # print whose turn it is
            self.take_turn() # take the player's turn
            if self.is_curr_winner() == True: # if the current player has won
                print(self) # print the status of the players
                return self.get_curr_name() + ' has won!' # return which player has won
            self.go_to_next_player() # go to the next player

The class could be better named. PlayerSet is not just a set of players: it also knows how to play the game. I would consider calling it PassThePigs since it implements the logic for the game.
Most of your comments are unnecessary. Now, I understand that you're learning to program so you probably find it helpful at this stage to explain each line of code you write. But as you get more experienced, you'll realise that it's not necessary to write comments that merely say the same thing as the code. In cases like this:

return self.currentPlayer.get_name() # return the name of the player

you should find that you can easily read the code:

return self.currentPlayer.get_name()

as meaning "return the name of the current player".

Comments like return the name of the player eventually become a burden rather than a help, because you have to remember to update the comment every time you update the code. For example, it would be all too easy to end up with code like this:

return self.currentPlayer.get_score() # return the name of the player

where the code has been changed and the comment is now wrong.

The best comments explain things that can't easily be figured out by reading the code, for example why you wrote the code in a particular way. Here are a couple of my comments for comparison:

From here:

assert(limit == 10 ** max_digits) # algorithm works for powers of 10 only

And from here:

dt = self.clock.tick(FPS) / 1000.0 # convert to seconds

The Python style guide (PEP 8) suggests that you

  Limit all lines to a maximum of 79 characters [... This] makes it possible to have several files open side-by-side, and works well when using code review tools that present the two versions in adjacent columns.

Also, shorter lines wouldn't disappear off the right hand side of code blocks here on Code Review.

You're not obliged to follow PEP 8, but it makes it easier for you to collaborate with other Python programmers.
There's no need to store the number of players in self.playersCount because you can always get the number of players as len(self.playersList). Storing a fact in two places leads to a risk of inconsistency.
Instead of:

'Player ' + str(i) + ', please enter your name:'

consider:

'Player {}, please enter your name:'.format(i)

Building a string by repeatedly appending to it is an anti-pattern in Python: it takes time that's quadratic in the number of append operations, because each += operation creates a new string. It's nearly always better to use the str.join method. So this code:

string=''
for player in self.playersList:
    string+= player.get_name() + ' has ' + str(player.get_score()) + ' points \n'
return string

is better written:

return ''.join('{} has {} points\n'.format(player.get_name(), player.get_score())
               for player in self.playersList)

Python does not generally need accessor methods like the get_name and get_score in your Player class. Instead of player.get_name(), consider using just player.name. That allows you to write:

return ''.join('{0.name} has {0.score} points\n'.format(player)
               for player in self.playersList)

If the player has exactly 1 point, then this is going to produce ungrammatical output:

Parminder has 1 points

You need something like:

'{0.name} has {0.score} point{1}\n'.format(player, '' if player.score == 1 else 's')

The purpose of the __str__ method is "to compute the “informal” or nicely printable string representation of an object". But you are using it here to format the players' current scores. It would be better to use a method name like format_scores.
The go_to_next_player method seems very complicated. First, you have a loop where you try to find the index of the current player in the list:

index = 0
for i in range(len(self.playersList)):
    if self.playersList[i] == self.currentPlayer:
        index = i

Whenever you find yourself iterating over the indexes of a list, consider using the enumerate function:

index = 0
for i, player in enumerate(self.playersList):
    if player == self.currentPlayer:
        index = i

Second, when you find the player in the list, you set index but you carry on looping. It would be better to break out of the loop as soon as you find what you are looking for:

index = 0
for i, player in enumerate(self.playersList):
    if player == self.currentPlayer:
        index = i
        break

I've shown you how to rewrite the loop, but in fact no loop is needed. Python comes with a built-in method list.index for this operation:

index = self.playersList.index(self.currentPlayer)

Finally, why go to this effect of looking up the current player in the list of players, when you could just remember the index instead of the player? In __init__ you could write:

self.current_player_index = 0

combined with a property:

@property
def currentPlayer(self):
    return self.playersList[self.current_player_index]

and then the go_to_next_player method would be very simple:

self.current_player_index = (self.current_player_index + 1) % len(self.playersList)

A more "advanced" approach would be to use itertools.cycle. In __init__ you'd write:

self.player_cycle = itertools.cycle(self.playersList)
self.currentPlayer = next(self.player_cycle)

and then the go_to_next_player method would become:

self.currentPlayer = next(self.player_cycle)

This code:

if self.currentPlayer.get_score() >= 100:
    return True
else:
    return False

can be simplified to:

return self.currentPlayer.get_score() >= 100

since Boolean expressions like a >= b evaluate to True or False.
Instead of:

while self.is_curr_winner() == False:

you could write:

while not self.is_curr_winner():

but actually this should be

while True:

since the winner detection has already been done (after the player took their turn).
The play_game method returns a string but this is not mentioned in the documentation for the method. (In fact, it's probably a mistake.)
Consider giving a name to the value 100 so that it's clear what it means and so that it's easy to find and change if you need to.
When you have a method that's called from only one place, and is only one line long, consider whether it's worth having a method at all. Adding methods is good because it improves the clarity of the code (you can give the method a name that explains its purpose, and provide documentation), but on the other hand each method increases the length of the code and makes it harder to read.
Once you do this inlining process, it will become clear that there isn't really any persistent state in the game: everything important happens inside PlayerSet.play_game. So is it really worth having a class at all? Compare your implementation with this one:

from itertools import cycle

def pass_the_pigs(self, n, winning_score=100):
    """Play a game of Pass the Pigs with n players."""
    players = [Player(input('Player {}, please enter your name:'.format(i)))
               for i in range(1, n + 1)]
    for player in cycle(players):
        for p in players:
            print('{0.name} has {0.score} point{1}'
                  .format(p, '' if p.score == 1 else 's'))
        for p in players:
            if p.score >= winning_score:
                print('{0.name} has won!'.format(p))
                return
        print("{0.name}, it's your turn".format(player))
        player.play_turn()

(Note that there's almost nothing here that's specific to Pass the Pigs — the same code would work as the main loop for any turn-based game where the players do not interact with each other, and where there's a winning score.)


Implementing PDO, code correct so far? I have decided to go PDO and have started implementing it on one of my webprojects. I have limited programing skills but can usually pull it off with some tutorials and forums.

Here is my code this far and it works fine but how is the code correct regarding the picking up errors, syntax, order and begintransaction etc.? Have I missunderstood anything? Is anything unneccesary?

connect.php

 <?php
try {
    $DBH = new PDO("mysql:host=$host;dbname=$database", $username, $password, array(
        PDO::ATTR_PERSISTENT => true
    ));
    $DBH -> exec("set names utf8");
}
catch(PDOException $e) {
    echo $e->getMessage();
}
?>

query.php

try {
        $DBH->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

        $DBH->beginTransaction();

        $STH = $DBH->prepare('SELECT id from users where uid = :uid');
        $STH->setFetchMode(PDO::FETCH_ASSOC);
        $STH->bindParam(':uid', $uid); // $uid value is set
        $STH->execute();

        $uid_in_db = $STH->rowCount();
        if($uid_in_db==0){ //=new user, insert info in db.
            $STH = $DBH->prepare("INSERT INTO USERS (uid,namn) VALUES (:uid, :name)");
            $STH->bindParam(':uid', $uid);
            $STH->bindParam(':namn', $_POST['namn']); // a value posted form user input
            $STH->execute();
        }

        $DBH->commit(); //

    } catch (Exception $e) {
        $DBH->rollBack();
        echo "Fel: " . $e->getMessage();
    }
}else{
    $error=1;
}

Instead of checking for a UID collision before an insert, I would put a unique key on that column, and then write fallback code for the case (very very unlikely) when two collide. That way you can simplify your logic and reduce the number of queries to one instead of two. You also don't need a transaction if you implement this change, since there is only 1 query.

//you can remove the outer try/catch since only the execute() should possibly fail
$DBH->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); //<~ put this in the initialize of the db connection
$STH = $DBH->prepare("INSERT INTO USERS (uid,namn) VALUES (:uid, :name)");
$STH->bindParam(':uid', $uid);
$STH->bindParam(':namn', $_POST['namn']); // a value posted form user input
try {
    $STH->execute();
} catch( PDOException $e ) {
    //deal with collision
}


Writing nested for loops to produce certain output My task:

  Write nested for loops to produce the following output with each line
  48 characters wide:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~+~~+~~+~~+~~+~~+~~+~~+~~+~~+~~+~~+~~+~~+~~+~~+~
+~++~++~++~++~++~++~++~++~++~++~++~++~++~++~++~+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This is my idea, but I guess it's pretty much redundant. If so, how can I minimize redundancy here? My output is correct, by the way.

public class Exercises {
    public static void main (String[] args){

    for (int i = 1; i <= 4; i++){
        for (int j = 1; j <= 12; j++){
            System.out.print("~");
        }
    }
            System.out.println();
            System.out.print("~");

    for (int i = 1; i <=15; i++){
            System.out.print("+~~");
    }
            System.out.println("+~");
            System.out.print("+~");
    for (int i = 1; i <= 15; i++){
            System.out.print("++~");
        }
            System.out.println("+");

    for (int i = 1; i <= 4; i++){
        for (int j = 1; j <= 12; j++){
            System.out.print("~");
        }
    }
  }
}

You're not breaking up the patterns in the most logical way.  Each line just consists of repeating blocks of three characters.

public class SquigglePlus {
    private static final String[] PATTERNS = { "~~~", "~+~", "+~+", "~~~" };

    public static void main(String[] args) {
        for (int line = 0; line < PATTERNS.length; line++) {
            for (int col = 0; col < 48; col += PATTERNS[line].length()) {
                System.out.print(PATTERNS[line]);
            }
            System.out.println();
        }
    }
}

Alternatively, the inner loop could just count to 16: for (int i = 0; i < 16; i++) { … }.  Note that in Java, that is the most idiomatic way to count: start with 0, and use < for the termination check.  The other way (for (int i = 1; i <= 16; i++) { … }) would not be wrong, but usually you would write it that way only if you had a special reason.

Better yet, use a modern enhanced for-loop instead of the old-style counting loop.

public class SquigglePlus {
    private static final String[] PATTERNS = { "~~~", "~+~", "+~+", "~~~" };

    public static void main(String[] args) {
        for (String pattern : PATTERNS) {
            for (int col = 0; col < 48; col += pattern.length()) {
                System.out.print(pattern);
            }
            System.out.println();
        }
    }
}


jQuery toggle sort order and save to variable This works fine but I find it quite big and had hoped there was a cleaner/smaller "toggle" function or alike which I could use but it seems to be related to visibility only - and I want to set a variable (for later usage).

Can this be optimized, if I want a toggle function which should be used to sort a column (and get the value into a variable)?

jsFiddle

    Col 1
    Col 2
    Col 3

    A
    B
    C

    B
    C
    A

And the jQuery stuff:

var column;
var order;

$('thead th').click(function () {

  // Get the current column clicked
  var thisColumn = $(this).text();

  // Check if the column has changed
  if(thisColumn == column) {
    // column has not changed

    if(order == "ascending") {
      order = "descending";
    } else {
      order = "ascending";
    }
  } else {
    // column has changed

    column = thisColumn;
    order = "descending";
  }

  // Replace text in DIV
  $("div").text("column=["+column+"], order=["+order+"]");

  // future code will use the sort order to get database
  // stuff with Ajax

});

Mostly I'ma java developer, but I think that there are a couple of things that you can do to tidy the code up.

If you do not want to store state in the DOM then you can remove the jQuery data statements and store in the global scope. If I were a jQuery developer I would probably wrap this functionality into a plugin (sortable?) and then to call it on a table with the sortabletable calss use $('.sortableTable').sortable(). It would be more reusable, more encapsulated, and less likely to cause you weird behavior if you have two tables in your page!

However here's my quick take on the above code:

var column;

$('thead th').click(function () {

    // Get the current column clicked
    var thisColumn = $(this);

    if (thisColumn.is(column)) {
        var sort = column.data("sort");
        var newSort = toggle(sort);
        column.data("sort", newSort);
    } else {
        column = thisColumn;
        column.data("sort", false);
    }
    // Replace text in DIV
    $("div").text("column=[" + column.text() + "], order=[" + column.data("sort") + "]");

});

function toggle(current) {
    return !current;
}

You could skip over setting an initial state and have toggle (and you Ajax code) work with undefined values properly (returning false).


Interleaving of two strings I have written this recursive code to find all interleaves of two strings, where the order of strings are preserved. For example:

s1="abc" s2="1"
a1bc
ab1c
abc1

You can see that the order of s1 and s2 is preserved.

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

#include "stdafx.h"
#include "iostream"
#include "vector"
#include "string"
using namespace std;
class Solution {
public:
    vector<string> res;
    void helper(string s1, string s2,string s3) {
        if (s1 == "" && s2 == "") {
            if(std::find(res.begin(),res.end(),s3)==res.end())
                res.push_back(s3);
            return;
        }
        if (s2 == "") {
            res.push_back(s3+s1);
            return;
        }
        if (s1 == "") {
            res.push_back(s3 + s2);
            return;
        }
        for (int i = 0; i < s1.size(); i++) {
            for (int j = 0; j < s2.size(); j++) {
                helper(s1.substr(i + 1, s1.size()),s2.substr(j+1,s2.size()),s3+s1.substr(0, i+1)+s2.substr(0,j+1));
            }
        }
    }
    void interleave(string s1, string s2) {
        if (s1==""|| s2=="")
            return;
        for (int i = 0; i < s2.size(); i++) {
            helper(s1, s2.substr(0, i+1),"");
        }
        res.pop_back();
    }
};
int main()
{
    Solution s;
    s.interleave("abc", "123");
    return 0;
}

Here are some observations that may help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Isolate platform-specific code

If you must have stdafx.h, consider wrapping it so that the code is portable:

#ifdef WINDOWS
#include "stdafx.h"
#endif

Make sure you have all required #includes

The code uses std::find but doesn't #include <algorithm>.

Be careful with signed and unsigned

In the current helper and interleave routines, the loop integer i and j are both signed int values, but they're being compared with size_t quantities (which are unsigned) s1.size() and s2.size().  Better would be to declare them all as size_t.

Understand how #include works

On most platforms, the difference between #include "iostream" and #include <iostream> is that the former looks first in the current directory.  So for system files such as iostream, you should really use #include <iostream> instead.  See this question for more details.

Rethink your algorithm

The result of this program is somewhat peculiar.  If we run it with ab and 12 we get this:

  a1b, ab1, a1b2, a12b

This seems to be incorrect because the first two don't include all of the input letters and none of the combinations begin with 1 and the combination ab12 is not listed. If this is intended, then it's just the description that is faulty.  If not, it's a bug and should be fixed.

Hide internal object details

If the sole purpose for the object is to create a vector of interleaved strings, it seems to me that interleave should actually return a vector of strings rather than keeping it as an internal object.  The helper function should also be made private if it's not intended to be used externally.  Finally, if the object has no data, as per this recommendation, the member functions can all be declared static so that no object instantiation need exist to use the function.

Pass by const reference where practical

All of the std::string arguments are being passed by value, which cause the argument to be duplicated. Better would be to make each of them const std::string & because they are not modified and don't need to be duplicated.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments:  "I didn't know that." or "That's bad advice!"  My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard.  For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

  [...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

  If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea.  We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function.  Reasons against omitting seem to boil down to "it looks weird".  If, like me, you're curious about the rationale for the change to the C standard read this question.  Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!)  In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.


Returning preferences I have a function that returns some preferences, but first I have to do some checks to see whether the user is authorised to read them. I'm still not happy with the result (the match is a bit ugly).

Note: Result is a framework class, it just means some kind of response.

// implementations are not relevant.
def canAccessPreferences(id: Long): Future[Boolean] = ???
def doGetPreferences(id: Long): Future[Result] = ???

def getPreferences(id: Long) = Future[Result] {

  canAccessPreferences(id).flatMap {
     case false =>
       Future.successful(Forbidden)
     case true =>
       doGetPreferences(id)
  }
}

The only alternative is using a for comprehension:

for {
  hasAccess <- canAccessPreferences(id)
  result <- if (hasAccess)
    Future.successful(Forbidden)
  else
    doGetPreferences(id)

} yield result

Unfortunately you cannot use the if guard of the for because then you would get an exception if hasAccess is false. And since you're composing Future monads there's no way around the Future.successful either.


Enum, constants, or other to represent chess pieces I'm learning C# and I decided to write a chess program to help me practice the concepts I'm learning in my book. I started the Board class tonight, which is going to handle 1) the board state and 2) checking if moves are legal.

I feel like I need to give careful consideration to how I represent the board. I started with a list of constants:

// CONSTANTS
private const sbyte EMPTYSQUARE = 0;
private const sbyte WKING = 1;
private const sbyte WQUEEN = 2;
private const sbyte WROOK = 3;
private const sbyte WBISHOP = 4;
private const sbyte WKNIGHT = 5;
private const sbyte WPAWN = 6;
private const sbyte BKING = 7;
private const sbyte BQUEEN = 8;
private const sbyte BROOK = 9;
private const sbyte BBISHOP = 10;
private const sbyte BKNIGHT = 11;
private const sbyte BPAWN = 12;

private const sbyte GAMEINPROGRESS = 0;
private const sbyte CHECKMATEWHITE = 1;
private const sbyte CHECKMATEBLACK = 2;
private const sbyte NOLEGALMOVESDRAW = 3;
private const sbyte THREEMOVEDRAW = 4;
private const sbyte FIFTYMOVEDRAW = 5;
private const sbyte WHITERESIGNED = 6;
private const sbyte BLACKRESIGNED = 7;

// VARIABLES
private sbyte[,] board =
{
    {9, 11, 10, 8,  7,  10, 11, 9},
    {12,12, 12, 12, 12, 12, 12,12},
    {0, 0,  0,  0,  0,  0,  0,  0},
    {0, 0,  0,  0,  0,  0,  0,  0},
    {0, 0,  0,  0,  0,  0,  0,  0},
    {0, 0,  0,  0,  0,  0,  0,  0},
    {6, 6,  6,  6,  6,  6,  6,  6},
    {3, 5,  4,  2,  1,  4,  5,  3}
};

Then I realized I could use an enum here, which I hear is good practice so you don't go mixing up your "magic numbers":

private enum Piece
{
    EMPTY, WKING, WQUEEN, WROOK, WBISHOP, WKNIGHT, WPAWN, BKING, BQUEEN, BROOK, BBISHOP, BKNIGHT, BPAWN
}

private enum State
{
    GAMEINPROGRESS, CHECKMATEWHITE, CHECKMATEBLACK, NOLEGALMOVEDRAW, THREEMOVEDRAW, FIFTYMOVEDRAW, WHITERESIGNED, BLACKRESIGNED
}

I then go to declare the board array and I had to use this ugly-looking monstrosity:

private sbyte[,] board =
{
    {(sbyte)Piece.BROOK, (sbyte)Piece.BKNIGHT, (sbyte)Piece.BBISHOP, (sbyte)Piece.BQUEEN, (sbyte)Piece.BKING, (sbyte)Piece.BBISHOP, (sbyte)Piece.BKNIGHT, (sbyte)Piece.BROOK},
    {(sbyte)Piece.BPAWN, (sbyte)Piece.BPAWN, (sbyte)Piece.BPAWN, (sbyte)Piece.BPAWN, (sbyte)Piece.BPAWN, (sbyte)Piece.BPAWN, (sbyte)Piece.BPAWN, (sbyte)Piece.BPAWN},
    {(sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY},
    {(sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY},
    {(sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY},
    {(sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY, (sbyte)Piece.EMPTY},
    {(sbyte)Piece.WPAWN, (sbyte)Piece.WPAWN, (sbyte)Piece.WPAWN, (sbyte)Piece.WPAWN, (sbyte)Piece.WPAWN, (sbyte)Piece.WPAWN, (sbyte)Piece.WPAWN, (sbyte)Piece.WPAWN},
    {(sbyte)Piece.WROOK, (sbyte)Piece.WKNIGHT, (sbyte)Piece.WBISHOP, (sbyte)Piece.WQUEEN, (sbyte)Piece.WKING, (sbyte)Piece.WBISHOP, (sbyte)Piece.WKNIGHT, (sbyte)Piece.WROOK}
};

It's really making me second-guess whether or not I want to keep using enum. That's a lot of typing and typecasting just to program in one chess board.

What do you think? Should I go constants or enums?

And while we're at it, what do you think of my choice of data structure for a chess board? Are there faster structures? Are there smaller structures? (I only need to store numbers 0 through 12) Should I enlarge the array to help me compute off-the-board knight moves? Should I be using a different data structure entirely?

I think it definitely makes sense to use an object hierarchy here. What's the difference between a black and a white pawn? Its colour! (which will affect which way it can move relative to the board).

All pieces must have exactly one colour, and we have two to chose from:

public enum PieceColour
{
    White = 0,
    Black = 1
}

As we've already reasoned, all pieces have to have a colour and unless you've got a defective set, none of the pieces can change colour midway through. Sounds like we have an invariant that we want all pieces to obey. We can lock that up with a base class:

public abstract class Piece
{
    private readonly PieceColour colour;

    public PieceColour Colour { get { return colour; } }

    protected Piece(PieceColour colour)
    {
        this.colour = colour;
    }
}

By making the piece class abstract, we are saying "you can't have a generic Piece, you have to have a specialized subclass". Let's see what that some of those might look like:

public class Pawn : Piece
{
    public Pawn(PieceColour colour)
        : base(colour)
    {
    }
}

public class Rook : Piece
{
    public Rook(PieceColour colour)
        : base(colour)
    {
    }
}

public class Knight : Piece
{
    public Knight(PieceColour colour)
        : base(colour)
    {
    }
}

Now you can start playing around with a board. To be completely honest, I haven't thought enough about it to figure out how I'd really want it to look but something like the following would be enough to get along with. Tuple<int, int> is just a convenient wrapper around two ints (the coordinates of a space).

public class Board
{
    private Dictionary<Tuple<int, int>, Piece> currentState;

    public static Board CreateNewBoard()
    {
        var board = new Board();

        board.currentState = new Dictionary<Tuple<int, int>, Piece>
        {                      // x, y
            { new Tuple<int, int>(0, 0), new Rook(PieceColour.White) },
            { new Tuple<int, int>(1, 0), new Knight(PieceColour.White) }
            // etc.
        };

        return board;
    }
}

}

Any common behaviour you should find behind all pieces you should add it to the base class Piece, for example they can all move...

Edit

I thought the above made it clear that I think there should be an abstract method on the piece class that deals with movement. I didn't commit to making a suggestion on the signature of the method because I haven't seen what you're doing currently. I have the impression OP intends on "moving pieces" entirely within the board class.


Sequence Alignment of two Strings I have some code which aligns the sequence of two strings, I am doing numbers just for my implementation.

I was wondering whether there are any performance enhancements I could make as the code itself is \$O(n^2)\$ which isn't ideal in terms of scalability, here's the code:

        int[] goal = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14,15, 0};
        int[] test = {1, 5, 9, 11, 2, 10, 0, 6, 8, 14, 15, 12, 7, 4, 3, 13};
        int gap = 1;
        // The penalties to apply
        int substitution = 1, match = 0;

        int[][] opt = new int[goal.length + 1][goal.length + 1];

        // First of all, compute insertions and deletions at 1st row/column
        for (int i = 1; i <= goal.length; i++) {
            opt[i][0] = opt[i - 1][0] + gap;
        }
        for (int j = 1; j <= test.length; j++) {
            opt[0][j] = opt[0][j - 1] + gap;
        }

        for (int i = 1; i <= goal.length; i++) {
            for (int j = 1; j <= test.length; j++) {
                int scoreDiag = opt[i - 1][j - 1] + (goal[i - 1] == test[j - 1] ? match : substitution); // different symbol
                int scoreLeft = opt[i][j - 1] + gap; // insertion
                int scoreUp = opt[i - 1][j] + gap; // deletion
                // we take the minimum
                opt[i][j] = Math.min(Math.min(scoreDiag, scoreLeft), scoreUp);
            }
        }

        for (int i = 0; i <= goal.length; i++) {
            for (int j = 0; j <= test.length; j++) {
                System.out.print(opt[i][j] + "\t");
            }
            System.out.println();
        }

The value that I actually need from the code is opt[goal.length][test.length]

One general way to speed up dynamic algorithms is the "Method of Four Russians" - \$O(\frac{n^2}{\log n})\$. But to implement this method you should use limited alphabet (not like numbers in your implementation).

Although theoretically this method looks awesome, practically the size of a lookup table is growing fast (\$O(3^{2t} \cdot \left|A \right| \cdot t)\$, where \$\left|A \right|\$ is the size of an alphabet, and \$t\$ is a block size). For example, in the case of DNA (4 letters alphabet) it can fit to the modern cash memory just in the case of \$t\$ equals to 3 (\$\sim0.1MB\$) or 4 (\$\sim10MB\$).

Another way to optimize the pairwise alignment is to take advantage of an extended instruction set of modern CPUs (SIMD extension). Up to now there are several such implementations of the Smith-Waterman algorithm (1 or 2).


Supermarket app using OOP I am going in a circle over and over trying to write a "supermarket" application using OOP.

The app should be useful for the employees of a supermarket. The app reads data from a CSV file which looks like this:

name,amount
soap,4
rice,5
bread,10

The supermarket has cashiers and managers. Cashiers should only be able to view the amount for a product. Managers can view but also change an amount.

Here's the code I came up with:

import pandas

class Data:
    """Creates a pandas dataframe out of a text file"""
    def __init__(self, datafile = "products.txt"):
        self.datafile = datafile

    def get_dataframe(self):
        df = pandas.read_csv("products.txt", index_col = "name")
        return df

    def save_to_csv(self):
        df = pandas.read_csv("products.txt", index_col = "name")
        df.to_csv(self.datafile)
        print(df)

class Product:
    """Represent a product from the dataframe table"""

    def __init__(self, name, dataframe = Data().get_dataframe()):
        self.name = name
        self.dataframe = dataframe

    def get_amount(self):
        """Return the available amount of the product object taken from the dataframe"""
        return self.dataframe.loc[self.name, "amount"]

    def add_amount(self, amount, password):
        """Changes the dataframe by adding an amount to the existing amount"""
        if password == 1234:
            self.dataframe.loc[self.name, "amount"] += amount
            data = Data()
            data.save_to_csv()
            print("Saved")
        else:
            return "You're not a manager"

    def remove_component(self, person):
        """Changes the dataframe by removing an amount from the existing amount"""
        if password == 1234:
            self.dataframe.loc[self.name, amount] -= 1
        else:
            return "You're not a manager"

    def __del__(self):
        """Delete the dataframe from memory when Product is destroyed"""
        del self.dataframe
        Data().save_to_csv()

I think the authentication layer is very poorly implemented here. I simply added a conditional in the add and remove methods. Is there any better way to do the authentication part?

Should I also create Manager and Cashier classes in case I expand the program later. For example, adding info about salaries for managers and cashiers. That would require to have Manager and Cashier objects I guess.

Will the code load many Data objects? Should I use a singleton for the Data object?

What other design pattern could I have used for this app?

From an OOP design perspective, you've duplicated the sales items. Data is the products, and Products are the products too (hope that makes sense?).

Think back for a second regarding what entities are at work here, and their specific functions. We have:

a manager,
a cashier,
a terminal (which the cashier and manager can both use),
and all the products on the store shelves.

Now, both managers and cashiers are of type Person, however Manager can override the Person class with an Edit function (for the products), whereas Cashier would just inherit Person with the in-built View function, correct? (are you with me so far?)
Now, we can be cheeky and assume that the Terminal has Loading and Saving functions - in lieu of the mainframe in the manager's office (typically terminals are just dumb devices and the price editing is handled by the manager with a list of products sent by head office).
Given that the Manager and the Cashier both need to use the Terminal, you would need to have a Logon and Logoff function, correct? That implies a state pattern, which I'll leave you to research.

Now, your question listed the desired functionality:

read/write CSV
Cashiers have view functionality
Managers have view and edit functionality.
Authentication
Potentially having salaries on managers and cashiers

All these can be achieved in the OOP layout I wrote above. The Authentication component of course would be tied into the Type of which (inherited) Person is logged onto the Terminal (that implies you either need to do some reflection on the logged on user or have an attribute which gives the answer to the terminal).
You also ask:

  Will the code load many Data objects?

That's hard to say, but it sounds like if you stick with a simple approach, it should keep them to a minimum. Also you ask:

  Should I use a singleton for the Data object?

Given the Terminal is responsible for the product data, if the Terminal loads and saves the product data, I don't think you need to worry about that.

Hope this helps!


Most optimized way to get minimum strpos from multiple needles function min_strpos_array($haystack, $needles) {
    $min = null;
    for ($i = 0; $i < count($needles); $i++) {
        $strpos = strpos($haystack, $needles[$i]);
        if ($strpos !== FALSE) {
            $min = $strpos;
            for ($i = $i + 1; $i < count($needles); $i++) {
                $strpos = strpos($haystack, $needles[$i]);
                if ($strpos !== FALSE) {
                    $min = $min < $strpos ? $min : $strpos;
                }
            }
        }
    }
    return $min;
}

echo min_strpos_array('hello#,?', array('#', '?')); //5

Well, any time you have nested for loops, you should cringe a little because it means you're running in O(n^2) time. Sometimes it's unavoidable, but often you can do something to bring it down significantly. Edit: this algorithm is actually O(n).

In your case, you can reduce your algorithm to O(n), which is a huge performance boost.

Here are your new steps:

Loop through each needle
    Get the position of the needle in the haystack
    If the position is less than our current minimum:
        Set the minimum to the position of the needle

Basically, we find the first minimum and then overwrite it as each new minimum is found.

Here is my version of your code:

function min_strpos_array($haystack, $needles){
    $length = count($needles);
    $min = strlen($haystack) + 1;
    for($i = 0; $i < $length; $i++){
        $strpos = strpos($haystack, $needles[$i]);
        if ($strpos !== FALSE){
            if($strpos < $min){
                $min = $strpos;
            }
        }
    }
    return $min;
}

echo min_strpos_array('hello#,?', array('#', '?')); //5

Outside of the algorithm, I made a few other changes.

for ($i; $i < count($needles); $i++) became for ($i = 0; $i < $length; $i++). When you have a function in a for loop, it gets called every iteration, which is unnecessary when the value doesn't change. Just get the number of needles outside the loop and reference that. You could also use a foreach loop, but I'm not familiar enough with PHP to definitively say that it's better.

$min = null; became $min = strlen($haystack) + 1;. This ensures that if our needle exists in the haystack, we can compare the position with our minimum. It also has the side effect of returning strlen($haystack) + 1 if none of the needles are found in the haystack, which may or may not be desirable. If you don't like that, just add an if statement before you return.

You really don't need to nest your if statements like you did either, but I decided to leave those in in case you chose to so intentionally.

Two important things to take away from this:

Watch out for nested for loops. Pretend they're screaming at you to find a better algorithm.
Don't call functions in the argument of a for loop unless you need to get a value that might change during the loop.


turning the digits of a number into an array int main()
{
    int x,numDigits,i,y,a,diff;
    scanf("%d",&x);

    a=x;
    int result = 1;
    int base=10;
    numDigits=0;

    do
    {
        ++numDigits;
        x = x / 10;
    } while ( x>0 );

    for(result=1;numDigits>0;--numDigits){
        result *= base;
        i=result;

        y=a%i;
        while(y>10){
            y=y/10;
        }
        printf(" %d",y);
    }
    return 0;
}

I manage to seperate integer's digits one by one. But I was not able to collect them in an array. Even though with limited numbers without taking input from user it is easier, I also want to process these numbers but I got stuck.

You can use the standard C function sprintf(buffer,"%d",x) to get an array of digits. In this case, the buffer will contain big-endian representation of x, so the conversion to a digit from each char will be something like this

for(int i = 0; buffer[i] != '\0' && i < sizeof(buffer);++i)
  digits[i] = buffer[i] - '0';

This work if x is positive.

But if you want to make your own implementation of splitting a number into digits, you should analyze the problem:

What is the base - is it always 10?
How many digits could be - is it always int, i.e. mostly 32-bit?

The answer for first question is to have the base parameter int base = 10 or 2 or 3 or 8 or any you want.

The answer for second question is ceil( log(|x| + 1) / log(base) ) + 1. The +1 for the minus sign. Or if you didn't want to care about maximum of digits use char digits[128] - more than enough even for 64-bit integer.

So, how to fill digits?

// int x - somewhere
unsigned base = 10;

char digits[128];
char isNegative = 0;
int digit = 0;

unsigned tmp = x; // this fix INT_MIN problem thanks chux - Reinstate Monica for note
if(x < 0) {
  isNegative = 1;
  tmp = -x;
}
if(x == 0) {
  digits[0] = 0;
  digit = 1;
} else {
  for(digit = 0; tmp > 0; ++digit) {
    digits[digit] = tmp % base;
    tmp /= base;
  }
}

At least you got little-endian representation of x on base as digits[i], where 0 <= i < digit and isNegative for the minus sign


Mimic sprintf with std::string output I know that stringstreams are the C++ recommended way to create formatted text.  However, they can often become quite verbose, especially when compared to the succinct format strings of printf and family.  However, the printf family can lead to its own issues, with buffer overflow issues, and so I would rather not use these functions directly

My goal was to make a function to behave similarly to snprintf, but return a std::string, avoiding any chance of buffer overflow.  Knowing that there are many pitfalls to string manipulation in C++, have I exposed myself to any errors in using this function?

#include <cstdio>
#include <cstdarg>
#include <string>

std::string string_sprintf(const std::string& format, ...){
  static const int initial_buf_size = 100;
  va_list arglist;
  va_start(arglist, format);
  char buf1[initial_buf_size];
  const int len = vsnprintf(buf1,initial_buf_size,format.c_str(), arglist) + 1;
  va_end(arglist);

  if(len<initial_buf_size){
    return buf1;
  } else {
    char buf2[len];
    va_start(arglist,format);
    vsnprintf(buf2,len,format.c_str(),arglist);
    va_end(arglist);
    return buf2;
  }
}

Since you're using cstdarg and va_list is a complete type, it should be std::va_list.

va_start with a std::string is undefined behavior. This means that you actually cannot use std::string as your format (at least not if you plan on using only the stdargs facilities).

It's literally impossible, since there's no way to forward the ... idea, but instead you can only forward a va_list, which of course presents a problem since you can't obtain a va_list with a std::string parameter.

Using vsnprintf implies that you're using C++11. Given this, I might be tempted to go the variadic template route rather than leveraing the old C style functions that don't have type safety. Unfortunately though, much of the parsing and transforming logic would have to be recreated. If you do want to consider going this route, Andrei Alexandrescu gave a talk in which he examined this option.

Note that using a std::string format with variadic templates is of course possible.

Oh, and of course you could do a hybrid where you use a variadic template, but only to forward the arguments to vsnprintf. That would allow you to use a std::string format but you don't get the true typesafety of the variadic approach.

vnp already covered this, but let's take a second look and consider an excerpt from the standard:

  The vsnprintf function returns the number of characters that would have been written
  had n been sufficiently large, not counting the terminating null character, or a negative
  value if an encoding error occurred. Thus, the null-terminated output has been
  completely written if and only if the returned value is nonnegative and less than n.

Unfortunately, you'll have to do some pretty gross handling for this since on MS systems a negative number currently means the buffer was not large enough whereas on standard compliant systems it means some of encoding error happened.

If you include the null character in the calculation, you need to use <=. If you don't include it, you need to use <. This means that your current logic of len < initial_buf_size is wrong. It should actually be len <= initial_buf_size.

Think of it this way: len represents the length of the entire, null terminated output string (since you added 1). initial_buf_size represents the size of the entire buffer, including the required null terminator. Since both numbers include the null terminator, the case where they are equal means that the entire buffer was utilized but that it does contain the full, null terminated output string.

I wouldn't bother using initialize_buf_size. Instead, I would just hard code it into the buf1 declaration and then use sizeof(buf1). I'm all for avoiding magic numbers and pulling them into constants, but it's really only used once (and you should be using sizeof anyway -- the declaration of buf1 can change).

2 spaces is rather unusual in C++. I would expect either 4 spaces or 1 tab.

I'm not a fan of the lack of white space. Like the previous item, this is just opinion, but I (and I think most people -- though obviously I'm prone to confirmation bias on this) prefer spacing around clauses and operators. In other words:

func(param1, param2, param3)

and

if (a < b) {

Your function has way too much duplicated code. Let the va_list function be handled at the top level, and then have a second function that actually does the work. This is actually exactly what every implementation of the standard library I've ever used does with.

Basically what I'm saying is that just as the standard library has printf and vprintf, you would have string_sprintf and string_vsprintf, and string_sprintf would just call string_vsprintf under the hood.

Instead of having two cases, I would be tempted to just do an empty run to vsnprintf to figure out exactly what size you'll need.

You're using a non-standard compiler extension to use a automatic memory duration array that is actually dynamically sized (buf2).

Instead, I would use a container like a std::vector to handle the buffer, or since you're arleady using C++11, you can use std::string since it's guaranteed to be contiguous.

All in all, I might do something like this:

std::string string_vsprintf(const char* format, std::va_list args) {
    va_list tmp_args; //unfortunately you cannot consume a va_list twice
    va_copy(tmp_args, args); //so we have to copy it
    const int required_len = vsnprintf(nullptr, 0, format, tmp_args) + 1;
    va_end(tmp_args);

    std::string buf(required_len, '\0');
    if (std::vsnprintf(&buf[0], buf.size(), format, args) < 0) {
        throw std::runtime_error{"string_vsprintf encoding error"};
    }
    return buf;
}

std::string string_sprintf(const char* format, ...) __attribute__ ((format (printf, 1, 2)));

std::string string_sprintf(const char* format, ...) {
    std::va_list args;
    va_start(args, format);
    std::string str{string_vsprintf(format, args)};
    va_end(args);
    return str;
}

Note that this does not handle Windows and MS's non-compliant implementation of vsnprintf.

It would also be a good idea to wrap the __attribute__ stuff in preprocessor checks since __attribute__ is only used in GCC and clang.

Oh, and the string is unnecessarily set to all null characters and only then over written with the data we actually care about. If you super care about performance, that might be an issue, but realistically, it shouldn't matter. Anyway around it would require an extra copy anyway (since you'd have to take whatever buffer you used and copy it into a std::string).