Accessing Wi-Fi network interface details from Java using netsh I am developing a Java program on Windows. I want to access Wi-Fi interface details such as SSID, IP, subnet, enabled or not, connected or not and so on. I am currently trying with using netsh and ipconfig commands.

For example, here is how I get the connected SSID. Is this a good way? Is there any better, easier method? Will this code work with all Windows versions without any problems?

String ssid;
ProcessBuilder builder = new ProcessBuilder(
        "cmd.exe", "/c", "netsh wlan show interfaces");
builder.redirectErrorStream(true);
Process p = builder.start();
BufferedReader r = new BufferedReader(new InputStreamReader(p.getInputStream()));
String line;
while (true) {
    line = r.readLine();
    if (line.contains("SSID")){
        ssid = line.split("\\s+")[3];
        System.out.println(ssid);
        return ssid;
    }
}

It looks like the solution you posted here was copy-pasted (with some cutting of irrelevant lines) from this SO answer and its original code is quite quick&dirty.

However, here are some observations about it:

This will work only on a Windows system.
Neither InputStreamReader nor BufferedReader are closed, which is a bad practice.
It's not clear how IOException is managed. I suppose that it is rethrown by the wrapping method?
while (true) is a bad pattern for loops.

A better choice would be:

String line = r.readLine();
while (line != null) {
  // logic
  line = r.readLine();
}

Using line.contains("SSID") seems to be enough to identify the line with the SSID, but it will fail if (accidentally) another line of the process output contains this substring. That should not occur normally, but who ever knows how the parameters are set on the host where it is run? I'd suggest here to split the line in two parts by ':' and check if the trimmed left side equals("SSID").


Checking two lists for equal items I'm using this code for checking if two List<T> are equal or not:

public static bool EqualsAll<T>(this IList<T> a, IList<T> b)
{
    if (a == null || b == null)
        return (a == null && b == null);

    if (a.Count != b.Count)
        return false;

    EqualityComparer<T> comparer = EqualityComparer<T>.Default;

    for (int i = 0; i < a.Count; i++)
    {
        if (!comparer.Equals(a[i], b[i]))
            return false;
    }

    return true;
}

Now assume that I'm using this code for checking two big lists. Is this code efficient?

Checking whether two lists are equal is naturally an O(n) operation. So yes, your code looks efficient.

Note that you could also replace your for loop with Enumerable.SequenceEqual

public static bool EqualsAll<T>(this IList<T> a, IList<T> b)
{
    if (a == null || b == null)
        return (a == null && b == null);

    if (a.Count != b.Count)
        return false;

    return a.SequenceEqual(b);
}

If you take a look at its implementation, you'll notice that it's very similar to yours. It might be just a tiny bit less efficient - so if performance is an issue here, measure both approaches, and then weigh readability/maintainability vs performance gains.

On a different note, I would rename the method to SequenceEqual and consider it a "specialization" of Enumerable.SequenceEqual for lists.

I would also consider throwing an exception if either argument is null.
This would be consistent with the BCL's own extension methods (Select, Where, ToList, SequenceEqual, Any, All, etc).


Palindrome numbers - Project Euler Problem 4 Project Euler Problem #4: Largest palindrome product:

  A palindromic number reads the same both ways. The largest palindrome
  made from the product of two 2-digit numbers is 9009 = 91 × 99.

  Find the largest palindrome made from the product of two 3-digit
  numbers.

I got the correct answer for the problem, but I want to know if my coding style is good or bad for this solution.
I need to know how I can improve my coding, if it is bad and write this in a more general way.

#include<stdio.h>
#define MAX 999
#define START 100

int main()
{
   int i,j,current,n,prev = 0;
for(i = START;i<=MAX;i++)
{
    for(j=START;j<=MAX;j++)
    {
        current = j * i;
        if(current > prev)  /*check the current value so that if it is less need not go further*/
        {
            n = palindrome(current);
            if (n == 1)
            {
                printf("The palindrome number is : %d\n",current);
                prev = current;  // previous value is updated if this the best possible value.
            }
        }
    }
}
}

int palindrome(int num)
{
int a[6],temp;
temp = num;
/*We need a array to store each element*/
a[5] = temp % 10;
a[4] = (temp/10) %10;
a[3] = (temp/100) %10;
a[2] = (temp/1000) %10;
a[1] = (temp/10000) %10;
if(temp/100000 == 0)
{
    a[0] = 0;
    if(a[1] == a[5] && a[2] == a[4])
        return 1;
}
else
{
    a[0] = (temp/100000) %10;
    if(a[0] == a[5] && a[1] == a[4] && a[2] == a[3])
        return 1;
    else
        return 0;
}
}

First of all, you are only interested in the biggest number, so why do you start from the bottom??
Correcting that allows you to dispense with an explicit lower boundary, a dynamic boundary can be computed depending on current-best result.
Why don't you read the upper boundary from the command-line?
Proper indentation is a very valuable help in quickly and accurately reading source-code, as well as ferreting out errors.
In your case, the one level of indentation missing through much of your code might be an artifact of sloppy posting here. Whether or not it is, don't do that.
You are depending on implicit definition of palindrome. Don't do that, reverse their positions.
An int only has a guaranteed range of -32767 to 32767. Use a long (or unsigned long), that has the proper range without depending on implementation-dependent behaviour.
Why do you use an array and all that paraphernalia to check for being a palindrome, simply reversing is easy:

unsigned long reverse(unsigned long in) {
    unsigned long r = 0;
    for(; in; in /= 10)
        r = r * 10 + in % 10;
    return r;
}

Final program (on coliru):

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

static int is_palindrome(const unsigned long in) {
    unsigned long r = 0;
    for(unsigned long x = in; x; x /= 10)
        r = r * 10 + x % 10;
    return r == in;
}

int main(int argc, char* argv[]) {
    unsigned long max = argc > 1 ? strtoul(argv[1], 0, 10) : 0;
    if(!max || max > 9999)
        max = 999;
    unsigned long _a = 0, _b = 0, _m = 0; // best yet
    for(unsigned long a = max, a_min = 1; a >= a_min; --a)
        for(unsigned long b = max, b_min = _m / a + 1; b >= b_min; --b)
            if(is_palindrome(a*b)) {
                _a = a, _b = b, _m = a*b, a_min = _m / max + 1;
                break;
            }
    printf("Biggest palindrome-number which is product of two natural numbers"
        " no bigger than %lu: %lu = %lu * %lu\n", max, _m, _a, _b);
    return 0;
}


Print consonant and vowels My assignment is to to create two threads running functions (called vow and cons). The threads take turns printing the respective words of the phrase supplied from a text file. The main thread shouldn't post anything (only cons and vow functions should print). I can't use synchronization primitives such as mutexes for thread coordination. But I can use yield(). I think I might have my answer below but I also might be way off and I didn't use yield() at all.

Could someone please take a look and tell me if this meets the requirements I listed above.

phrase.txt:

  In computer architecture, multithreading is the ability of a central processing unit (CPU) or a single core in a multi-core processor to execute multiple processes or threads concurrently, appropriately supported by the operating system.

#include <iostream>
#include <fstream>
#include <thread>

void cons(std::string currWord){
    std::cout << "cons: " << currWord << "\n";
}

void vow(std::string currWord){
    std::cout << "vow: " << currWord << "\n";
}

bool checkVowel(std::string word) {
    char vowels[10] = {'A', 'E', 'I', 'O', 'U', 'a', 'e', 'i', 'o', 'u'};
    for(int i = 0; i < 10; i++) {
        if(word[0] == vowels[i]) {
            return true;
        }
    }
    return false;
}

int main() {
    std::string currWord;
    std::ifstream input;
    input.open("phrase.txt");

    if(input.is_open()) {

    currWord = std::string();

    while(!input.eof()) {
            input >> currWord;
        if(checkVowel(currWord)) {
            std::thread vowThread(vow, currWord);
            vowThread.join();
        }
        else {
            std::thread consThread(cons, currWord);
            consThread.join();
        }
    }

    input.close();
    }

    else {
        std::cout << "Error: File not opened";
    }

    return 0;
}

bool checkVowel(std::string);
void vow(std::string);
void cons(std::string);

Your code is a little confusing.  You're using names like currWord and passing a word to checkVowel, which is then checking for 'aeiou' etc.  I think of vowels as letters, so on the face of it you appear to be checking whole words against individual vowel letters, which probably shouldn't match.  That said, maybe the file is ordered such that it's actually only reading letters at a time.  Either way, your code is unclear.

As has been said in the comments, whilst you've technically used threads to perform the printing, it's unlikely that this is the strategy that was expected to solve the problem.  There are many approaches that you could take to solving the problem, one of which might be something like this:

start ConThread
start VowelThread

Open File
ForEach word in file
    ForEach letter in word
        if Vowel
            StoreLetter
            Set VowelFlag
        else
            StoreLetter
            Set ConFlag
        while ReadNextLetterFlag not set give up control
        clear ReadNextLetterFlag

Shutdown...

The vowel/Con threads then consist of something like:

While not shutdown
    While ConFlag not set give up control
    clear ConFlag
    Retrieve letter
    Output to console
    Set ReadNextLetterFlag

Giving up control of the running thread could be achieve using yield (although this isn't particularly efficient) and you may want to pair it with sleep.

For flags, you probably want to look at either atomic and/or volatile variables.


consecutive_find function for returning a consecutive range of elements consecutive_find() is a function that returns the beginning of a range that contains strictly N consecutive elements. It was part of a SO question that I answered.

Currently this function runs in \$O(n)\$ time. I wanted to know if there are any pitfalls with this function and if it can be improved at all. For example, are there any standard algorithms that can be used here that have not been? Here is a demo.

Explanation of code:

It sets two variables to the start of the range: marker and lead. lead is always going to be 1 position greater than marker (in order to see if there are more consecutive elements).

Then we loop through a count how many elements equal *lead using a variable count.

It checks count to see if it equals n (the element count we're looking for) and if either lead is the iterator to the end, or if lead points to an element that is different than marker, then we know that there are no more matching elements that we haven't checked, and we return marker, which is n places behind lead.

If that condition fails, then we know *lead == *marker. This goes against the strict condition that we want because there are greater than N consecutive elements. So we set marker to the next position that doesn't equal *lead to prevent unneeded extra loops. marker is set to lead and count is reset to 1.

template <class Iter>
Iter consecutive_find(Iter first, Iter last, std::size_t n)
{
    Iter marker(first), lead(first);
    std::size_t count(1);

    using value_type = typename std::iterator_traits<Iter>::value_type;

    while (lead != last)
    {
        lead = std::next(marker);
        while ((lead != last) && (*marker == *lead)) { ++count; ++lead; }
        if (count == n)
        {
            if ((lead == last) || !(*lead == *marker))
                return marker;
        }
        marker = std::find_if_not(lead, last,
                                  [&lead] (value_type const& x) { return x == *lead; });
        count = 1;
    }
    return last;
}

I can change the nested while() loop to lead = std::find_if_not(lead, last, \[&lead\] (value_type const& x) { return x == *lead; }); but then that violates the DRY rule.

Your function does not work correctly. The bug is in the lines:

    marker = std::find_if_not(lead, last,
                              [&lead] (value_type const& x) { return x == *lead; });

Say you call the function with the following code:

std::vector<int> v{1, 2, 2, 2, 3};
auto it = consecutive_find(v.begin(), v.end(), 3);

In the function, you start with:

marker -> 1
last   -> 1

Then, in the first statement of the outer while block, you execute:

    lead = std::next(marker);

Now,

last   -> 2

You break out of the first inner while block with count set to 1, Then you execute

    marker = std::find_if_not(lead, last,
                              [&lead] (value_type const& x) { return x == *lead; });

When this line is finished executing,

marker -> 3

and you completely missed the correct return value.

You can change those lines to simply:

    marker = lead;

and the function works fine.

Here's a sample program and its output:

#include <iostream>
#include <vector>
#include <algorithm>

template <class Iter>
Iter consecutive_find(Iter first, Iter last, std::size_t n)
{
    Iter marker(first), lead(first);
    std::size_t count(1);

    while (lead != last)
    {
        lead = std::next(marker);
        while ((lead != last) && (*marker == *lead)) { ++count; ++lead; }
        if (count == n)
        {
            if ((lead == last) || !(*lead == *marker))
                return marker;
        }
        marker = lead;
        count = 1;
    }
    return marker;
}

void test1()
{
   std::vector<int> v{1, 2, 2, 2, 3};
   auto it = consecutive_find(v.begin(), v.end(), 3);
   for (int i = 0; i < 3 && it != v.end(); ++i, ++it )
   {
      std::cout << *it << " ";
   }
   std::cout << std::endl;
}

int main()
{
   test1();
}

Output:

2 2 2


Universal Spreadsheet reader/writer I have a project that reads spreadsheets as a rectangular list of lists (matrix) transforms the matrix and then writes it to another spreadsheet.  I want to be able to read from and write to multiple formats.  Consequentially, the program should double as spreadsheet conversion tool.

Here is an example of just copying one sheet to another:

def ConvertSheet(in_sheet, out_sheet)
    sheet_io = SheetIO()
    data_matrix = sheet_io.read_sheet(in_sheet)
    # different functions would operate on data_matrix before writing.
    sheet_io.write_sheet(out_sheet, modified_data)

Depending on what type of object in_sheet and out_sheet are determine how SheetIO writes it but as far as the client cares, that's SheetIO's problem.

The design I have implemented is an odd factory pattern.  It's primarily why I am asking for advice.

Abstract base class

This is pretty basic and would essentially be the interface in a proper factory pattern:

class ISheetIO(object):
    """ Abstract base class...
    """
    def read_sheet(self):
        raise NotImplementedError("read_sheet not implemented")

    def write_sheet(self):
        raise NotImplementedError("write_sheet not implemented")

Concrete Class

The only concrete base class I have implemented so far.  Note that it reads and writes sub-vectors as rows.  I am specifying that all Concrete classes operate that way.

Also note that I am allowing optional arguments, specifying the range to write them to.

class ExcelSheet(ISheetIO):

    def read_sheet(self, sheet, col_start=0, row_start=0, col_cnt=-1, row_cnt=-1):

        # TODO: Raise an error instead. Research which error to raise
        assert not sheet.ragged_rows

        if row_cnt < 0:
            row_cnt += sheet.nrows + 1
        if col_cnt < 0:
            col_cnt += sheet.ncols + 1

        return [sheet.row_values(r, col_start, row_start)
                for r in xrange(row_start, row_cnt + 1)]

    def write_sheet(self, data, sheet, col_start=0, row_start=0):
        for r, row, in enumerate(data):
            for c, el in enumerate(row):
                sheet.write(row_start + r, col_start + c, el)

# class CSVSheet(ISheetIO):
# class OpenXMLSheet(ISheetIO):

Wrapper Class

This is where things get hairy.  My understanding of the factory pattern is that this should be the factory at it would return a concrete class.  Instead the client instructs SheetIO to read or write and SheetIO gets and uses the concrete class itself through its own factory. The reason for composing the concrete class instead of returning it is, I am permitting the client to request the data with sub-lists as columns.  SheetIO encapsulates that state with read_by_row and remembers to transpose the data again when writing.

class SheetIO(ISheetIO):

    def __init__(self, read_by_row=True):
        self.by_row = read_by_row

    def _factory(self, sheet):

        sheet_type = type(sheet)

        if sheet_type is xlrd.sheet or sheet_type is xlwt.WorkSheet:
            return ExcelSheet()
        # elif CSV
        #    return CSVSheet()
        # elif OpenXML
        #    return OpenXMLSheet()
        else:
            raise TypeError("Unknown Sheet type or not a sheet")

    def read_sheet(self, sheet, **kwargs):
        data = self._factory(sheet).read_sheet(sheet, **kwargs)
        return self.orient_data(data)

    def write_sheet(self, data, sheet, **kwargs):
        oriented_data = self._orient_data(data)
        self._factory(sheet).write_sheet(sheet, oriented_data, **kwargs)

    def _orient_data(self, data):
        if self.by_row:
            return data
        else:
            return self._transpose(data)

    def _transpose(self, matrix):
        return zip(*matrix)

Indeed, this factory pattern is not right. A factory should have the single responsibility of creating instances. You violated that the moment you made it a subclass of ISheetIO, which is an entirely different responsibility.

The first step to make this better is to change the factory to be more factory-like:

Rename it to SheetIOFactory
Extend object (stop extending ISheetIO)
Rename the _factory method to create_sheet method: this will become the official way to create sheets

The methods that remain, read_sheet, write_sheet, _orient_data, _transpose belong in another class. Let's call it SheetIOHelper.
This class should not extend ISheetIO.

Why? You would rightfully ask:
it has read_sheet and write_sheet methods.
It even uses the same kind of parameters (uhm, more or less).
But a crucial (and somewhat subtle) difference is that the methods in the helper class and the methods in ExcelSheet have completely different contracts:

The methods in ExcelSheet are designed to work with Excel sheets
The methods in SheetIOHelper are designed to work with whatever kind of sheet you throw at them

That is, the methods in SheetIOHelper are expected to detect appropriate source / target sheet type, the methods in ExcelSheet are just not designed that way, they work with only a specific type of sheet.
In strongly typed languages,
these two interfaces wouldn't be compatible, the code wouldn't compile.

In other words,
the helper and the concrete classes don't follow the same interface.
You could create a dedicated parent class for SheetIOHelper.
That might sound like overkill,
since you only plan to have just one implementation,
it might be worth it,
to make the distinction from ISheetIO loud and clear.
Or you could be lazy and just make it extend object.

And when you use the helper,
you should add "helper" in the variable name to avoid confusion:

def ConvertSheet(in_sheet, out_sheet)
    sheet_io_helper = SheetIOHelper()
    data_matrix = sheet_io_helper.read_sheet(in_sheet)
    # different functions would operate on data_matrix before writing.
    sheet_io_helper.write_sheet(out_sheet, modified_data)

Actually,
since you probably won't ever work with the concrete classes directly,
it would be better to rename the interface of the concrete classes instead.
So in the end, have this class structure:

SheetIOHelper: interface of the helper, that can read / write any types of sheet

SheetIO: implementation of the helper

ConcreteSheetIO: interface of concrete implementations that can only read / write sheets of type T

ExcelSheetIO: concrete implementation to read / write Excel sheets
CsvSheetIO: concrete implementation to read / write CSV sheets

SheetIOFactory: implementation of the factory, creates ConcreteSheetIO instances and does nothing else

In the end, the names are quite different from what I used in the beginning.
I hope you see the evolution that lead us here, and hopefully it's not too confusing.
If this is not clear,
let me know, I can restructure your original if it would help.

Code review

In this code:

def read_sheet(self, sheet, col_start=0, row_start=0, col_cnt=-1, row_cnt=-1):
    # ...
    if row_cnt < 0:
        row_cnt += sheet.nrows + 1
    if col_cnt < 0:
        col_cnt += sheet.ncols + 1

When I see row_cnt += sheet.nrows + 1 I have 2 thoughts:

What is the value of row_cnt before the += ?
Why the +1 in sheet.nrows + 1 ?

These are very easy to answer, but I still have to think of them.
It would be better if I didn't have to think.
It seems to me that this simpler variant would be just as good,
and it won't make me think:

if row_cnt < 0:
    row_cnt = sheet.nrows
if col_cnt < 0:
    col_cnt = sheet.ncols

In this code:

if sheet_type is xlrd.sheet or sheet_type is xlwt.WorkSheet:
    return ExcelSheet()
# elif CSV
#    return CSVSheet()
# elif OpenXML
#    return OpenXMLSheet()
else:
    raise TypeError("Unknown Sheet type or not a sheet")

It's a minor thing,
but since each elif branch returns,
I prefer this writing style:

if sheet_type is xlrd.sheet or sheet_type is xlwt.WorkSheet:
    return ExcelSheet()
# if CSV
#    return CSVSheet()
# if OpenXML
#    return OpenXMLSheet()
raise TypeError("Unknown Sheet type or not a sheet")


Find all distinct triples less than N that sum to S Exercise 2.41.  Write a procedure to
  find all ordered triples of distinct
  positive integers i, j, and k less
  than or equal to a given integer n
  that sum to a given integer s.

(define (enumerate-integers i j)
  (if (= i j)
      (list j)
      (cons i (enumerate-integers (+ i 1) j))))

(define (filter f seq)
  (if (null? seq)
      null
      (if (f (car seq))
          (cons (car seq) (filter f (cdr seq)))
          (filter f (cdr seq)))))

(define (remove x seq)
  (filter (if (pair? x)
              (lambda (y) (not (member y x)))
              (lambda (y) (not (= x y)))) seq))

(define (unique-triples-less-than n)
  (let ((the-number-list (enumerate-integers 1 (- n 1))))
    (flatmap (lambda (i)
               (flatmap (lambda (j)
                          (map (lambda (k) (list i j k))
                               (remove (list i j) the-number-list)))
                        (remove i the-number-list)))
             (enumerate-integers 1 (- n 1)))))

(define (flatmap f seq)
  (accumulate append null (map f seq)))

(define (accumulate op initial seq)
  (if (null? seq)
      initial
      (op (car seq)
          (accumulate op initial (cdr seq)))))

(define (s-sum-triples-below-n n s)
  (filter (lambda (y) (= (accumulate + 0 y) s))
          (unique-triples-less-than n)))

Can this code be improved in any way?

Your unique-triples-less-than-n is unnecessarily complex because you can state it recursively. S. Kucherenko's answer tries to achieve this but is also unnecessarily complex.
A simple recursive formulation is here, but this assumes that enumerate-integers returns an empty list if called with values from > to.

 (define (unique-tuples m from to)
    (if (= n 0) '(())) ; one empty tuple
        (flatmap (lambda (n)
                   (map (lambda (t) (cons n t))
                        (unique-tuples (- m 1) (+ n 1) to)))
                 (enumerate-integers from to))))

This unfolds as recursive calls e.g. like this:

 (u-t 3 1 4) = (flatmap ... '(1 2 3 4))
             = (flatten '(,(map (lambda (t) (cons 1 t))
                                (u-t 2 2 4))
                          ,(map (lambda (t) (cons 2 t)) ;; X
                                (u-t 2 3 4))
                          ,(map (lambda (t) (cons 2 t))
                                (u-t 2 4 4))
                          ,(map (lambda (t) (cons 2 t))
                                (u-t 2 5 4))))

and e.g. if you look at line marked with X, this takes recursively (unique-tuples 2 3 4), i.e. 2-tuples whose first integer is between 3 and 4, so the list is '((3 4)), and then for every element of that list, we add 2 at the beginning, so map returns '((2 3 4)).

Then just

(define (unique-triples n) (unique-tuples 3 1 n))

The results need to be filtered afterwards for the sum.


jQuery duplicated selector $("#colors_added").html(parseFloat($("#colors_added").html()) + 1);

This JS line makes my IDE go Duplicated jQuery selector with the following explanation:

  Checks that jQuery selectors are used in an efficient way. It suggests to split descendant selectors which are prefaced with ID selector and warns about duplicated selectors which could be cached.

I've made a screenshot of it:

Is my IDE on to something here, can I write this in a better way?

You should cache your selector in a variable:

var $addedColors = $("#colors_added");
$addedColors.html(parseFloat($addedColors.html()) + 1);

Reason: placing selectors in variables is recommended, since overusing selectors can result in poor performance.

Everytime you would call a function on $("#colors_added"), the whole DOM has to be parsed by the browser. This is not the case when you store it in a variable.

Also, it's a general principle to not repeat yourself in code, also called the DRY principle (as mentioned by Peter Rader in the comments).


Does my classes contain right destructors Every Account has its owner who is Person The bank has list of owners. I am not sure if my destructors are right and delete bank will delete all elements of list<Account*>* v nad the list itself. Please help.

#include <iostream>
#include <vector>
#include <list>
using namespace std;
class Person{
public:
    char* name;
    int age;

    Person(char* name, int age){
        this->name = name;
        this->age = age;
    }
    ~Person(){
    }

    void show(){
        cout<<name<<" "<<age<<" yo";
    }
};

class Account{
public:
    Person& owner;
    double money;
    Account(Person* owner, double money):
        owner(*owner) , // this->owner = *owner;
        money(money)  { //Uninitialized reference member
    }

     void show(){
        cout<<"\n-------------\n";
        owner.show();
        cout<<endl;
        cout<<money<<"USD\n-------------";
    }
      ~Account(){
    }
};

class Bank{
public:
    list<Account*>* v;
    Bank(){
        v = new list<Account*>();
    }
    Account* openNewAccount(Person* client, double money){
        Account* r = new Account(client, money);
        v->push_back(r);
        return r;
    }
    void zmienWlasciciela(Person& o, Account* r){

        r->owner = o;
    }
    void usunRachunek(Account* r){
        cout<<"\n\n\nUSUNALEM"<<endl;
       v->remove(r);
       delete r;
    }
    void show(){
    for (list<Account*>::iterator it = v->begin(); it != v->end(); ++it){
        (*it)->show();
    }
    }
    ~Bank(){
        delete v;
    }
};

int main(){
    Bank* bank = new Bank();
    Person* thomas = new Person("thomas", 34);
    Account* r1 =  bank->openNewAccount(thomas, 64363.32);
    Account* r2 =  bank->openNewAccount(thomas, 41251.54);
    Account* r3 =  bank->openNewAccount(thomas, 3232.32);
    bank->show();

    Person* margaret = new Person("Margaret", 23);
    bank->zmienWlasciciela(*margaret, r2);
    cout<<"I have changed owner of account r2"<<endl;
    bank->show();
    cout<<"I have deleted account r3"<<endl;
    bank->usunRachunek(r3);
    bank->show();

    delete bank;
    return 0;
}

General Comment on Style

This is probably not what you want.

Bank* bank = new Bank();
Person* thomas = new Person("thomas", 34);

You actually want:

Bank   bank;
Person thomas("thomas", 34);

This creates automatic variables. That are properly destroyed automatically at the end of scope.

It is very rare to use RAW pointers in C++ Bank* or Person*. When you dynamically allocate objects using new you want to make sure that the returned pointer is put into an object that controls their lifespan and garbage collects the values when you are finished using them.

std::shared_ptr<Bank> bank = new Bank;  // This is the closest to a Java object
                                        // that C++ has. It allows multiple users
                                        // and it is correctly destroyed when nobody
                                        // has a reference to it anymore.

Looking at the code:

Don't do this:

using namespace std;

I know all the shitty C++ books you read have it in. Its fine for 10 line long programs you type out of a book. But once you get past those programs it can cause problems with namespace pollution. Its thus a very bad habit and one you should break as soon as possible. See: https://stackoverflow.com/q/1452721/14065

Person

Pointers are bad.

    char* name;

Unless you do a lot of validation and know the ownership of the thing you are pointing at and it is not going to be cleaned up behind you then you should be very weary of using pointers. I have checked your code and you are fine in this simple program but this is not a good design outside of a small toy project. C++ also has its own string type std::string so you don't need to know or use C-String. So I would change this to

    std::string  name;

In constructors prefer to use the initializer list to initialize the members.

    Person(char* name, int age){
        this->name = name;
        this->age = age;
    }

Second point about pointers here. Its not normal to pass pointers to around in C++ (this is very common in C but modern C++ you should basically never do it (with a few exceptions that you will work out when you have more experience)). Pass objects by reference if you want to retain ownership; otherwise pass a smart_pointer to transfer ownership of the object.

In this case you want to pass a std::string. As you don't want to change the original but make your own copy pass a const reference (if you are in C++11 you may want to also add a move constructor (but this is advanced so don't worry about it)).

So it should look more like:

    Person(std::string const& name, int age)
        : name(name)
        , age(age)
    {}

If a destructor does not do anything then don't declare it.

    ~Person(){
    }

Its fine to have a public show() method. But any method that does not change the state of the object should be declared as const. This allows you to call the object from const objects (useful technique later). And if you add a parameter so it can be any stream it makes it more versatile. But you should also declare a stream operator for your Person type. Thus you can stream your person just like any other type.

    void show(std::ostream& out = std::cout) const {
                                        //   ^^^^^    Does not change the state
                                        //            so make the method const.
          //  ^^^^^^^^^^^^^^^^^  Pass stream to print on.
          //                     You may want to print in other places.
          //                     rather than std::cout (like a file).

        out<<name<<" "<<age<<" yo";  // A few spaces to make it readable
                                      // would be nice.
    }

// Pass person by const reference so the compiler
// knows you are not going to modify the object.
// This means you can only call const methods (so `show()` must be const.
std::ostream& operator<<(std::stream& stream, Person const& person)
{
    person.show(stream);
    return stream;
}

int main()
{ std::cout << Perons("Loki", 962) << "\n";
}

Account

This is fine.

Person& owner;

It means you have a reference to a person that is stored somewhere else. But you need to be careful of lifetimes. Make sure the Person object is still alive while you are alive.

Passing by pointer again.

Account(Person* owner, double money):

This is dangerous. Especially since you de-reference without checking. Since you are storing a reference. Pass the value in by reference.

Account(Person& owner, double money)
    : owner(owner)
    , money(money)
{}

Make the show() method const and pass a stream.

 void show(std::ostream& out = std::cout) const {
                                      //  ^^^^^
    out<<"\n-------------\n" << owner << std::endl << money<<"USD\n-------------";
    return out;
}

Add a stream operator

std::ostream& operator<<(std::stream& stream, Account const& account)
{
    account.show(stream);
}

Destructor does nothing so remove it.

  ~Account(){
}

Bank

Ok. Obviously a java (or java inspired language user).

list<Account*>* v;

You don't want to use pointers here. By making this a pointer you need to do all sorts of memory management (and make sure you obey the rule of three (look it up)). But you don't need to do that just make it a normal object and all those problems go away.

list   v;

When the bank object is created. It automatically creates a list now. When the bank is destroyed it automatically cleans up its members (including the list). When the back is copied it automatically copies the accounts to the new Bank.

Second; don't make the Account's inside the list pointers. Again you will need to manage the each object and control its lifespan. Much easier to put Account objects into the list.

list   v;

There is also a principle called Separation of Concerns. Basically this says that an type should either be Business Logic or Resource Management you should not do both in the same type. Since Bank is obviously business logic you should not be doing resource management in the Bank. If you were using pointers then you would delegate the memory management of those pointers to anther type that would correctly do the clean up. C++ already has a whole bunch of smart pointers explicitly for managing pointers.

BUT in this situation I would defiantly sidestep the memory management by using automatic objects and make the changes I described above as ALL your memory management issues disappear.

Remember use initializer list.

Bank(){
    v = new list<Account*>();
}

But if you change the type as (described above) you don't need to do anything. The object automatically creates and initializes all members.

Again passing pointers is bad:

Account* openNewAccount(Person* client, double money){
    Account* r = new Account(client, money);
    v->push_back(r);
    return r;
}

If you return a pointer Account* you don't specify any ownership semantics. If you don't specify ownership semantics then you are very liable to leak as nobody knows who is supposed to clean up. In this situation the Account obviously belongs to the Bank object (The Bank will do the memory management). So here you should return a reference. This is an indication you can use the account. But the account will be managed by the Bank.

Account& openNewAccount(Person const& client, double money)
{
    Account  newAccount(client, money);  // Create a new account.
    v->push_back(newAccount);            // Not when you push objects into containers
                                         // they make a copy of the object.
                                         // So you have a copy of `newAccount`

    return v.last();   // return a reference to the last member of `v`
                       // that you just pushed there via `push_back`
                       // The user can now operate on the referenc
                       // Also the reference will never be NULL
                       // so the user needs to check for that.
}

Pass by reference. You don't need to check for NULL

void zmienWlasciciela(Person& o, Account* r)

// Becomes.
void zmienWlasciciela(Person& o, Account& r){
    r.owner = o;
}

Pass by reference (that's how it was given to you).

void usunRachunek(Account* r)

// Becomes
void usunRachunek(Account* r){
    cout<<"\n\n\nUSUNALEM"<<endl;
   v.remove(r);
}

Make show() const and take a stream

void show(std::ostream& out = std::cout ) const {
for (list<Account*>::iterator it = v.begin(); it != v.end(); ++it){
    (*it)->show(out);
}

You could use a simple algorithm to copy the Account to the stream.

void show(std::ostream& out = std::cout ) const
{
    // for this to work Account must have operator<< defined.
    std::copy(v.begin(), v.end(), std::ostream_iterator(out));
}

}

This is correct BUT NOT ENOUGH for your original definition where v is a pointer. If you use objects you don't need to do anything.

~Bank(){
    delete v;
}

If you were using pointers then you also need to define a copy constructor and assignment operator to make sure you are not violating the rule of three. If you are using objects the compiler generated defaults work perfectly fine.


Dispatching Brainfuck commands using if-else or dictionary Here's the first, straightforward version of BF interpreter:

def run(step_limit, memo, m_i, code):
    """ Runs valid BF code. """

    if len(code) == 0:
        return

    i = 0
    for step in range(step_limit):
        cmd = code[i]

        # Switch - case
        if cmd == '+':
            memo[m_i] += 1
        elif cmd == '-':
            memo[m_i] -= 1
        elif cmd == '>':
            m_i += 1
        elif cmd == '<':
            m_i -= 1
        elif cmd == '.':
            print (memo[m_i])
        elif cmd == ',':
            memo[m_i] = int ( input ( "Input: " ) )
        elif cmd == '[':
            if memo[m_i] == 0:
                i = code.find(']', i, len(code) )
        else: # if cmd == ']'
            if memo[m_i] != 0:
                i = code.find('[', 0, i )

        i += 1
        if i == len(code):
            break

And the second one:

""" Second version of the same method. """
# I tried to use namedtuples but I failed. Using simple, usual tuples instead.

def plus (args):
    return args[1]+1, args[2]+1, args[3]

def minus (args):
    return args[1]+1, args[2]-1, args[3]

def rigth (args):
    return args[1]+1, args[2], args[3]+1

def left (args):
    return args[1]+1, args[2], args[3]-1

def output (args):
    print (args[2])
    return args[1]+1, args[2], args[3]

def u_input (args):
    return args[1]+1, int (input ("Input: ") ), args[3]

def loop_begin (args):
    if args[2] == 0:
        return args[0].find(']', args[1], len(args[0]))+1,args[2],args[3]
    else:
        return args[1]+1, args[2], args[3]

def loop_end (args):
    if args[2] != 0:
        return args[0].find('[', 0, args[1] )+1, args[2], args[3]
    else:
        return args[1]+1, args[2], args[3]

def nothing (args): # So that memo[m_i] wouldn't go out of index
    return args[1], args[2], args[3]

commands = {
    '+' : plus,
    '-' : minus,
    '>' : rigth,
    '<' : left,
    '.' : output,
    ',' : u_input,
    '[' : loop_begin,
    ']' : loop_end,
    ' ' : nothing }

def run(step_limit, memo, m_i, code):
    """ Runs valid BF code. """

    if len(code) == 0:
        return

    code += ' ' # So that memo[m_i] wouldn't go out of index
    i = 0
    for step in range(step_limit):
        args = (code, i, memo[m_i], m_i)
        i, memo[m_i], m_i = commands[ code[i] ] ( args )
        if args[1] == len(args[0]) - 1 or code[i] == ' ':
            break

# Now we have to add space after program so that memo[m_i] wouldn't go out of index
run(1000000000, [478234, 5161845], 1, "[-<+>]<. ") # TEST, adds two numbers

Yes, I know I should have used dictionaries to increase readability because now the indexes are horrible, but this is just a quick concept.

Question: would you ever prefer second structure over the first one, and under what conditions?
Maybe you know a solution better than both of these versions?

Some of my thoughts:

Second version is 2x slower. Benchmarked with run(1000000000, [478234, 5161845], 1, "[-<+>]<. "), finishes in 73.5s while first one does the job in 36s.
In my opinion, second version is more decompositioned, thus easier to maintain and expand in very big projects, but not worthy the effort in the small ones.

A few coding points:

I think you could simplify the arguments a lot by using kwargs, rather than passing a tuple of arguments.
Having your dict in the global namespace, rather than the function's local namespace, will slow it down considerably.
0 is False, so if args[2] != 0: can just be if args[2].
For unused variables, like step, I consider it better to mark it as such by using _ as the variable name.
Half of the functions are simple one-liners that can use lambdas.
Although half of the functions can be lambdas, some of them would work better by doing in-place operations on memo.
If you want to search until the end, you don't need to specify an end to find.
You add 1 to i in every case, so that can be moved out of the functions.

As for your questions:

There are three major situations where I would use the dict structure.

The functions can either be lambdas or external functions that are short and easy-to-read as dict values.  Usually this is better with only one or a few consistent arguments.
The functions are so long that putting them in an if chain makes it hard to keep track of the flow.  Again, usually this is better with only one or a few consistent arguments.
There are a huge number of possible inputs, which makes for an overly long if test chain, but where there are again only one or a few consistent arguments.

The problem with your case is that this is in-between, they are simple enough that you can follow the flow easily in an if test, but they are not all so simple that they can be put into lambdas without sacrificing clarity (it is possible to put all but one in lambdas, but half would be really hard to read).  Further, there are not so many situations that it becomes hard to read in an if chain (although it is getting close), and there are enough arguments (or enough inconsistency in the arguments) that it is getting to the point where I would not use the dict approach even if all the other criteria were satisfied (although again it is kind of borderline).

Ultimately, what it comes down to is whether the dict approach helps or hurts readability.  In your case, I think it hurts it.  If the criteria I described above hold, then it will probably help it (although you really need to look on a case-by-case basis, since there will be corner cases either way).


Java method - levels of abstraction In his Clean Code book, Robert C. Martin states that functions should do only one thing and one should not mix different levels of abstraction. So I started wondering whether the code below meets these requirements (I assume the answer is negative). Which one of the following code snippets is better?

Snippet 1

public void run() {
    isRunning = true;

    try {
        serverSocket = new ServerSocket(port);
    }
    catch (IOException | IllegalArgumentException e) {
        System.out.println("Could not bind socket to given port number.");
        System.out.println(e);
        System.exit(1);
    }

    while(isRunning) {
        Socket clientSocket = null;

        try {
            clientSocket = serverSocket.accept();
        }
        catch (IOException e) {
            logEvent("Could not accept incoming connection.");
        }

        Client client = new Client(clientSocket, this);
        connectedClients.put(client.getID(), client);
        Thread clientThread = new Thread(client);
        clientThread.start();
        logEvent("Accepted new connection from " + clientSocket.getRemoteSocketAddress().toString());
        client.send("@SERVER:HELLO");
    }
}

Snippet 2

public void run() {
    isRunning = true;

    createServerSocket();

    while(isRunning) {
        Socket clientSocket = null;
        clientSocket = acceptConnection();
        addClient(clientSocket);
    }
}

private void createServerSocket() {
    try {
        serverSocket = new ServerSocket(port);
    }
    catch (IOException | IllegalArgumentException e) {
        System.out.println("Could not bind socket to given port number.");
        System.out.println(e);
        System.exit(1);
    }
}

private Socket acceptConnection() {
    try {
        Socket clientSocket = serverSocket.accept();
    }
    catch (IOException e) {
        logEvent("Could not accept incoming connection.");
    }
    return clientSocket;
}

private void addClient(Socket clientSocket) {
    Client client = new Client(clientSocket, this);
    connectedClients.put(client.getID(), client);
    Thread clientThread = new Thread(client);
    clientThread.start();
    logEvent("Accepted new connection from " + clientSocket.getRemoteSocketAddress().toString());
    client.send("@SERVER:HELLO");
}

However, splitting such simple code into multiple fragments seems like an overkill (although it's more readable - is it?). Besides, createServerSockets has side effects (Sys.exit) and addClient does too many things at once. Any advice?

The second one is much easier to read, it expresses the developers intent. At first sight I (as a maintainer or another developer in the same team, for example) just want a quick overview about the code and don't care about the details. (How the code creates a server socket, for example.) The second one exactly gives that.

The System.exit side effect could be eliminated: throw an exception with a proper message (and don't forget to set the cause). Then handle the exception in the run method. I often see wrappings like the following:

public void run() {
    try {
        doRun();
    } catch (SomeException e) {
        // exception handling here
    }
}

This often makes the doRun() simpler and easier to read.

Some other notes:

Socket clientSocket = null;
clientSocket = acceptConnection();

Could be written as

Socket clientSocket = acceptConnection();

I'd modify the createServerSocket() method to return the server socket and the acceptConnection() method to have a ServerSocket parameter. It would remove the temporal coupling between the two methods. It would be impossible to call them in the wrong order.
I've not written any threading code recently but I don't think that creating a separate thread for every client scale well. You might want to use an executor/thread pool there.


Implementation of has_member_function From https://stackoverflow.com/a/257382/6949852
I learned how to check whether a member function exists, then I re-write it to standardize the codes:

Now the codes become:

#include <type_traits>
#include <iostream>

struct A  //has hello
{
  void hello() {}
};

struct B {}; //no hello

template<typename T>
struct has_hello
{
private:
  using yes = char;
  using no = long;
  template<typename C>
  static yes has(decltype(&C::hello));
  template<typename C>
  static no has(...);
public:
  enum name : bool {value = std::is_same<decltype(has<T>(nullptr)), yes>::value};
};

int main()
{
  std::cout << static_cast<std::underlying_type<has_hello<A>::name>::type>(has_hello<A>::value) << std::endl;
  std::cout << static_cast<std::underlying_type<has_hello<B>::name>::type>(has_hello<B>::value) << std::endl;
}

Compared to original codes:

0 -> nullptr
enum -> enum name : type
sizeof -> is_same
typeof -> decltype

Old technique

The version you're using relies on overload resolution. The current form of detection idiom relies on SFINAE. IIRC SFINAE didn't exist pre-C++11.(Thanks to @JDlugosz)

Code Review

Inherit from std::true_type and std::false_type

That will spare a lot of boilerplate, and incorporates with tag dispatch quite well. It might not be directly usable in your case without some indirection and hiding into namespace detail.
Don't use enum as a member where static constexpr bool will do

Even though with freestanding values it is a different story due to linkage. bool will just bring less surprises.
Provide xxx_v value aliases

Somewhat modern approach

Lets take somewhat more interesting example.

  Identify if value of type Element can be push_backed into container of type Container.

Today, it is suggested to first "carve out" the operation itself into an alias:

template <typename Container, typename Element>
using pushback_t = decltype(std::declval<Container>().push_back(std::declval<Element>()));

Then, std::void_t is used:

template <typename Container, typename Element, typename = std::void_t<>>
struct has_pushback : std::false_type{};

template <typename Container, typename Element>
struct has_pushback<Container, Element, std::void_t<pushback_t<Container, Element>>>:
                        std::true_type{};

And then _v value alias is provided:

template <typename Container, typename Element>
inline constexpr bool has_pushback_v = has_pushback<Container, Element>::value;

Demo.

Shameless self PR: if you're a bit confused about how SFINAE works in the new version, I recommend reading this question which I've asked in the past.

Better upgrade path

When std::is_detected will arrive, one can easily put something like this (provided a copy the operation alias from above):

template <typename Container, typename Element>
struct has_pushback : std::is_detected<pushback_t, Container, Element>
{};

That will spare even more boilerplate. See comments for already existing implementations.


Parsing of a file using sed I have a file containing lines having this structure :

var marker25 = createMarker(point, '

Katz\'s Deli
205 E. Houston Street, Manhattan, New York City,
New York, USA

');
var marker26 = createMarker(point, '

Rockerfeller Roof Gardens
5th Avenue between 49th &amp; 50th Streets, Manhattan, New York City,
New York, USA

');

I want to quickly be able to have as a result line of this type:

Katz\'s Deli 205 E. Houston Street, Manhattan, New York City,New York, USA,When_Harry_Met_Sally

Rockerfeller Roof Gardens,5th Avenue between 49th &amp; 50th Streets,,New York, USA, Spider-man

To reiterate, I want to remove all HTML/JS strings to keep only addresses, country and so on.

What I did so far is :

grep 'createMarker' New_York_Filming_Locations.php
cut -d'<' -f1,2,3,4,5,6,7,8
sed 's/
/,/g;s/
/,/g;s/<\/h3>/,/g'
cut -d',' -f3,4,5,7,8,9,11,12
sed 's///g'
cut -d',' -f1,2,4,5,6,7

It is done in a pipe, but for the clarity of the questions, I put each command on a separate line.

Is this the best way to do it? It is a small file, so I do not care about performance.

The sed syntax can be hard to understand. Do you guys know another way to do it using bash?

Consider removing tags with

sed 's/<[^>]*>/ /g'


Simple Bash Parallel Tool (env_parallel dies on big env) This is a simple tool to parallelize execution of multiple tasks in Bash, since env_parallel can't handle the environment size (or something of the sort - dies for mysterious reasons.) I'd love to hear your critique.

#
# Simple parallel executor. Takes a list of commands and executes each in its own child process. Assumes commands are distinctly named.
# Returns with 0 if all are successful. Otherwise, prints out their stdout/stderr outputs and returns with 1.

function simple_par () {

    local TMP_FILE="${WRITABLE_DIR}/simple_par_$(uuidgen)"
    touch $TMP_FILE

    for prog in "$@"
    do
        local PROG_TMP="${TMP_FILE}_${prog}_$(uuidgen)"
        (
            ( $prog 1>$PROG_TMP 2>&1 )
            echo "$?" >> $TMP_FILE
        ) &
    done

    while true
    do
        RESULT_COUNT=$(wc -l ${TMP_FILE} | awk '{ print $1 }')

        if [[ $RESULT_COUNT -eq $# ]]; then
            if [[ $(cat $TMP_FILE) == *"1"* ]]; then
                echo "Failed at least one command"
                for prog in "$@"
                do
                    echo "OUTPUT FOR: $prog"
                    cat ${TMP_FILE}_${prog}_*
                    rm ${TMP_FILE}_${prog}_*
                done

                rm $TMP_FILE
                return 1
            else
                return 0
            fi
        fi

        sleep 5
    done
}

A couple of things could be written better, let's go from top to bottom.

It's recommended to double-quote variables when used as paths,
to avoid globbing and word-splitting.
Even if you're certain that $TMP_FILE won't have any troublesome characters, it's a good practice to double-quote anyway:

touch "$TMP_FILE"

Similarly, you should double-quote $prog in $prog 1>$PROG_TMP 2>&1.

You can simplify for arg in "$@"; do loop as for arg; do.

In Bash, instead of prog 1> somewhere 2>&1 you can write prog &> somewhere.

Instead of this:

RESULT_COUNT=$(wc -l ${TMP_FILE} | awk '{ print $1 }')

You could write simpler without awk like this:

RESULT_COUNT=$(wc -l < "$TMP_FILE")

The trick is that when wc is used with stdin,
it won't print a file name, only the number.
Although RESULT_COUNT will have some whitespace padding,
if you use it in [ ... ] without double-quoting,
it will behave as expected.

Notice again that I double-quoted $TMP_FILE.

Don't use -eq in [[ ... ]], use == or = instead.

This is pretty ugly:

if [[ $(cat $TMP_FILE) == *"1"* ]]; then

A better way to achieve the same thing is using grep:

if grep -q 1 "$TMP_FILE"; then


Filtering wordpress comments by votes I have a website that, based on a user's comments, filters comments by "most liked first, most disliked first" etc...

I have a custom table for comments scoring system and system working with +1 and -1's.

A few days ago I wanted to filter comments by most liked and most disliked as I said and wrote this code:

In my functions.php

function bestanswers($a){

    global $wpdb;

    $results = $wpdb->get_results( 'SELECT * FROM wp_comments LEFT JOIN wp_custom_scoring ON wp_custom_scoring.entryID = wp_comments.comment_ID WHERE class="point" and comment_post_ID='.intval($a).' AND type="plus" GROUP BY entryID ORDER BY COUNT(entryID) DESC');

    if(empty($results)){
        echo 'Its Empty!';
        return false;
    }else{
        return $results;
    }
}

And in my functions.php again:

function score($a,$b){

    if($b == "minus"){
    global $wpdb;
    $results = $wpdb->get_results( 'SELECT count(id) as total,type,class,entryID FROM wp_custom_scoring WHERE type="minus"  and class="point" and entryID='.intval($a));

        if($results[0]->total!= 0){
            return "-".$results[0]->total;
        }else{
            return 0;
        }

    }elseif($b == "plus"){
    global $wpdb;
    $results = $wpdb->get_results( 'SELECT count(id) as total,type,class,entryID FROM wp_custom_scoring WHERE type="plus"  and class="point" and entryID ='.intval($a));

        if($results[0]->total != 0){
            return "+".$results[0]->total;
        }else{
            return 0;
        }

    }else{
    global $wpdb;
    $results = $wpdb->get_results( 'SELECT count(id) as total,class,entryID FROM wp_custom_scoring WHERE  class="fav" and entryID='.intval($a));

    return $results[0]->total;
    }

}

And in comments.php:

if(isset($_GET['filter']) and sanitize_text_field(esc_attr($_GET['filter'])) == 'bestanswers'){

foreach (bestanswers($postID) as $bests) {
if ( score($bests->comment_ID,"plus") + score($bests->comment_ID,"minus") > 0 ){
echo $bests->comment_ID;
comment_text($bests->comment_ID);
///...etc with my custom theme template
}

It's working and I'm able to get everything as I wanted but it's looking ugly to me. Is there are any better way to do this? Or any improvement on my code?

Here is my list of recommendations:

To make the database object available in the custom functions' scope, pass it as a parameter.  I know your global declaration is commonly used by WPers, but I consider it to be sloppy development.  If you are going to persist with using global, then you should only do it once per custom function.
In bestanswers() to make your SQL easier to read, use newlines, indentation, spaces around operators, ALLCAPS mysql keywords, double-quoting the full string, and single-quoting the values in the string. *That said, for the most secure and consistent project, you should use prepared statements anytime you are supplying variables to your query.

$results = $wpdb->get_results(
    "SELECT *
     FROM wp_comments
     LEFT JOIN wp_custom_scoring
         ON wp_custom_scoring.entryID = wp_comments.comment_ID
     WHERE class = 'point'
         AND comment_post_ID = " . (int)$a . "
         AND type = 'plus'
     GROUP BY entryID
     ORDER BY COUNT(entryID) DESC");

empty() does 2 things.  It checks if a variable !isset() OR loosely evaluates as "falsey". You know that $result will be set because you have unconditionally declared it on the previous line of code. (untested...)

if (!$results)) {
    echo 'Its Empty!';
    return false;
}else{
    return $results;
}

Or if you don't need the echo:

return $results ? $results : false;

In scores(), your $b parameter only determines the WHERE clause, so you can DRY out your code like this:

function score($wpdb, $a, $b) {
    if (in_array($b, ['plus','minus']) {
        $where = "class = 'point' AND type = '{$b}'";
    } else {
        $where = "class = 'fav'";
    }

    $results = $wpdb->get_results(
        "SELECT COUNT(id) as total
         FROM wp_custom_scoring
         WHERE entryID = " . (int)$a . "
           AND {$where}";

    if (!$results || !$results[0]->total) {
        return 0;
    } elseif ($a === 'plus') {
        return "+{$results[0]->total}";
    } elseif ($a === 'minus') {
        return "-{$results[0]->total}";
    } else {
        return $results[0]->total;
    }
}

but again, I urge you to consider using prepared statements as a consistent/stable/secure technique across your whole application.

Ultimately, I don't know what is down in the ...etc. portion of your script, but if you are making two function calls to calculate: score($bests->comment_ID,"plus") + score($bests->comment_ID,"minus") > 0, then it may make better sense to create a single custom function that performs this calculation and returns the sum instead.


Rational number arithmetic I was assigned to make a Rational class to perform functions on fractions. After awhile I got it to work, but now I'm wondering how to simplify and clean up my code. Basically how to improve and use good Java practices so I can write better Java in the future.

public class Rational {

    private int numer, denom;

//constructors
        public Rational(){
            int num = 1;
            int den = 2;
            reduce();
        }
        public Rational(int num, int den){
        numer = num;
        denom = den;
        reduce();
        }
        public Rational(Rational x){
        numer = x.numer;
        denom = x.denom;
        reduce();
        }

//setters
        public void setNumer(int num){
        numer = num;
        reduce();
        }
        public void setDenom(int den){
        denom = den;
        reduce();
        }
        public void setRational(int num, int den){
        numer = num;
        denom = den;
        reduce();
        }

//getters
        public int getNumer(){
        return numer;
        }
        public int getDenom(){
        return denom;
        }

//Copy method
        public void copyFrom(Rational x){
        numer = x.numer;
        denom = x.denom;
        reduce();
        }

//Equals method
        public boolean equals(Rational x){
        if (numer / denom == x.numer / x.denom){
        return(true);
                }
        else {
        return(false);
            }
        }

//Compare to method
        public int compareTo(Rational x){
        if (numer / denom == x.numer / x.denom){
        return (0);
        }
        else if (numer / denom < x.numer / x.denom){
        return (-1);
        }
        else{
        return (1);
            }
        }

//Find greatest common divisor
        static int gcd(int x, int y){
        int r;
        while (y != 0) {
        r = x % y;
        x = y;
        y = r;
            }
        return x;
        }

//Rational Addition
        public void plus(Rational x){
        int greatdenom = x.denom * denom;
        int multx = greatdenom / x.denom;
        int mult = greatdenom / denom;
        denom = x.denom * denom;
        numer = (x.numer * multx) + (numer * mult);
        reduce();
        }

//Rational Subtraction
        public void minus(Rational x){
        int greatdenom = x.denom * denom;
        int multx = greatdenom / x.denom;
        int mult = greatdenom / denom;
        denom = x.denom * denom;
        if (x.numer > numer){
        numer = (x.numer * multx) - (numer * mult);
            }
        else {
        numer = (numer * mult) - (x.numer * multx);
            }
        reduce();
        }

//Multiplication
        public void times(Rational x){
        numer = numer * x.numer;
        denom = denom * x.denom;
        reduce();
        }

//Division
        public void divBy(Rational x){
        numer = numer / x.numer;
        denom = denom / x.denom;
        reduce();
        }

//Fraction simplifier
        private void reduce(){
        int divisor;
        divisor = Rational.gcd(numer, denom);
        numer = numer / divisor;
        denom = denom / divisor;
        }

    @Override
        public String toString(){
        if (denom == 1){
        return numer + "";
        }
        else{
        return numer + " / " + denom;
        }
    }
}

Right, there's two parts to this review:

Is what you are doing 'good'?
Is there a better way?

The answer to the second question is 'yes', but we'll have to explore your current code to understand why....

Current code, is it good?

Your code, for the most part, is neat, well-named, and generally understandable. Great. But, there are some significant problems too:

your 'default' constructor creates the Rational 1/2 .... why? What's special about that....? Remove it.
It is better to have one 'core' constructor, and then have the other constructors call it.... :

public Rational(int num, int den){
    numer = num;
    denom = den;
    reduce();
}

public Rational(Rational x){
    this(x.numer, x.demon);
}

The equals(...) and hashCode() contract. If you override one of equals() or hashCode() you should always also override the other.
your equals() method is oddly broken because it should take an Object as an argument. You have a situation where you may compare two rationals in a context where Java will call the equals(Rational) in one situation, but in a different situation (if Java does not know that the value really is a Rational) it may call equals(Object)... and that may return false. This will lead to some interesting confusion..... hmmm.
you implement the method compareTo(Rational) but your class does not implement the Comparable interface. If you implement the Comparable interface you can do things like put your values in an array and then sort them using the native Java mechanisms, etc.
your public interface methods should have comprehensive names, even if your variables do not: setNumer(int) and setDemon(int) should be setNumerator(int) and setDenominator(int) respectively. The same is true for the get* versions. Just because you were a bit lazy and used the auto-generated getter/setter mechanism in your IDE to create these methods, does not mean they are good names.
you are not validating your denominator for division-by-zero.
if your numerator is 0 you should have a special case for it.
you should check for nulls for all input Rationals

OK, that's enough about the style.... let's go through the bugs... ;-)

you do integer division a lot, and you are getting wrong answers (note that this equals() method is broken for other reasons too):

public boolean equals(Rational x){
    if (numer / denom == x.numer / x.denom){
        return(true);
    } else {
        return(false);
    }
}

This is broken, because, consider two Rationals 1/1 and 99/50 ... when you do your math, you are doing integer division, so both of them have the result 1, and 1 == 1 so your code will declare that 1 == 1.5, which is only off by nearly 100%....

This same type of logic is used in the compareTo() method.

Since you reduce() your Rational after every operation, your equals method could be as simple as return numer == x.numer && denom == x.denom;

OK, that's enough about the general issues....

The better way.

Storing your Rational numbers is an important part of many programs. The underlying problem with your class is that it is Mutable. .... you can change the value of a Rational. This is unfortunate for a few reasons:

you cannot use it as a Key in a java.util.Map<Rational,...> because, if the value changes, it will break the internal hashing in the map.
similarly, you cannot store these rationals in a java.util.Set<Rational>().
If you sort your data, the order will change if you change a Rational's value.
The instances are not thread safe.
serializing the instances is a problem.

This problem is well understood, and all the Java number-type classes are Immutable (Integer, Long, Double, BigInteger, BigDecimal, ......).

BigDecimal is a really good example of what you should be doing here. Have a look at it's Javadoc. Notice how all the mathematical methods do not modify the BigDecimal, but return a new BigDecimal with the right value.

Putting all of this together, your class should really look something like:

public final Rational implements Comparable<Rational> {

    private static final int getGCD(int a, int b) {
        .....
        return divisor;
    }

    private static final Rational checkRational(rat) {
        if (rational == null) {
            throw new NullPoiterException("Must supply a non-null Rational value");
        }
        return rat;
    }

    private final int numer, denom;

    public Rational(int numerator, denominator) {
        if (denominator == 0) {
            throw new IllegalArgumentException("Denominator cannot be 0");
        }
        // reduce the value at construct time....
        int div = numerator == 0 ? 1 : getGCD(numerator, denominator);
        // div is guaranteed to be a divisor, so integer division is safe
        this.numer = numerator/div;
        this.denom = numerator == 0 ? 1 : denominator/div;
    }

    public Rational(Rational other) {
        this(other.numer, other.denom);
    }

    @Override
    public int hashCode() {
        // somewhat convenient hashcode ... it's a bitwise trick...
        // could be improved...
        return numer * 31 ^ denom * 31;
    }

    @Override
    public boolean equals(Object other) {
        return other instanceof Rational
              && ((Rational)other).numer == numer
              && ((Rational)other).denom == denom;
    }

    @Override
    public int compareTo(Rational other) {
        checkRational(other);
        Rational difference = subtract(other);
        if (difference.numer > 0) {
            return 1;
        }
        if (difference.numer < 0) {
            return -1;
        }
        return 0;
    }

    // return a new instance instead of updating ourselves...
    public Rational add(Rational addend) {
        checkRational(other);
        return new Rational(addend.numer * denom + numer * other.denom, denom * other.denom);
    }

    ....... Lots more .....

}


Program that provides diagrams for documentation The general problem I'm trying to solve is that I have a code base that has zero documentation so I'm trying to automatically generate some diagrams of classes in package that I'm analyzing using erdantic.
Trying to make this as reusable as possible, so I have a generate_diagrams() function that should take in one argument, which is the package I want to iterate through.
I often feel like I throw the terms module and package around interchangeably — I might be doing it now — so I defaulted my argument to the name "directory".  But I feel that's not right since I'm not passing in a path.
Should I change the argument in generate_diagrams() below? What's a more accurate name?
import erdantic as erd
import glob
import importlib
import example.models

from os import path, makedirs

def draw_diagram(module, cls_name, output_path):
    """

    Args:
        module: module object
        cls_name: name of the class that's associated with  the module
        output_path: path where diagram to be saved

    Returns:
        void - outputs .png file at given path

    """
    try:
        diagram = erd.create(module.__getattribute__(cls_name))
        diagram.models
        diagram.draw(f"{output_path}/{cls_name}.png")

    except erd.errors.UnknownModelTypeError:
        pass

def generate_diagrams(directory):
    """

    Args:
        directory: package you have imported that contains module(s) you wish to diagram

    Returns:
        void - generates diagram using draw_diagram function

    """

    doc_path = path.dirname(__file__)
    modules = glob.glob(path.join(path.dirname(directory.__file__), "*.py"))
    module_names = [path.basename(f)[:-3] for f in modules if path.isfile(f) and not f.endswith('__init__.py')]

    for module_name in module_names:

        if not path.exists(path.join(doc_path, module_name)):
            print(f'Making {module_name} directory')
            makedirs(module_name)

        module = importlib.import_module(f'{directory.__name__}.{module_name}')
        classes = dict([(name, cls) for name, cls in module.__dict__.items() if isinstance(cls, type)])

        for name, cls in classes.items():
            print(f'Drawing {module_name}.{name} diagram')
            draw_diagram(module, cls_name=name, output_path=path.join(doc_path, module_name))

def main():
    generate_diagrams(directory=example.models)

if __name__ == "__main__":
    main()

The Doxygen recommendation notwithstanding,

Consider adding PEP484 type hints, particularly to the signatures of functions such as draw_diagram. For that method, it's not really correct Python terminology to call the return value "void" - it's None.
Does the single-expression statement diagram.models actually have any effect? If so that deserves a comment.
Rather than glob.glob consider pathlib.Path.glob which has some usage niceties
path.basename(f)[:-3] is a very fragile way of acquiring a stem. Again, pathlib.Path can do a better job.

The line
dict([(name, cls) for name, cls in module.__dict__.items() if isinstance(cls, type)])

first of all should not use an inner list, since dict can directly accept a generator; and secondly would be better-represented as a dictionary comprehension:
{name: cls for name, cls in module.__dict__.items() if isinstance(cls, type)}

This loop:
    for name, cls in classes.items():
        print(f'Drawing {module_name}.{name} diagram')
        draw_diagram(module, cls_name=name, output_path=path.join(doc_path, module_name))

doesn't actually use cls, so should only iterate over the dictionary's keys, not its items.


Leap year check in Haskell, using pattern matching or bind operator One of the first Haskell puzzles on http://exercism.io/ is to implement a leap year check. I've done this twice.

Using pattern matching:

isLeapYear :: Integer -> Bool
isLeapYear year
  | year `mod` 400 == 0 = True
  | year `mod` 100 == 0 = False
  | year `mod` 4 == 0   = True
  | otherwise           = False

Using the bind operator >>=:

isLeapYear :: Integer -> Bool
isLeapYear year = head $
                  [(400, True), (100, False), (4, True), (1, False)]
                  >>= check year
  where check y (interval, isLeap) = [isLeap | y `mod` interval == 0]

I'd like to know which implementation is “better“ / more idiomatic in Haskell. I am unsure whether I might have misused a t0o powerful concept in the second try, and the first try might just be more readable.

The first one is a lot more readable, whereas the second one uses a "hack". I would go with the first one, except that I would use rem, which is a little bit faster. And one could introduce some DRY:

isDivisibleBy :: Integral n => n -> n -> Bool
isDivisibleBy x n = x `rem` n == 0

isLeapYear :: Integer -> Bool
isLeapYear year
  | divBy 400 = True
  | divBy 100 = False
  | divBy   4 = True
  | otherwise = False
 where
   divBy n = year `isDivisibleBy` n

That being said, for a programming challenge, your version is perfectly fine:

isLeapYear :: Integer -> Bool
isLeapYear year
  | year `rem` 400 == 0 = True
  | year `rem` 100 == 0 = False
  | year `rem`   4 == 0 = True
  | otherwise           = False

The latter can be rewritten without >>= as list comprehension:

isLeapYear :: Integer -> Bool
isLeapYear year = head [isLeap | (interval, isLeap) <- classifications
                               , year `isDivisibleBy` interval]
  where
    classifications = [(400, True), (100, False), (4, True), (1, False)]

You could get rid of the "hack" with safeHead and maybe False, but that's left as an exercise.

If you really want to use check, remove the y. It just introduces an additional error source:

isLeapYear :: Integer -> Bool
isLeapYear year = head $
                  [(400, True), (100, False), (4, True), (1, False)]
                  >>= check
  where check (interval, isLeap) = [isLeap | year `rem` interval == 0]

Note that this shows perfectly that >>= is just flip concatMap for lists. So let's take advantage:

isLeapYear :: Integer -> Bool
isLeapYear year = head $ concatMap check classifications ++ [False]
  where
    check (interval, isLeap) = [isLeap | year `rem` interval == 0]
    classifications = [(400, True), (100, False), (4, True)]

Which is easier to grasp than the version with >>=.


What a crooked way to compute the next straight string (advent of code 11) This year I have been participating in the Advent of Code series of challenges, and it just so happened that I'd be doing them in Javascript. Not my usual weapon of choice, but I do have some experience in it.

Day 11 asks us to find the next valid password given a set of three requirements. The idea is that you'd be incrementing the given password until it matched all three criteria, but I wanted to do something more efficient. But the task "find the next string that has a straight of length three" turned out a bit more difficult than I anticipated, and the code happened to be a bit more convoluted than I'd like, whooping 38% of the whole challenge.

A straight is defined as

  like abc, bcd, cde, and so on, up to xyz. They cannot skip letters; abd doesn't count.

My main concern is that my code isn't "obviously bug-free" and that the logic is hard to follow. How can I clean my logic up without sacrificing performance? Of course, reviewers are welcome to comment on other aspects of the code as well.

So, here we go:

//jshint esversion: 6

function nextChar(x){return String.fromCharCode(x.charCodeAt(0)+1);}
var requirements = [
  {name: "straight3", nextMatch: function nextMatch(str){
    var prefix = str;
    while(prefix.length > 2){
      if(prefix === straighten(prefix)) return str;
      prefix = prefix.slice(0,-1);
    }
    prefix = str;
    while(true){
      if(prefix.length <= str.length - 3){
        return str.replace(/(.)([z]*)...$/, function(_, c, zs){
          return nextChar(c) + zs.replace(/z/g, 'a') + "abc";
        });
      } else if(prefix.match(/[yz]..$|xz.$/)) prefix = prefix.slice(0,-1);
      else if(str < straighten(prefix)) {
        var rv = straighten(prefix);
        while(straighten(rv).slice(0,-1) === straighten(rv.slice(0,-1))) rv = rv.slice(0,-1)
        while(rv.length < str.length) rv += 'a';
        return rv;
      } else {
        prefix = prefix.replace(/.(?=..$)/, nextChar).replace(/..$/, "aa");
        while(prefix.length < str.length) prefix += 'a';
        return nextMatch(prefix);
      }
    }
    function straighten(str){
      var ord = str.slice(-3).charCodeAt(0);
      return str.slice(0,-3) + String.fromCharCode(ord, ord+1, ord+2);
    }
  }},
  {name: "noIOL", nextMatch: function(x){
    return x.replace(/([iol])(.*)$/g, function(_, c, xs){
      return nextChar(c) + xs.replace(/./g, 'a');
    });
  }},
  {name: "twoPairs", nextMatch: function(str){
    if(str.match(/(.)\1.*(.)\2/)) return str;
    else if(str.match(/(.)\1../)){
      var rv = str.slice(0,-2) + nextPair(str.slice(-2));
      return rv.replace(/(.)\1\1$/, "$1$1a");
    } else {
      var rv = str.slice(0,-4) + nextPair(str.slice(-4,-2)) + "aa";
      return rv.replace(/(.)\1\1aa$/,"$1$1aaa");
    }
    function nextPair(x){
      if(x[0] > x[1]) return x[0] + x[0];
      else return nextChar(x[0]) + nextChar(x[0]);
    }
  }}
];

var pwd = "hxbxxzaa";
var toSat = requirements.length;
while(toSat){
  var oldPwd = pwd;
  pwd = requirements[0].nextMatch(pwd);
  if(pwd === oldPwd){
    toSat --;
  }else{
    toSat = requirements.length;
    console.log(oldPwd + " -> " + requirements[0].name + " -> " + pwd);
  }
  if(pwd.match(/[^a-z]/)) throw "oops";
  requirements.push(requirements.shift());
}
console.log(pwd);

First of all, I can't tell if your code is simply matching some password with the rules, generating one, or generating the next valid one. It's best if you split the operations into small functions so that it's easy to read through. I'd expect a function like generateNextPossiblePassword(pwd) somewhere that goes like:

var newPassword = generateNextPossiblePassword(OLD_PASSWORD);

The code also needs some formatting. You have an object which you are one-lining, then some you expand. Just ran this through JSBeautifier and it looks better than the one posted. Much readable.

As far as I understand, you're just generating passwords and checking if it passes the requirements. Generating should be a straightforward function:

function generateNextPossiblePassword(oldPassword){...}

var potentialPassword = generateNextPossiblePassword('abcdefgh'); //abcdffaa

Now, you don't have to pack everything in the generateNextPossiblePassword function. You can split the operation into generateNextString and checkIfStringIsValidPassword. That way, generateNextString can focus on string generation while checkIfStringIsValidPassword is the actual function that checks the validity.

function generateNextString(str){...}

function doesPasswordPassAllTests(str){...}

function generateNextPossiblePassword(pwd){
  var potentialPwd = generateNextString(pwd);
  var isValidPwd = doesPasswordPassAllTests(potentialPwd);

  // If the value passes, return. Otherwise, call again.
  // Yep, recursion. Lesser temporary counter variables.
  return isValidPwd ? potentialPwd : generateNextPossiblePassword(potentialPwd);
}

Checking against requirements is simply an every operation. every runs the callback for each item. If everything returns true, the result of every is true. If it finds one false, it immediately bails out and the operation results in a false.

Also note that the checks are functions which are then put into an array. This makes it easy to just define a function, then if we ever want to add it, we just pop in its name in the array. Your array isn't very bulky, just references to the functions.

// They take in a password, and simply return true or false
function checkForStraight(pwd){...}

function checkForInvalidLetters(pwd){...}

function checkForTwoPairs(pwd){...}

function doesPasswordPassAllTests(pwd){
  return passwordTests.every(function(test){
    return test(pwd);
  });
}

var passwordTests = [
  checkForStraight,
  checkForInvalidLetters,
  checkForTwoPairs,
];

var passwordPassesAllTests = doesPasswordPassAllTests(NEW_PASSWORD);

I renamed noIOL to checkForInvalidLetters. That's because I, O and L are arbitrary. A more general way to classify this would be character restriction, hence the new name.

By now, we've simplified your code. You can then worry about writing the logic and squeeze performance. Now that they're split, you can optimize them individually. For instance, you can optimize generateNextString to generate passwords as close to the rules as possible to avoid excessive cycles. The checks may easily be regex checks or even just regular string parsing.

It's also very readable. You can easily fold the function blocks in your IDE, only revealing their very meaningful names. Additionally, it's really easy to test each function.

function generateNextPossiblePassword(oldPassword){...}

function generateNextString(str){...}

function doesPasswordPassAllTests(pwd){...}

function checkForStraight(pwd){...}

function checkForInvalidLetters(pwd){...}

function checkForTwoPairs(pwd){...}

var passwordTests = [
  checkForStraight,
  checkForInvalidLetters,
  checkForTwoPairs,
];

var newPassword = generateNextPossiblePassword(OLD_PASSWORD);


Print all combinations of balanced parentheses Given ( and ) and length of 4, generate the following combinations -
  ()() and (()).

Looking for code review, optimizations and best practices. Also verifying complexity to be O(2(n+1) - 1), where n is half the 'input length'.

public final class BraceCombinations {

    private BraceCombinations() {}

    /**
     * Returns sets of valid combinations.
     *
     * @param length    the sum of length of all- opening + closing braces
     * @return          the valid brace combinations
     */
    public static List<List<String>> getBraceCombinations(int length) {
        if (length <= 0) {
            throw new IllegalArgumentException("The length should be greater than zero");
        }

        if (length % 2 == 1) {
            throw new IllegalArgumentException("The length should be positive");
        }

        final List<List<String>> paranthesesCombo = new ArrayList<List<String>>();
        generate(paranthesesCombo, new LinkedList<String>(), 0, 0, length/2);
        return paranthesesCombo;
    }

    private static void generate(List<List<String>> paranthesesCombo,  LinkedList<String> parenthesis, int openBrace, int closeBrace, int halfLength) {
        if (openBrace == halfLength && closeBrace == halfLength) {
            paranthesesCombo.add(new ArrayList<String>(parenthesis));
        }
        if (openBrace > halfLength || closeBrace > openBrace) {
            return;
        }
        parenthesis.add("(");
        generate(paranthesesCombo, parenthesis, openBrace + 1, closeBrace, halfLength);
        parenthesis.removeLast();

        parenthesis.add(")");
        generate(paranthesesCombo, parenthesis, openBrace, closeBrace + 1, halfLength);
        parenthesis.removeLast();
    }

    public static void main(String[] args) {
        List<List<String>> paths1 = Arrays.asList(
            Arrays.asList("(", ")")
        );
        assertEquals(paths1, getBraceCombinations(2));
        List<List<String>> paths = Arrays.asList(
                Arrays.asList("(", "(", ")", ")"),
                Arrays.asList("(", ")", "(", ")")
            );
        assertEquals(paths, getBraceCombinations(4));
    }
}

Jus a few minor points:

I guess you have a typo here:

if (length % 2 == 1) {
    throw new IllegalArgumentException("The length should be positive");
}

If length is 3 it prints that The length should be positive.
An exaplanatory variable would be a little bit easier to read here and it would express the purpose of the expression:

final boolean oddLength = (length % 2 == 1);
if (oddLength) {
    throw new IllegalArgumentException("...");
}

It could be easier to read and modify the expected outputs in the tests, especially if length is higher than four:

Arrays.asList("(", "(", "(", ")", ")", ")"),

I'd rather use a custom verification method here:

private static void verifyPaths(final List<String> expectedPaths,
        final int length) {
    final List<List<String>> braceCombinations = getBraceCombinations(length);

    final List<String> joinedBraceCombinations =
        flattenBraceCombinations(braceCombinations);

    assertEquals(expectedPaths, joinedBraceCombinations);
}

private static List<String> flattenBraceCombinations(
        final List<List<String>> braceCombinations) {
    final Joiner joiner = Joiner.on("");
    final List<String> joinedBraceCombinations = new ArrayList<>();
    for (final List<String> braceCombination: braceCombinations) {
        final String joinedBraceCombination = joiner.join(braceCombination);
        joinedBraceCombinations.add(joinedBraceCombination);
    }
    return joinedBraceCombinations;
}

With plain strings as expected output:

final List<String> expectedPaths = Arrays.asList(
    "((()))",
    "(()())",
    "(())()",
    "()(())",
    "()()()");
verifyPaths(expectedPaths, 6);

I would consider changing the current

List<List<String>>

return type to

Set<List<String>>

The order of combinations doesn't look so important and it would make the test less specified (the order of expected paths would not be important too).


Self-implemented C++ exception class I wrote my own exception class, deriving from std::runtime_error to have Error-IDs, timestamps and inner exceptions. It seems to work, but are there any drawbacks?

The only thing I see is the deep-copy in the copy-constructor, which is not efficient, when there are many nested exceptions. But exceptions should be rare and not be nested too much, so I think it's a downside I can cope with.

#pragma once

#include <stdexcept>
#include <string>
#include <sstream>
#include <time.h>
#include <memory>

class MyException : public std::runtime_error
{
public:
    MyException(const MyException& exception)
        : std::runtime_error(exception.what()),
        exceptionId(exception.id()),
        ts(exception.timestamp()),
        innerException(NULL)
    {
        if (exception.inner() != NULL)
        {
            innerException = new MyException(*exception.inner());
        }
        else
        {
            innerException = NULL;
        }
    }

    MyException(const std::string& _Message)
        : std::runtime_error(_Message),
            exceptionId(0),
        innerException(NULL)
    {
        time(&ts);
    }

    MyException(const std::string& _Message, unsigned int id)
        : std::runtime_error(_Message),
        exceptionId(id),
        innerException(NULL)
    {
        time(&ts);
    }

    MyException(const std::string& _Message, unsigned int id, MyException* innerException)
        : std::runtime_error(_Message),
        exceptionId(id),
        innerException(new MyException(*innerException))
    {
        time(&ts);
    }

    virtual ~MyException()
    {
        delete innerException;
    }

    unsigned int id() const { return exceptionId; }
    time_t timestamp() const { return ts; }
    const MyException* inner() const { return innerException; }

private:
    unsigned int exceptionId;
    time_t ts;
    const MyException* innerException;
};

This is how I would use it:

void handleException(MyException *ex)
{
    cout << "exception " << ex->id() << " - " << ex->what() << endl;
    const MyException* innerException = ex->inner();
    int t = 1;
    while (innerException != NULL)
    {
        for (int i=0; i<t; ++i)
        {
            cout << "\t";
        }
        ++t;
        cout << "inner exception " << innerException->id() << " - " << innerException->what() << endl;
        innerException = innerException->inner();
    }
}

void throwRecursive(int temp)
{
    if (temp == 0)
    {
        throw runtime_error("std::runtime_error");
    }

    try
    {
        throwRecursive(--temp);
    }
    catch (MyException &ex)
    {
        throw MyException("MyException", (temp+1), &ex);
    }
    catch (exception &ex)
    {
        throw MyException(ex.what(), (temp+1));
    }
}

void myExceptionTest()
{
    try
    {
        throwRecursive(3);
    }
    catch (MyException &ex)
    {
        handleException(&ex);
    }
}

And the output:

exception 3 - MyException
        inner exception 2 - MyException
                inner exception 1 - std::runtime_error

I already found out here that I'm violating the rule of three, having forgotten the copy assignment operator.

Other issues mentioned: cloning, reserved names and throw specifications.

Can anyone tell me more about that?

Not all compilers support #pragma once

#pragma once

There are C++ equivalent of most C libraries that put the appropriate interface into the standard namespace.

#include <time.h>

// Prefer

#include <ctime>

My pet peeve (OK second after using namespace std;); because everybody thinks they know the rules but get it wrong all the time:

_Message

DO NOT USE IDENTIFIERS WITH A LEADING UNDERSCORE. Even if you know the rules most people get it wrong (such as this case) as a result it leads to maintenance problems. https://stackoverflow.com/a/228797/14065 An identifier with a leading underscore and a capital letter is reserved in all scopes (i.e. the implementation is potentially going to use a macro with that name and Message seems like a prime candidate).

Your three versions of the constructor can be written as a single constructor:

    MyException( const std::string& _Message
               , unsigned            int id = 0             // Use default values
               , MyException*        innerException = NULL) // That way you do not have
        : std::runtime_error(_Message)                      // three versions of the
        , exceptionId(id)                                   // same constructor
        , innerException(innerException)
    {
        time(&ts);
    }

A class is its own friend.
So you don't need to call getter methods to access members of the object you are copying in the copy constructor; you can just get the values. When constructing the base class (std::runtime_error) why not take advantage of its copy constructor (it may have optimizations that you are failing to take use of by using the non standard version).

MyException(const MyException& exception)
    : std::runtime_error(exception)
    , exceptionId(exception.exceptionId)
    , ts(exception.ts)
    , innerException(NULL)

The inner exception is passed around as a pointer. So we have no ownership semantics associated with it. You attempt to take ownership but fail (because you do not implement the rule of 3 (5 in C++11)).

{
    if (exception.inner != NULL)
    {
        // Slicing problem if exception.inner is derived from MyException
        innerException = new MyException(exception.inner);
    }
    else
    {
        // Already set in the initializer list no need to set again.
        innerException = NULL;
    }
}

Also because of the way you implement it you can not subclass MyException because the copy constructor will slice your object on copy. You should probably use a smart pointer to handle ownership of the exception. Since sharing the same exception would solve most of your problems the easy way out is to use shared_ptr<X>. Personally I would make X a std::runtime_errornotMyException` thus allowing you to chain on standard exceptions (or anything derived from them).

In the handler:

void handleException(MyException *ex)

Why pass a pointer to the exception. Now you have to check for NULL. Pass it as a reference.


Database method to query I have a database class that in __construct() initialize a PDO connection and insert the instance into a $db private var.
Now i'm working on a method that can be used to query in this way:

$db = new db;
$db->query(array(
 'select' => 1,
 'from' => 'table',
 'where' => array('id' => 1, 'name' => 'charlie'),
 'limit' => array(1, 5)
));

I did something that works pretty nicely long time ago while PDO was something unknown, but i was wondering:

How could i improve this code a bit
How can i end it? I mean how to use the PDO then to submit the query?

Here's the method query():

# Defining the type
     if (isset($array['select'])) { $type = 'SELECT'; $type_value = (is_int($array['select'])) ? '*' : $array['select']; }
     if (isset($array['update'])) { $type = 'UPDATE'; $type_value = $array['update']; }
     if (isset($array['delete'])) { $type = 'DELETE FROM'; $type_value = $array['delete']; }
     if (isset($array['insert'])) { $type = 'INSERT INTO'; $type_value = $array['insert']; }
     if (!isset($type)) { trigger_error("Database, 'type' not selected."); } // Error

     # From
     if (isset($array['from']))
     {
      $from = 'FROM';
      $from_value = mysql_real_escape_string($array['from']); // table cannot be pdoed
     }
     # Where
     if (isset($array['where']))
     {
      if (!is_array($array['where'])) { trigger_error("Database, 'where' key must be array."); }
      $where = 'WHERE'; $where_value = $array['where'];
      # Fixing the AND problem
      if (count($array['where']) > 1)
      {
       $list = $where_value;
       foreach ($list as $a => $b) { $w[] = "{$a} = {$b}"; }
       $and = implode(' AND ', $w);
       $where_value = $and;
      }
     }
     # Limit
     if (isset($array['limit']))
     {
      if (!is_array($array['limit'])) { trigger_error("Database, 'limit' key must be array."); }
      if (count($array['limit']) != 2) { trigger_error("Database, 'limit' array must be two-keys-long"); }
      $limit_first = $array['limit'][0];
      $limit_second = $array['limit'][1];
      $limit = 'LIMIT';
      $limit_value = "{$limit_first}, {$limit_second}";
     }

     # Set
     if (isset($array['set']))
     {
      if (!is_array($array['set'])) { trigger_error("Database, 'set' key must be array."); }
      $edits = $array['set'];
      foreach ($edits as $a => $b) { $e[] = "{$a} = {$b}"; }
      $set = 'SET';
      $set_value = implode(',', $e);
     }

     $vals = array('from', 'from_value', 'set', 'set_value', 'where', 'where_value');
     foreach ($vals as $v) { if (empty($$v)) { $$v = ''; } }

     $sql = "
      {$type} {$type_value}
      {$from} {$from_value}
      {$set} {$set_value}
      {$where} {$where_value}
     ";

# Here there would be something like mysql_query($sql), but i'd like PDO! PDO get me hornier.

And now? How to bind parameters? Is that possible to work it out?

That query method (and the db class) has a lot of responsibilities:

Do the PDO stuff, connection handling
Be a query builder
be a query executor
Handle the params and possibly execute the same statement with different params (it would to that too)
Do all the error checking
maybe more i don't see

Usually that functionality is handled in 2 to 3 classes, sometimes even more and not in one single function.

And you are doing some very creepy magic to achieve all the work

foreach ($vals as $v) { if (empty($$v)) { $$v = ''; } }

and all that so you can write

array("select " => "something" , ...

instead of

"select something" ...

Also you are using mysql_real_escape_string so it seems you don't want to use the pdo escaping (not binding parameters) so why try to use PDO if you limit yourself to mysql anyways ?

So far everything i spotted thinking about it for 5 minutes. Will improve upon feedback / direction from you if it helped at all :)


"Hello, world!" program using a class for printing Please take a look at my program and let me know how I can improve it.

/*
"   To Print A Line On The Display Screen"
    Date:5th January 2011
    Programmer:Fahad
*/
#include <iostream>
using namespace std;
class Print
{
    public:
        void print_();
};
int main()
{
    Print Obj;
    Obj.print_();
    system( "pause" );
    return 0;
}
void Print::print_()
{
    cout << "I am in print function and the program runs fine." << endl;
}

In addition to the other comments, I would also use a different naming convention for types and objects.

For example, this looks unconventional.

Print Obj;
Obj.print_();

I prefer:

Print obj;
obj.print();

It's just a convention but being able to easily spot names that denote types helps if you start to use more complex expressions. For example:

Print().print();

Personally, I would also avoid system("pause"). You need to #include either <stdlib.h> or <cstdlib> to use it. Although the system call itself is standard C++ (from the standard C library), what you pass to it is system dependent.

In general I don't believe you should make your programs stop artificially. If they are designed to run in a terminal then the terminal user will be able to see the output even after the program exits.