Handling date rollover in same function I have a program that specifies when an alarm should be active (say, 10AM to 8PM). Part of my program then checks if the alarm is active right now. The "is it active" checker needs to deal with alarm ranges that cross the midnight boundary into tomorrow (say, 8PM to 10AM alarm).

I am currently handling this with redundant functions that are identical, except for AND vs OR comparison. What is the better way, please? (Of course, any other review comments are also most welcome.)

import datetime as dt

def check_alarm_bounds_on_same_date(time_now, time_of_day_bounds):
    time_start_of_current_day = dt.datetime.combine(time_now.date(), dt.datetime.min.time())
    if ( time_now >= (time_start_of_current_day + dt.timedelta(hours=time_of_day_bounds[0])) and #<< AND
         time_now < (time_start_of_current_day + dt.timedelta(hours=time_of_day_bounds[1])) ):
        return True
    return False

def check_alarm_bounds_on_different_dates(time_now, time_of_day_bounds):
    time_start_of_current_day = dt.datetime.combine(time_now.date(), dt.datetime.min.time())
    if ( time_now >= (time_start_of_current_day + dt.timedelta(hours=time_of_day_bounds[0])) or #<< OR
         time_now < (time_start_of_current_day + dt.timedelta(hours=time_of_day_bounds[1])) ):
        return True
    return False

def is_alarm_live(time_current, time_of_day_bounds):
    alarm_start_time = time_of_day_bounds[0]
    alarm_end_time = time_of_day_bounds[1]
    if alarm_end_time <= alarm_start_time:
        return check_alarm_bounds_on_different_dates(time_current, time_of_day_bounds)
    else:
        return check_alarm_bounds_on_same_date(time_current, time_of_day_bounds)

# specify two alarm periods (#1 @ 10AM-8PM; #2 @ 8PM-10AM next day)
alarms = [(10,20),(20,10)]
for hour in range(0,24):
    # test all hours in a sample day, and output which alarm is active
    print 'hour %i: ' % hour,
    time_test = dt.datetime(2017,01,22,hour,10,30)
    for alarm in alarms:
        print is_alarm_live(time_test, alarm),
    print ''

Very quick review.

No docstrings.
The code uses datetime.datetime.combine to make datetime.datetime objects representing the bounds, and then compares these with time_current. But it would be simpler to make datetime.time objects to represent the bounds, and then compare those with time_current.time.
Having done that, it would be simpler still if the function took a datetime.time object as its first argument, leaving it up to the caller to pass time_current.time if that's what's needed.
There's nothing about this code that is specific to alarms (it could be used for appointments, for example), so it would be easier to understand if the functionality were expressed abstractly.

Revised code:

import datetime

def time_in_range(time, start_hour, end_hour):
    """Return True if time (a datetime.time object) lies between
    start_hour and end_hour (inclusive).

    >>> time_in_range(datetime.time(hour=12), 10, 14)
    True

    The range is allowed to span midnight:

    >>> time_in_range(datetime.time(hour=0), 22, 2)
    True

    """
    start = datetime.time(hour=start_hour)
    end = datetime.time(hour=end_hour)
    if end <= start: # Range spans midnight.
        return start <= time or time <= end
    else:
        return start <= time <= end


Create path to file under 64-bit %programfiles% from 32-bit process I have a process that runs as 32-bits regardless of the architecture.  In it, I want to be able to spawn a process from the 64-bit program files menu (e.g. c:\program files instead of c:\program files (x86)\).

I tried using System.Environment.GetFolderPath, but for a 32 bit process, both SpecialFolders.ProgramFiles and SpecialFolders.ProgramFilesX86 returned the x86 folder.  Ditto trying to use System.Environment.ExpandEnvironmentVariables("%programfiles%").

Instead I put together this mess:

Path.Combine(System.Environment.ExpandEnvironmentVariables("%systemdrive%") + @"\", @"Program Files\...");

The Path.Combine seems pretty useless since I've already put every '\' in there, so I simplified it to:

System.Environment.ExpandEnvironmentVariables("%systemdrive%") + @"\Program Files\...";

In my environment(s), the program files folder is always on the system drive, but the system drive has varying drive letters.  Is there any better way to write this?

In the end, I made a slight simplification and ended up with:

System.Environment.ExpandEnvironmentVariables("%systemdrive%\Program Files\...")


Create a state dropdown with an abbreviation option and a full option I have this simple function here because I am too lazy to write out a state dropdown every time I need one. I already have an array with all the states set up, abbreviations as keys and names as values. It works just fine, but I like to write concise, clean, legible code, and I am still very unseasoned, so I find I miss a lot of stupidly simple s*** (the price I pay for teaching myself and not practicing often enough).

I see the four lines of redundant code inside the two primary conditionals, and I would move them outside the if statement, except that I have that last echo statement, which is really the whole purpose for the condition in the first place, and I don't think retesting the $style variable is all that kosher.

function create_state($style, $form = NULL) {
  global $state;
  echo '<select name="state">'."\n";
  echo '<option value="">--</option>'."\n";
  if ($style === 'abbr') {
    ksort($state);
    foreach ($state as $abbr => $name) {
      echo "<option value=\"$abbr\"";
      if($form === $abbr)
        echo 'selected="selected"';
      echo ">$abbr</option>\n";
    }
  } else {
    asort($state);
    foreach ($state as $abbr => $name) {
      echo "<option value=\"$abbr\"";
      if($form === $abbr)
        echo 'selected="selected"';
      echo ">$name</option>\n";
    }
  }
  echo '</select>'."\n";
}

I'm going to assume here (having done this myself before when I first started out with PHP), that you are storing the state data in a MySQL database, which you are pulling and storing in the global $state var.

Do not do that. You're slowing down the entire program by making a call out to the database for what is pretty much static data (the last state to join the union joined 55 years ago).

Now, I would just have the dropdowns stored in functions which just return a string. One function for long name, one for abbreviated.

However, using this as an exercise for you in DRY, I went ahead and created a class, StateDropDown, which contains an associative array of states/properties and their abbreviation. On initiation, the class generates the output form. If the $style contains 'abbr', then we use the abbreviated version after performing a ksort(). Otherwise, since the data in the array is static and already sorted as we prefer, we don't perform an asort and just go straight to storing the select form in a property $output.

class StateDropDown{
    protected $states = array(
        "Alabama" => "AL",
        "Alaska" => "AK",
        "Arizona" => "AZ",
        "Arkansas" => "AR",
        "California" => "CA",
        "Colorado" => "CO",
        "Connecticut" => "CT",
        "Delaware" => "DE",
        "Florida" => "FL",
        "Georgia" => "GA",
        "Hawaii" => "HI",
        "Idaho" => "ID",
        "Illinois" => "IL",
        "Indiana" => "IN",
        "Iowa" => "IA",
        "Kansas" => "KS",
        "Kentucky" => "KY",
        "Louisiana" => "LA",
        "Maine" => "ME",
        "Maryland" => "MD",
        "Massachusetts" => "MA",
        "Michigan" => "MI",
        "Minnesota" => "MN",
        "Mississippi" => "MS",
        "Missouri" => "MO",
        "Montana" => "MT",
        "Nebraska" => "NE",
        "Nevada" => "NV",
        "New Hampshire" => "NH",
        "New Jersey" => "NJ",
        "New Mexico" => "NM",
        "New York" => "NY",
        "North Carolina" => "NC",
        "North Dakota" => "ND",
        "Ohio" => "OH",
        "Oklahoma" => "OK",
        "Oregon" => "OR",
        "Pennsylvania" => "PA",
        "Rhode Island" => "RI",
        "South Carolina" => "SC",
        "South Dakota" => "SD",
        "Tennessee" => "TN",
        "Texas" => "TX",
        "Utah" => "UT",
        "Vermont" => "VT",
        "Virginia" => "VA",
        "Washington" => "WA",
        "West Virginia" => "WV",
        "Wisconsin" => "WI",
        "Wyoming" => "WY",
        "American Samoa" => "AS",
        "District of Columbia" => "DC",
        "Federated States of Micronesia" => "FM",
        "Guam" => "GU",
        "Marshall Islands" => "MH",
        "Northern Mariana Islands" => "MP",
        "Palau" => "PW",
        "Puerto Rico" => "PR",
        "Virgin Islands" => "VI",
        "Armed Forces Africa" => "AE",
        "Armed Forces Americas" => "AA",
        "Armed Forces Canada" => "AE",
        "Armed Forces Europe" => "AE",
        "Armed Forces Middle East" => "AE",
        "Armed Forces Pacific" => "AP"
    );
    public $output = "";

    public function __construct($style = false, $form = null){
        if($style === true){
            ksort($this->states);
        }

        $this->output = "<select name='state'>";
        $this->output .= "<option value=''>--</option>";

        foreach($this->states as $name => $abbr){
            $this->output .= "<option value='" . $abbr . "'";
            if($form === $abbr){
                $this->output .= "selected='selected'";
            }

            if($style === true){
                $this->output .= ">" . $abbr . "</option>";
            } else {
                $this->output .= ">" . $name . "</option>";
            }
        }
        $this->output .= "</select>";
    }
}

To use, simply instantiate and echo output.

Here, we do a regular state name, with no selected state:

$dropdown = new StateDropDown();
echo $dropdown->output;

Here, we do a regular state name, with a selected state, NY:

$dropdown = new StateDropDown(null, "NY");
echo $dropdown->output;

Here, we do an abbreviated state name, with no selected state:

$dropdown = new StateDropDown(true);
echo $dropdown->output;

Here, we do an abbreviated state name, with a selected state, FL:

$dropdown = new StateDropDown(true, 'FL');
echo $dropdown->output;

While we may have the overhead of instantiating a new object, etc - we save on a possible database call. Additionally, we no longer do multiple checks, but rather do one check for abbreviation preference and assume the non-abbreviated by default, as well as keeping it non-abbreviated sort by default.

EDIT

Since OP indicated that I was incorrect in my assumption that they were storing this data in a database, I've modified my above Class into a function which does the same thing:

function statesDropdown($style = false, $form = null){
    $states = array(
        "Alabama" => "AL",
        "Alaska" => "AK",
        "Arizona" => "AZ",
        "Arkansas" => "AR",
        "California" => "CA",
        "Colorado" => "CO",
        "Connecticut" => "CT",
        "Delaware" => "DE",
        "Florida" => "FL",
        "Georgia" => "GA",
        "Hawaii" => "HI",
        "Idaho" => "ID",
        "Illinois" => "IL",
        "Indiana" => "IN",
        "Iowa" => "IA",
        "Kansas" => "KS",
        "Kentucky" => "KY",
        "Louisiana" => "LA",
        "Maine" => "ME",
        "Maryland" => "MD",
        "Massachusetts" => "MA",
        "Michigan" => "MI",
        "Minnesota" => "MN",
        "Mississippi" => "MS",
        "Missouri" => "MO",
        "Montana" => "MT",
        "Nebraska" => "NE",
        "Nevada" => "NV",
        "New Hampshire" => "NH",
        "New Jersey" => "NJ",
        "New Mexico" => "NM",
        "New York" => "NY",
        "North Carolina" => "NC",
        "North Dakota" => "ND",
        "Ohio" => "OH",
        "Oklahoma" => "OK",
        "Oregon" => "OR",
        "Pennsylvania" => "PA",
        "Rhode Island" => "RI",
        "South Carolina" => "SC",
        "South Dakota" => "SD",
        "Tennessee" => "TN",
        "Texas" => "TX",
        "Utah" => "UT",
        "Vermont" => "VT",
        "Virginia" => "VA",
        "Washington" => "WA",
        "West Virginia" => "WV",
        "Wisconsin" => "WI",
        "Wyoming" => "WY",
        "American Samoa" => "AS",
        "District of Columbia" => "DC",
        "Federated States of Micronesia" => "FM",
        "Guam" => "GU",
        "Marshall Islands" => "MH",
        "Northern Mariana Islands" => "MP",
        "Palau" => "PW",
        "Puerto Rico" => "PR",
        "Virgin Islands" => "VI",
        "Armed Forces Africa" => "AE",
        "Armed Forces Americas" => "AA",
        "Armed Forces Canada" => "AE",
        "Armed Forces Europe" => "AE",
        "Armed Forces Middle East" => "AE",
        "Armed Forces Pacific" => "AP"
    );
    $output = "";
    if($style === true){
        ksort($states);
    }

    $output = "<select name='state'>";
    $output .= "<option value=''>--</option>";

    foreach($states as $name => $abbr){
        $output .= "<option value='" . $abbr . "'";
        if($form === $abbr){
            $output .= "selected='selected'";
        }

        if($style === true){
            $output .= ">" . $abbr . "</option>";
        } else {
            $output .= ">" . $name . "</option>";
        }
    }
    $output .= "</select>";

    return $output;
}

To output the dropdown, simply do:

echo statesDropdown();


Structure and readability of basic calculator I created this code to make a basic calculator after viewing a few examples. I added the function to continue the calculations myself. I am wondering if there is a way to make this more efficient as I feel I've used some unnecessary code but I can't make it work without.

I also was looking for feedback on my codes readability and how to improve if it is of a poor quality.

#include <stdio.h>

int main ()
{
int cancel = 1;
char o, end  = 'Y';
float numone = 0.0, numtwo = 0.0;

         /* Explaining structure and receiving inputs */

         printf("Please enter the two numbers you wish to manipulate!\n");
         scanf("%f", &numone);

         do {
                 scanf("%f", &numtwo);
                 printf("Please enter the operation you wish to perform (+, -, *, /)\n");
                 scanf(" %c", &o);

                 /* Checking for correct input and manipulating numbers with regards to user input */

                 switch(o){

                         case '+':
                                 printf("%.1f + %.1f = %.1f\n", numone, numtwo, numone + numtwo);
                                 break;
                         case '-':
                                 printf("%.1f - %.1f = %.1f\n", numone, numtwo, numone - numtwo);
                                 break;
                         case '*':
                                 printf("%.1f * %.1f = %.1f\n", numone, numtwo, numone * numtwo);
                                 break;
                         case '/':
                                 printf("%.1f / %.1f = %.1f\n", numone, numtwo, numone / numtwo);
                                 break;
                         default:
                                 /* Error message for incorrect input */
                                 printf("Error! Incorrect input!\n");
                                 break;
                 }`enter code here`
                 /* Option to continue calculations */

                 printf("Would you like to perform a further calculation? (Y/N)\n");

                 /* Checking for answer and terminating if N, resuming if Y */

                 scanf(" %c%", &end);
                        if (end == 'N') {
                         return 0;
                        }
                        else {}
                 numtwo = 0;

                 /* Requesting new number to use as numtwo for calculations */

                 printf("Current number = %f please input a second number!\n", numone);
                 } while (end == 'Y');

         return 0;
}

Minor stuff:

I'm getting several compiler warnings. Capture the returned variable
    from scanf() to check if the user actually entered a number.

main.cpp:17: warning: ignoring return value of 'int scanf(const char*, ...)', declared with attribute warn_unused_result [-Wunused-result]
         scanf("%f", &numtwo);
                             ^

Your if-else statement needs to be fixed.

else {
numtwo = 0;
}


Checkered flag: a modular usage of Tkinter's geometry management This is code that produces a checkered flag-like GUI. I wanted to demonstrate, also learn in detail, how tkinter's geometry managers can be used modularly, so I decided that I could write code for a checkered flag that has frames inside frames, that which have their geometries managed independent of each other.

The code produces the following:

The main window(root) has a frame that has 2 vertical frames as partitions (class TwoFrames).
Each one of those frames has 2 horizontal labels as partitions (class TwoLabels).

I wanted to show that you can manage a frame's geometry which manages its own widgets' geometry.

import tkinter as tk

#a class that has 2 columns of frames inside
class TwoFrames(tk.Frame):
    def __init__(self, master=None):
        super().__init__(master)

        #creates 2 frame objects and passes self as parent, which means object created using TwoFrames class is the parent
        self.frame1 = tk.Frame(self)
        self.frame2 = tk.Frame(self)

        #manages 2 frames geometry
        self.frame1.grid(row=0, column=0, sticky="nsew")
        self.frame2.grid(row=0, column=1, sticky="nsew")

        #enables resizing for 0th row, and 1st and 2nd columns of an object of this class
        self.rowconfigure(0, weight=1)
        self.columnconfigure(0, weight=1)
        self.columnconfigure(1, weight=1)

class TwoLabels(tk.Frame):
    def __init__(self, master=None, color=True):
        super().__init__(master)

        #creates 2 Label objects with TwoLabels object as parent
        self.label1 = tk.Label(self)
        self.label2 = tk.Label(self)

        #configures the background color of labels for demonstrative purposes
        if color:
            #label1 will have black color
            self.label1.configure(bg="black")
            #label2 will have white color
            self.label2.configure(bg="white")
        else:
            #label1 will have white color
            self.label1.configure(bg="white")
            #label2 will have black color
            self.label2.configure(bg="black")

        #manages the geometry
        self.label1.grid(column=0, row=0, sticky="nsew")
        self.label2.grid(column=0, row=1, sticky="nsew")

        #enables resizing like above, but this time for 2 rows and 1 column
        self.rowconfigure(0, weight=1)
        self.rowconfigure(1, weight=1)
        self.columnconfigure(0, weight=1)

#creates the mainWindow
mainWindow = tk.Tk()

#creates a mainFrame that has 2 frames in it
mainFrame = TwoFrames(mainWindow)

#manages geometry of mainFrame and display it
mainFrame.pack(fill="both", expand=True)

#creates row_labels1 and row_labels2, both has 2 colored labels in them
row_labels1 = TwoLabels(mainFrame.frame1, True)
row_labels2 = TwoLabels(mainFrame.frame2, False)

#manages geometry of labels inside frames and displays them
row_labels1.pack(fill="both", expand=True)
row_labels2.pack(fill="both", expand=True)

#run the application
mainWindow.mainloop()

Review Concerns:

My main concern is to have this code well written in the structure of object oriented programming. Tiniest of issue is important to me.
I'd also like to know in what ways it can be more concise for better understanding tkinter's geometry managers.

OOP is not merely creating classes: it is also about a thoughtful design. This becomes even more true when it comes to tkiner. I mean you created 2 separate classes to refer to tk.Label() and tk.Frame() classes. This is not the best option for your context as that design is misleading in that it makes the reader of your code think you are trying to create custom Label() and Frame() widgets (which is not what the application actually does).
Also, from the OOP perspective, the lines of code spanning from mainWindow = tk.Tk() to the end do not comply with the OOP philosophy (they are mere sequential instructions not even wrapped within a function)
You say the goal of your application is to play with the tkinter geometry managers. Actually there are 3 of them: place(), pack() and grid(). You are using only one of them. The ideal  is to use the 3 of them, but in my solution below I will use only 2 of them as the context is not suitable for a smart use of place().
You can use pack() for the frames and grid() to manage the labels.
It is better that you attach your widgets to a parent widget instead of the instance of the class you use. Example: instead of coding something like self.frame1 = tk.Frame(self), it would be more appropriate to write self.frame1 = tk.Frame(self.some_more_specific_parent)
There is an easier way to play with the flag (color) you are using as a background color for the label widgets (see my solution below)
It is hard, when launching your application, to see where is the main window and stretch it to make it visible. So for the sake of a better user experience, you should set default dimension to it and keep it resizable (see my solution below)
No need to use the pack() geometry manager the way you did.

Based on the elements stated above, and based on some of Tkinter best practices, I would like to suggest you the solution below which requires, logically, only one class, and refactors your code in a cleaner way:

import tkinter as tk

class SimpleGeometryManagersDemo(tk.Frame):

    def __init__(self, master):
        self.master = master
        super().__init__(self.master)
        self.configure_gui()
        self.create_widgets()

    def configure_gui(self):
        self.master.title('Simple Geometry Managers Demo')
        self.master.geometry('400x200')

    def create_widgets(self):
        self.create_two_frames()
        self.create_two_labels_per_frame()

    def create_two_frames(self):
        self.frame1 = tk.Frame(self.master)
        self.frame2 = tk.Frame(self.master)
        self.frame1.pack(fill=tk.BOTH, expand=1)
        self.frame2.pack(fill=tk.BOTH, expand=1)

    def create_two_labels_per_frame(self):
        self.create_two_labels(self.frame1, 'black')
        self.configure_columns_and_rows(self.frame1)
        self.create_two_labels(self.frame2, 'white')
        self.configure_columns_and_rows(self.frame2)

    def create_two_labels(self, parent, color):
        self.label1 = tk.Label(parent, bg=color)
        bg_color = 'white' if color == 'black' else 'black'
        self.label2 = tk.Label(parent, bg=bg_color)
        self.label1.grid(row=0, column=0, sticky='nswe')
        self.label2.grid(row=0, column=1, sticky='nswe')

    def configure_columns_and_rows(self, parent):
        parent.rowconfigure(0, weight=1)
        parent.columnconfigure(0, weight=1)
        parent.columnconfigure(1, weight=1)

def main():
   root = tk.Tk()
   geometry_managers_demo = SimpleGeometryManagersDemo(root)
   root.mainloop()

if __name__ == '__main__':
   main()


Converting an object into a JSON string I wrote a Python method to convert an object and a subset of its attributes into a JSON string with a given field as the key.

There's a lot of string concatenation, and I'm not too sure what the best way to do that in Python is.

I'm also wondering if attr.replace('"','\"') is sufficient to sanitize input.

def to_json_with_key(nodes, key, fields, insertIndex=False):
    i=0
    s="{"
    for node in nodes:
        s += '"%s":{' % getattr(node,key)
        attrs=[]
        for field in fields:
            attr = getattr(node,field)
            if not isinstance(attr,int) and not isinstance(attr,float) and not isinstance(attr,long):
                attrs.insert(0, '"%s":"%s"' % (field, attr.replace('"','\"') ))
            else:
                attrs.insert(0, '"%s":%s' % (field, attr ))

        if (insertIndex):
            s+='index:%d,' % i
            i=i+1

        s+=','.join(attrs) + "},"
    s = s.rstrip(",") + "}"
    return s

Sample input:

to_json_with_key(myObj, "title", fields=["length","time_offset"], insertIndex=True)

Sample output:

{"Introduction_to_Lists":{index:0,"length":128,"time_offset":0,
 "Pass_by_Reference":{index:1,"length":84,"time_offset":128}}

Firstly, python includes a json module. Use that rather then writing your own.

However, just to give some comments on your style:

def to_json_with_key(nodes, key, fields, insertIndex=False):

python recommend words_with_underscores for parameter names

    i=0

Rather then manually keeping a count use enumerate

    s="{"
    for node in nodes:
        s += '"%s":{' % getattr(node,key)

You don't check the value to make sure it is json-safe. Building strings by concatenation is not usually a good idea. Either build a list and join it or use StringIO

        attrs=[]
        for field in fields:
            attr = getattr(node,field)
            if not isinstance(attr,int) and not isinstance(attr,float) and not isinstance(attr,long):

You can pass several types in a tuple to isinstance to check several types at once.

                attrs.insert(0, '"%s":"%s"' % (field, attr.replace('"','\"') ))

This won't be sufficient. For example, you could have a \ at the end of a string. You also need to handle newlines, tabs, and other escaped characters.

            else:
                attrs.insert(0, '"%s":%s' % (field, attr ))

Why insert rather then append?

        if (insertIndex):

No need for parens

            s+='index:%d,' % i
            i=i+1

        s+=','.join(attrs) + "},"
    s = s.rstrip(",") + "}"

It would be cleaner to join a list of items rather then stripping off stray commas.

    return s


Simple pet caring game - console I'm a beginner programmer and i'm looking for interesting projects to improve my low skills. I tried one day to improve my skills by trying to create a small game in which a user owns a cat, and has to feed it and clean it. My main aim of the game(which yet has many illogical things like the never decreasing happiness and cleanness level) was to try and keep my code as clean and presentable as possible. The game is made in 4 different files, and I will paste them all below. I will be happy if anyone could give me help on understanding where should I improve.
Lets go into the code:
main.cpp
#include "Cat.h"
#include "pet game.h"
#include <iostream>
using namespace std;

int main() {
    //Create the user's profile
    //User();

    //Welcome messages
    cout
            << "Welcome to Unga bonga! Here you buy pets and raise them as your kids, and try to level up, in order to finish the game!"
            << endl;
    showHelp();

    //Command execution
    do {
        commandIdentify();
    } while (true);
}
Cat cat;
//User user;

void showHelp() {
    cout << "The commands in this game are:" << endl
            << "1. to feed your cat, type f" << endl
            << "2. to clean your cat, type c" << endl
            << "3. to check your cat's happiness, type h" << endl
            << "4. to check your cash balance, type m" << endl
            << "5. to check your level status, type l" << endl
            << "6. to display this help message, type ?" << endl
            << "7. to exit, type e" << endl
            << "8. Finally, to play this game, enjoy!" << endl;
}

void commandIdentify() {
    //take input
    cin >> command;
    //identify and execute
    switch (command) {
    case 'f':
        feedCat();
        break;
    case 'c':
        cleanCat();
        break;
        case 'h':
         checkHappiness();
         break;
    case '?':
        showHelp();
        break;
    case 'e':
        exit();
    default:
        cout << "That's not a command -yet (probably)" << endl;
    }
}

void feedCat() {
    cout << "Time for food!!!" << endl;
    cat.feed();
}

void cleanCat() {
    cout << "Let's take a bath!!!" << endl;
    cat.clean();
}

void checkHappiness(){
 cat.checkHappiness();
 }

int exit() {
    exit(0);
    return 0;
}

pet game.h:
    #ifndef PET_GAME_H_
#define PET_GAME_H_

#include <iostream>
#include "Cat.h"
//#include "User.h"

using namespace std;

//Global variables
char command;
int exit();

//functions
void commandIdentify();
void buyNextCat();
void showHelp();
void tutorial();
void feedCat();
void cleanCat();
void checkHappiness();

//functions defined for a future use but they cause problems right now
void checkMoney();
void checkLevel();

#endif /* PET_GAME_H_ */

Cat.cpp:
#include <iostream>
#include <string>
#include "Cat.h"

using namespace std;

Cat::Cat(){
    happiness = 1;
    cleanness = 1;
}

void Cat::feed(){
    cout << "Yummy!!" << endl;
    Cat::happiness = Cat::happiness + 0.2;
}

void Cat::clean(){
    cout << "So soupy!!" << endl;
    Cat::cleanness = Cat::cleanness + 0.2;
}

Cat.h:
#ifndef CAT_H_
#define CAT_H_

#include <iostream>

using namespace std;

class Cat {
private:
    int happiness;
    bool happy = happiness >= 0.6;

    int cleanness;
    bool isClean = cleanness >= 0.6;
public:
    Cat();
    void feed();
    void clean();
    void checkHappiness() {
        cout << "Your cat's happiness is " << happiness*100 << "%" << endl;
    }

};

#endif /* CAT_H_ */

it will be great if you could tell me how to improve code

Avoid using namespace std; - it can cause problems with name collisions. It's best to qualify names where necessary, e.g.: std::cout

std::endl adds a newline, but it also flushes the output stream. Unless we actually need the flushing behavior, it's best to simply output a newline "\n" where necessary (e.g. << "2. to clean your cat, type c\n").

char command; does not need to be a global variable. It's only accessed in a single function, so it can be a local variable.

The exit() function is not necessary. We can call std::exit(0) directly. Note that we should include <cstdlib> for this function, and use the exit codes EXIT_SUCCESS or EXIT_FAILURE instead of a number like 0.

Functions that are used only in a single file do not need to be declared in header files. If necessary, we can declare them above the point of use, or move the function definition higher up the file. e.g.:
void commandIdentify(); // forward declaration

int main() { ... }

void commandIdentify() { ... } // function definition

or
void commandIdentify() { ... } // function definition

int main() { ... }

In the Cat class, the happy and isClean members are not quite correct. They will only be set on initialization. If the cat's happiness or cleanness changes at a later time, they will not be updated. In addition, initialization of variables occurs before the body of the constructor, in the order that they are declared in the class.
For the initial values of happy and isClean to be set correctly, we would have to use a member initializer list:
Cat::Cat():
    happiness(1),
    /* `happy` is initialized here */
    cleanness(1),
    /* `isClean` is initialized here */
{
    // if we set happiness now, `happy` won't get updated!
    happiness = 0.2,
}

In any case, since we want the values to always be updated, we should make happy and isClean member functions instead:
bool isClean() const { return cleanness >= 0.6; }

That way, we don't need to update any extra state.

Member functions that don't modify the state of the class (i.e. any member veriables) should be declared const, e.g.:
void checkHappiness() const { ... }

Cat cat; This doesn't need to be a global variable. We can make the cat a local variable in main() and pass a reference to it to the functions that need to use it:
int main() {
    Cat cat;
    ...
    commandIdentify(cat);
}

void commandIdentify(Cat& cat) {
    ...
    feedCat(cat);
}

void feedCat(Cat& cat) {
    ...
    cat.feed();
}

This may seem like more work, but it's quite important, as means that our commandIdentify function (and the functions for each command) will now work with any cat, not just the one cat that we declared in main().
So now we can feed many cats!


Design pattern: image downloads This is an image representation of my code below:

This is what they do:

CellWithImageX: Subclasses of UITableViewCell. They have an image and an loading indicator. Only 1 can be shown at the time.

ImageRequester: Downloads the images and gives ImageCache a callback when it's done.

FileExplorer: Saves the images.

ImageCache: Manages everything. Since I am most concerned about the current design, I think the code for the ImageCache is only relevant:

// Used to weakly hold the CellWithImageX image classes. (Is this bad?)
open class WeakObjectWrapper<T: AnyObject> {
    public weak var obj: T?

    public init(obj: T) {
        self.obj = obj
    }

}

class ImageCache {

    // I can not understand how it can work if it isn't a singleton.
    // From every cell, I can easily pass the reference without to much hassle.
    // I am not sure for an singleton alternative.
    static let sharedInstance = ImageCache()

    // TODO: Maybe remove some elements from time to time?
    // Yes, this is an array of the images in the table view cell...
    private var loadableImages: [WeakObjectWrapper<LoadableImage>] = []

    private init() {}

    func handleImage(photoIdentifier: Int64, loadableImage: LoadableImage) {
        // Bit concenered about a race condition.
        // I read about DispatchQueues and Semaphores, can't really
        // decide what would be the best way.
        assert(Thread.isMainThread)
        // LoadableImage has an identifier which we can use to later on match the downloaded image.
        loadableImage.identifier = photoIdentifier

        guard photoIdentifier != 0 else {
            loadableImage.show(image: #imageLiteral(resourceName: "anonymous"))

            return
        }

        guard let image = determineImage(photoIdentifier: photoIdentifier) else {
            loadableImages.append(WeakObjectWrapper(obj: loadableImage))
            loadableImage.showIndicator()

            return
        }

        loadableImage.show(image: image)
    }

    func received(image: UIImage, forPhotoIdentifier: Int64) {
        assert(Thread.isMainThread)

        FileExplorer.sharedInstance.save(image: image.pngData()!, photoIdentifier: forPhotoIdentifier) // This is validated that this works, we can safely force unwrap.

        for loadableImage in loadableImages.filter({ $0.obj?.identifier == forPhotoIdentifier }) {
            // While iterating it may occur that the reference is gone.
            loadableImage.obj?.show(image: image)
        }
    }

    private func determineImage(photoIdentifier: Int64) -> UIImage? {
        guard let image = FileExplorer.sharedInstance.determineImage(photoIdentifier: photoIdentifier) else {
            ImageRequester.sharedInstance.download(photoIdentifier: photoIdentifier)

            return nil
        }

        return image
    }
}

In my cellForRowAt method in my tableView, I am calling the ImageCache handleImage method on my cell.loadableImage property.

My concerns:

I am using a few singletons: ImageRequester, ImageCache and
FileExplorer. I am not sure if this is the best approach, but I can
not imagine a better way because this way, all my cells can easily
register themselfs.
I have a array of weak objects which references
to the loadable images inside of a tableViewCell. Isn't there a
better way?
I am asserting I am on the main thread. If I am on multiple threads, race conditions may occur while I loop through the array. I looked into DispatchQueues and Semaphores. I just didn't found the right way to do it, and what the best way would be in my case. Does someone has a suggestion about what way would be a good way in my case?

The way I see it your requirements can be summarized as:

Have a central repository of images.
Load Images from a some data source.
Save Persist Images.
Give a cell its appropriate image based on an identifier.

The main issue I see is that the cells are tightly coupled to the ImageCache object, it might be better to have some sort of ViewModel or controller layer sit between your image cache and cells. This model or controller would contain the information needed to retrieve an image for image cache along with the information needed to display the image to the cell. This would make your code much more testable and decouple your UITableViewCells from the the your singleton cache.

On whether a singleton is necessary-- I am going to take a guess that this is all for one ViewController. Why not just initialize the ImageCache object in that ViewController? If this is truly used across many ViewControllers than I could maybe be convinced to get behind the use of a singleton.

On the WeakReference array... that is valid and actually relatively advanced pattern in Swift, I'm not sure that you really need it here though although its a little hard to tell without more context. It appears LoadableImage is some sort of object that encapsulates a callback that displays the image or an indicator on the UI... in which case yes you would need a weak reference, although I am unsure as to why you even need to store the LoadableImages in an array to begin with. I think that you could abstract away the array if you refactored to a more traditional Callback oriented approach.

In a callback approach the TableViewCells would merely request the appropriate image and show the loading indicator until the callback was called. If you really needed to cache the images, you could do so in an array within the cache.

I wish that your method names were more descriptive in Swift style... for example determineImage(for photoId:String). That's part of the expressiveness and readability of the language. Another example the function received doesn't really express what it's use is I think that its some sort of callback that will display an image once it has been loaded and display that image to a specific LoadableImage, but this needs to be more clear.

I think to give you more advice you need to post more code, along with a description of what the requirements are. For example: How many ViewControllers are using the Cache? Do you need to store images across the lifecycle of multiple ViewControllers? How are you planning on linking the ImageIdentifiers to the TableViewCells.

If you post a few more details, I will follow up with a few code samples, but as it is I don't see how I can give you any tangible direction without knowing a little bit more.


Rearranging numbers to get the largest number Given a Integer, find the maximum number that can be formed from the digits.

Input: 8754365
Output: 8765543

private static int largestNumber(int data) {
    int num = data;
    int[] times = new int[10];
    while (num != 0) {
        if (num == 0) {
            break;
        }
        int val = num % 10;
        times[val]++;
        num /= 10;
    }
    String largestNumber = "";
    for (int i = 9; i >= 0; i--) {
        for (int j = 0; j < times[i]; j++) {
            largestNumber += i;
        }
    }
    return Integer.parseInt(largestNumber);
}

Is there any optimization or anything I can improve here?

while (num != 0) {
    if (num == 0) {
        break;
    }

That if is redundant and can be removed.

Using a StringBuilder for largestNumber is roughly twice as fast on my machine.

Avoiding strings altogether is faster still:

int largestNumber = 0;
for (int i = 9; i >= 0; i--) {
  for (int j = 0; j < times[i]; j++) {
    largestNumber = largestNumber * 10 + i;
  }
}
return largestNumber;

You might want to return a long instead of an int so that, for example, largestNumber(Integer.MAX_VALUE) returns 8776444321, as expected.


C++ string parser for wpa_supplicant I have created a naive string parser for unsolicited wpa_supplicant messages.  I could potentially extend this in the future.  I am not sure if this is the best way to parse these kinds of messages.  I would prefer something resembling a switch statement but I am not sure I could do that without increasing code complexity.  I would like a more scalable solution though, in case I need to look for many different message types in the future.

Code:

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

#define DEBUG_PRINT printf

class wpa_monitor
{
    public:
        void handle_wpa_message( const std::string& message );

    private:
        std::map< std::string, std::string > mac_ssid_association_map;
};

void wpa_monitor::handle_wpa_message( const std::string& message )
{
    DEBUG_PRINT( "WPA event: '%s'\n", message.c_str() );
    size_t pos;

    pos = message.find( "Trying to associate with " );

    if( pos != std::string::npos )
    {
        std::string mac_substring = message.substr( pos + sizeof( "Trying to associate with " ) - 1 );
        std::string mac_addr = mac_substring.substr( 0, mac_substring.find( " " ) );

        if( mac_addr.empty() )
        {
            return;
        }

        pos = message.find( "SSID='" );

        if( pos == std::string::npos )
        {
            return;
        }

        std::string ssid_substring = message.substr( pos + sizeof( "SSID='" ) - 1 );
        std::string ssid = ssid_substring.substr( 0, ssid_substring.find( "'" ) );

        DEBUG_PRINT( "Associating mac_address '%s' with ssid '%s'\n", mac_addr.c_str(), ssid.c_str() );
        mac_ssid_association_map.insert( std::make_pair< std::string, std::string >( mac_addr, ssid ) );

        return;
    }

    pos = message.find( "CTRL-EVENT-CONNECTED - Connection to " );

    if( pos != std::string::npos )
    {
        std::string mac_substring = message.substr( pos + sizeof( "CTRL-EVENT-CONNECTED - Connection to " ) - 1 );
        std::string mac_addr = mac_substring.substr( 0, mac_substring.find( " " ) );

        if( mac_addr.empty() )
        {
            return;
        }

        std::map< std::string, std::string >::iterator it = mac_ssid_association_map.find( mac_addr );

        if( it != mac_ssid_association_map.end() )
        {
            DEBUG_PRINT( "We are now connected to ssid '%s'\n", it->second.c_str() );
        }

        return;
    }

    pos = message.find( "CTRL-EVENT-SCAN-RESULTS" );

    if( pos != std::string::npos )
    {
        //scan results ready
        DEBUG_PRINT( "Scan results are ready\n" );
        return;
    }
}

Example Output (personal data obfuscated):

WPA event: '<3>CTRL-EVENT-SCAN-STARTED '
WPA event: '<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00'
WPA event: '<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01'
WPA event: '<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02'
WPA event: '<3>CTRL-EVENT-SCAN-RESULTS '
Scan results are ready
WPA event: '<3>WPS-AP-AVAILABLE '
WPA event: '<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)'
Associating mac_address '00:00:00:00:00:00' with ssid 'my_ssid'
WPA event: '<3>Associated with 00:00:00:00:00:00'
WPA event: '<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0'
WPA event: '<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]'
WPA event: '<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]'
We are now connected to ssid 'my_ssid'

I'll make no comment on the use of C-style <stdio.h>, since that's likely an artefact of the surrounding code, rather than something you'd have written yourself.

A missing header and a typo/oversight

The header required for std::make_pair is <utility>.  Your implementation may have defined it as a side-effect of including <map>, but it's not portable to rely on that.

size_t pos ought to be std::size_t pos.

Test program

I managed to re-create some input from your sample output:

int main()
{
    static auto const messages = {
        "<3>CTRL-EVENT-SCAN-STARTED ",
        "<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00",
        "<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01",
        "<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02",
        "<3>CTRL-EVENT-SCAN-RESULTS ",
        "<3>WPS-AP-AVAILABLE ",
        "<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)",
        "<3>Associated with 00:00:00:00:00:00",
        "<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0",
        "<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]",
        "<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]",
    };

    wpa_monitor monitor;

    for (auto m: messages)
        monitor.handle_wpa_message(m);
}

Naming

I avoid names that include their type, as it can be confusing if you later change that type.  So I'd call the map mac_to_ssid instead.  That also has the benefit of being shorter, so easy to type and to read.

Extensibility

You'll find that a single large method gets harder to navigate as it gets bigger.  As hinted in another answer, it makes sense to delegate to a bunch of simple methods that each handle a single case.  You could iterate through a vector, but I'd stick to something simple like this:

class wpa_monitor
{
public:
    bool handle_wpa_message(const std::string& message);

private:
    std::map<std::string, std::string> mac_ssid_association_map = {};

    bool handle_associate(const std::string& message);
    bool handle_connected(const std::string& message);
    bool handle_scanresult(const std::string& message);
};

bool wpa_monitor::handle_wpa_message(const std::string& message)
{
    DEBUG_PRINT("WPA event: '%s'\n", message.c_str());

    return handle_associate(message)
        || handle_connected(message)
        || handle_scanresult(message);
}

Now we can look at each matcher in turn.

Refactor some common code

We repeatedly search for a string and take an interest in its end, like this:

auto pos = message.find("Trying to associate with ");
if (pos == std::string::npos)
    return false;

std::string mac_substring = message.substr(pos + sizeof "Trying to associate with " - 1);

Obviously there's scope for error if we don't keep the two copies of the string in agreement.  Instead, we can write a small function to do this:

static std::size_t find_end(const std::string& s, const std::string& substr)
{
    auto pos = s.find(substr);
    if (pos != std::string::npos)
        pos += substr.size();
    return pos;
}

That makes the caller simpler and more robust:

auto pos = find_end(message, "Trying to associate with ");
if (pos == std::string::npos)
    return false;

auto const mac_substring = message.substr(pos);

Actually, that's not really the MAC substring, as it continues to the end of the string.  We really want another helper, accepting a termination character:

static std::string find_in_delimiters(const std::string& s,
                                      const std::string& open, const char close)
{
    auto start = s.find(open);
    if (start == std::string::npos)
        return {};
    start += open.size();

    auto end = s.find(close, start);
    if (end == std::string::npos)
        return {};

    return s.substr(start, end-start);
}

And we simplify the callers even more:

    auto mac_addr = find_in_delimiters(message, "Trying to associate with ", ' ');
    if (mac_addr.empty()) {
        return false;
    }

More on make_pair()

I get a failure to compile, as GCC seems to want rvalue references for make_pair<string,string>().  I could resolve that with a std::move() like this:

    mac_ssid_association_map.insert( std::make_pair< std::string, std::string >( std::move(mac_addr), std::move(ssid) ) );

However, we can let template type deduction do its thing:

    mac_ssid_association_map.insert(std::make_pair(std::move(mac_addr), std::move(ssid)));

Or even just:

    mac_ssid_association_map.insert({std::move(mac_addr), std::move(ssid)});

Use auto instead of exact iterator types

Here's a place where changing the map type could be a maintenance headache:

    std::map< std::string, std::string >::iterator it = mac_ssid_association_map.find( mac_addr );

We can use auto, which also makes it more readable:

    auto it = mac_ssid_association_map.find(mac_addr);

My version

#include <cstdio>
#include <string>
#include <map>
#include <utility>

#define DEBUG_PRINT std::printf

class wpa_monitor
{
public:
    bool handle_wpa_message(const std::string& message);

private:
    std::map<std::string, std::string> mac_to_ssid = {};

    bool handle_associate(const std::string& message);
    bool handle_connected(const std::string& message);
    bool handle_scanresult(const std::string& message);
};

// Helper method - returns substring of s contained between
// open and close, or empty if not matched
static std::string find_in_delimiters(const std::string& s,
                                      const std::string& open, const char close)
{
    auto start = s.find(open);
    if (start == s.npos)
        return {};
    start += open.size();

    auto end = s.find(close, start);
    if (end == s.npos)
        return {};

    return s.substr(start, end-start);
}

// Handlers for specific message types

bool wpa_monitor::handle_associate(const std::string& message)
{
    auto mac_addr = find_in_delimiters(message, "Trying to associate with ", ' ');
    if (mac_addr.empty())
        return false;

    auto ssid = find_in_delimiters(message, "SSID='", '\'');
    if (ssid.empty())
        return false;

    DEBUG_PRINT("Associating mac_address '%s' with ssid '%s'\n", mac_addr.c_str(), ssid.c_str());
    mac_to_ssid.insert({std::move(mac_addr), std::move(ssid)});
    return true;
}

bool wpa_monitor::handle_connected(const std::string& message)
{
    auto mac_addr = find_in_delimiters(message, "CTRL-EVENT-CONNECTED - Connection to ", ' ');
    if (mac_addr.empty())
        return false;

    auto it = mac_to_ssid.find(mac_addr);
    if (it != mac_to_ssid.end()) {
        DEBUG_PRINT("We are now connected to ssid '%s'\n", it->second.c_str());
    }

    return true;
}

bool wpa_monitor::handle_scanresult(const std::string& message)
{
    auto pos = message.find("CTRL-EVENT-SCAN-RESULTS");
    if (pos == message.npos)
        return false;

    //scan results ready
    DEBUG_PRINT("Scan results are ready\n");
    return true;
}

// Test program

int main()
{
    static auto const messages = {
        "<3>CTRL-EVENT-SCAN-STARTED ",
        "<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00",
        "<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01",
        "<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02",
        "<3>CTRL-EVENT-SCAN-RESULTS ",
        "<3>WPS-AP-AVAILABLE ",
        "<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)",
        "<3>Associated with 00:00:00:00:00:00",
        "<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0",
        "<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]",
        "<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]",
    };

    wpa_monitor monitor;

    for (auto m: messages)
        monitor.handle_wpa_message(m);
}


Codeigniter active record I understand that I should use prepared statements to prevent SQL injection. Unfortunately, having read the PHP docs on the matter I am none the wiser. Here is one of my simple active record SQL queries in a CI2 model, could someone show me an example of how I might turn this into a prepared statement - do I even need to?

function get_item($id){
        $this->db
            ->select('*')
            ->from('item_entries')
            ->where('item_entries.item_id', $id)
            ->join('item_categories_rel', 'item_categories_rel.item_id = item_entries.item_id');
        $query = $this->db->get();
        if(is_object($query)){return $query->result_array();}else{return $query;}
    }

Am I correct in thinking prepared statements are only necessary if I am accepting user data - if so lets assume $id is user submitted. Although it is not, I am about to write some form to db statements, so advice in preparation for this is appreciated.

I'm not really sure if this question is on topic, but I just started using SQL myself, so here's a chance for me to apply what I've learned. The following is using PDO.

//unspecified placeholder
$sql = "SELECT * FROM item_entries WHERE item_id=?";
$sth = $dbh->prepare( $sql );
$sth->execute( array( $id ) );

//specific placeholder
$sql = "SELECT * FROM item_entries WHERE item_id=:id";
$sth = $dbh->prepare( $sql );
$sth->execute( array( ':id' => $id ) );

The idea being that the SQL statements don't have values directly injected into them by using placeholders. In the first example that placeholder was unspecified ?. You can use any number of unspecified placeholders, but then the array you pass in to the execute() function has to be in the same order. While this may not be a big deal for one input, it could quickly become tedious trying to keep track of multiples. Which is where the second example comes in. It uses specific placeholders :id to define keys in the execution array. This means these elements can be in any order so long as the correct key is associated with the correct input.

As for if this is only necessary for user data? I don't know, but I'm just as prone to make mistakes as users, so it couldn't hurt. I'm quite interested to here if I got this right as well.


Coding Challenge Solution - Good Range This my solution to this Good Range Coding Challenge

There is a number space given from 1 to N. And there are M queries followed by that. In each query, we were given a number between 1 to N (both inclusive). We add these number one by one into a set.
Good range: A range in which there is exactly one element present from the set.
For each query, we need to find the good ranges. We need to return the sum of boundry of all good ranges.
Input:
First line will take two integer for input N and M.
Then following m lines would be numbers between 1 and N (both inclusive).
Output:
Following M lines contains sum of boudaries of good ranges.
Note:
Range can consist of single element and represented as (x-x) where boundary sum will be x+x.
Example:
Input:

10 4
2
5
7
9

Output:

11
18
30
46

Explaination:

step-1) set: 2
good range: (1-10)
sum: 1+10=11

step-2) set: 2 5
good range: (1-4), (3-10)
sum: 1+4+3+10=18

step-3) set: 2 5 7
good range: (1-4), (3-6), (6-10)
sum: 1+4+3+6+6+10=30

step-4) set: 2 5 7 9
good range: (1-4), (3-6), (6-8), (8-10)
sum: 1+4+3+6+6+8+8+10=46

#include <iostream>
#include <set>
using namespace std;

class Solution
{
    public:
    Solution(const unsigned int _N, const unsigned int _M) : N(_N), M(_M) {}

    void solve()
    {
        for(unsigned int i=0; i<M; ++i)
        {
            unsigned int v;
            cin >> v;
            cout << "New Element = " << v << endl;
            q.insert(v);
            print_res();
            cout << endl;
        }
    }

    void print_res()
    {
        unsigned int left=1;
        auto it=q.begin();
        unsigned int last = *it;

        for(++it; it!=q.end(); ++it)
        {
            const unsigned int curr = *it;
            const unsigned int right = curr-1;
            cout << "[" << left << ", "<< right << "] contains " << last << " and sum = " << (left+right) << endl;
            left = last+1;
            last = curr;
        }
        const unsigned right = N;
        cout << "[" << left << ", "<< right << "] contains " << last << " and sum = " << (left+right) << endl;
    }

    private:
    unsigned int N;
    unsigned int M;
    set<unsigned int> q;
};

int main() {
    // your code goes here
    unsigned int N=0;
    unsigned int M=0;
    cin >> N >> M;
    Solution sol(N,M);
    sol.solve();
    return 0;
}

Note: I am aware I'm returning more information than required by the problem description but I have chosen to do it to include also debugging information

Don't force a class where a function is perfectly fine.
The namespace std is not designed for wholesale importation, see "Why is “using namespace std” considered bad practice?" for more detail.
Identifiers starting with an underscore followed by an uppercase letter are reserved.
Input is generally unreliable. But you don't test for error at all.
Always recomputing the current result from first principles, instead of just updating it with the newest addition, is a waste of time.
Flushing is expensive, so avoid std::endl and explicitly request it with std::flush where unavoidable.
You should probably follow the requested output-format...
You know that int in unsigned int is implicit?
In C++ and C99+, return 0; is implicit for main().
I wonder why you sometimes surround binary operators with space, and sometimes don't.
Having over-long lines is very cumbersome to read. Admittedly, you need not make a hard cut at 79 nowadays.
You should double-check whether duplicate inputs can occurr (there are none in the example), and if so whether they should cause anything but repeated output.
I assumed duplicated possible, and they only cause output.

Duplicates have an effect would destroy ranges instead of adding them, thus needing an ordered std::set.
Duplicates not possible would mean the std::unordered_set can be dispensed with.

#include <cstdlib>
#include <iostream>
#include <unordered_set>

int main() {
    unsigned n, m, x;
    std::cin >> n >> m;
    if (!std::cin || !n) {
        std::cerr << "Bad Input!\n";
        return EXIT_FAILURE;
    }
    std::unordered_set<unsigned> nums;
    auto r = 0ULL;
    while (--m) {
        std::cin >> x;
        if (!std::cin || x < 1 || x > n) {
            std::cerr << "Bad Input!\n";
            return EXIT_FAILURE;
        }
        auto [iter, created] = nums.insert(x);
        if (created)
            r += 2 * x;
        std::cout << r << '\n';
    }
}


Wild and changing business rules implemented with functional programming I attempted the Business Rules Kata. Here's a video overview.

However, I am not confident that going functional is a good strategy for the following objective:

  How can you tame these wild business rules? How can you build a system
  that will be flexible enough to handle both the complexity and the
  need for change? And how can you do it without condemning yourself to
  years and years of mindless support?

Is there an alternative FP approach that satisfies the objective stated above?

module PaymentSystem

(*Types*)
type ProductId =          ProductId of string
type MemberId =           MemberId  of string
type Email =              Email     of string
type Agent =              Agent
type RoyaltyDepartment =  RoyaltyDepartment

type PackingSlip = {
    MemberId:MemberId
    ProductId:ProductId
}

type PhysicalProducts =
    | Book
    | Video
    | Other

type MembershipType =
    | Membership of MemberId
    | Upgrade    of MemberId

type PaymentFor =
    | PhysicalProduct of PhysicalProducts * PackingSlip
    | Membership      of MembershipType

type PackingSlipOptions =
    | PackingSlip       of PackingSlip
    | DuplicateSlips    of PackingSlip
    | WithFirstAidVideo of PackingSlip

type PaymentResponse =
    | PackingSlip        of PackingSlipOptions
    | ActivateMembership of MemberId
    | UpgradeMembership  of MemberId
    | EmailOwner         of MembershipType
    | CommissionPayment  of Agent

(*Functions*)

let publish payload = ()       // Stub
let getAgent productId = Agent // Stub

let respondTo (payment:PaymentFor) =

    match payment with
    | PhysicalProduct     (kind , packingSlip) ->

        publish (CommissionPayment (getAgent packingSlip.ProductId))

        match kind with
        | Book  -> publish (DuplicateSlips    packingSlip)
        | Video -> publish (WithFirstAidVideo packingSlip)
        | Other -> publish packingSlip

    | Membership kind ->

        publish(EmailOwner kind)

        match kind with
        | MembershipType.Membership memberId -> publish(ActivateMembership memberId)
        | MembershipType.Upgrade    memberId -> publish(UpgradeMembership  memberId)

The original requirement

  How can you tame these wild business rules? How can you build a system that will be flexible enough to handle both the complexity and the need for change? And how can you do it without condemning yourself to years and years of mindless support?

seems, to me, to insinuate some sort of generic rules engine. This is, I believe, a red herring. The problem with rules engines, in my experience, is that the selling point is always that 'business users' can maintain them without involving developers.

This is a pipe dream, because if business rules can be arbitrarily complex, you're going to need a rules engine that can handle all of this complexity. In other words, you're going to need a programming language, and then it becomes too technical for business users.

This is called the inner platform effect and is, in my opinion, best avoided.

So you might as well use a programming language to implement the rules, and I see no reason you can't use a functional language like F#.

Here's a sketch of some of the rules. Like in the real world, some of the rules are vaguely defined, so I wasn't sure how to interpret them...

type Membership = Basic | Gold

type Good =
| PhysicalProduct of string
| Book of string
| Video of string
| Membership of Membership
| Upgrade

type Command =
| Slip of string * (Good list)
| Activate of Membership
| Upgrade
| PayAgent

These are simply some types, but given these types, you can implement various business rules, e.g.

// Good -> Command list
let slipForShipping = function
| PhysicalProduct name
| Book name
| Video name -> [Slip ("Shipping", [PhysicalProduct name])]
| _ -> []

// Good -> Command list
let slipForRoyalty = function
| Book name -> [Slip ("Royalty", [Book name])]
| _ -> []

// Good -> Command list
let activate = function | Membership x -> [Activate x] | _ -> []

// Good -> Command list
let upgrade = function | Good.Upgrade -> [Upgrade] | _ -> []

You'll notice that all these rules have the same type, which means that you can collect all of them:

// ('a -> 'b list) list -> 'a -> 'b list
let handle handlers good = handlers |> List.collect (fun h -> h good)

// Good -> Command list
let handleAll = handle [slipForShipping; slipForRoyalty; activate; upgrade]

Example:

> handleAll (Book "The Annotated Turing");;
val it : Command list =
  [Slip ("Shipping",[PhysicalProduct "The Annotated Turing"]);
   Slip ("Royalty",[Book "The Annotated Turing"])]

As far as I can tell, this would be fairly maintainable, because if you need to add a new rule, you'll have to add a new function, and add that function to handleAll.


Obtaining a target number only using the operations ×2, ×3, and +1 Problem Introduction

  You are given a primitive calculator that can perform the following
  three operations with the current number x: multiply x by 2, multiply
  x by 3, or add 1 to x. Your goal is given a positive integer n, find
  the minimum number of operations needed to obtain the number n
  starting from the number 1.

  Task: Given an integer n, compute the minimum number of operations needed to obtain the number n starting from the number 1.

  Input Format: The input consists of a single integer \$1 <=n <= 10^6.\$

  Output: Print the sequence of intermediate numbers. For example for input \$ n=96234\$, output is 1 3 9 10 11 22 66 198 594 1782 5346
  16038 16039 32078 96234 or 1 3 9 10 11 33 99 297 891 2673 8019 16038
  16039 48117 96234. Both are valid output.

Now I have designed an algorithm and am finding intermediate numbers for each and every number between 1 and n. It is giving me time limit exceeded error. How can I improve its efficiency?

#include <iostream>
#include <climits>
#include <vector>
#include <list>
void primitive_calculator(int64_t number)
{
        std::vector<int64_t> min_steps(number+1,INT_MAX);
        std::list<int64_t>* path=new std::list<int64_t>[number+1];
        min_steps[0]=0; min_steps[1]=0;
        path[0].push_back(0);
        path[1].push_back(1);
        for(int i=2;i<=number;i++)
        {
            if(i%3==0)
            {
                if(min_steps[i/3] < min_steps[i])
                {
                        min_steps[i]=min_steps[i/3]+1;
                        path[i]=path[i/3];
                        path[i].push_back(i);
                }
            }
            if(i%2==0)
            {
                if( min_steps[i/2] < min_steps[i])
                {
                        min_steps[i]=min_steps[i/2]+1;
                        path[i]=path[i/2];
                        path[i].push_back(i);
                }
            }
            if( min_steps[i-1] < min_steps[i])
            {
                min_steps[i]=min_steps[i-1]+1;
                path[i]=path[i-1];
                path[i].push_back(i);
            }
    }
    std::cout<<min_steps[number]<<"\n";
    while(!path[number].empty())
    {
            std::cout<<path[number].front()<<" ";
            path[number].pop_front();
    }
}
int main()
{
    int64_t number;
    std::cin>>number;
    primitive_calculator(number);
    return 0;
}

For each i = 0...number, your method stores a std::list containing
the complete sequence of intermediate numbers from 1 to i. Each list is created by
copying the list from the optimal predecessor and appending i.
Even for a single i, path[i] may be assigned a new list up to
three times.

That is a lot of copying which consumes a lot of time.
It can be improved by choosing a different data structure:
For each number, store only the "optimal predecessor".
This requires less memory and less copying.
 For the final number,
the sequence of intermediate numbers can then be reconstructed by traversing the predecessors from number to 1, and printing
the numbers in reverse order.

More remarks:

The use of integer types is not consistent. You use int64_t
at most places, but int i as the loop variable, and
INT_MAX as initial value. –
For n in the given range 1...10^6, int32_t is sufficiently large.
The initialization of all elements in min_steps to INT_MAX
can be avoided by testing i - 1 as predecessor first.
Coding style: There should be more space around operators
and keywords.

The function then would look like this:

void primitive_calculator(int32_t number)
{
    std::vector<int32_t> min_steps(number + 1);
    std::vector<int32_t> predecessor(number + 1);

    for (int32_t i = 2; i <= number; i++) {
        min_steps[i] = min_steps[i-1] + 1;
        predecessor[i] = i - 1;
        if (i % 3 == 0) {
            if (min_steps[i/3] < min_steps[i]) {
                min_steps[i] = min_steps[i/3] + 1;
                predecessor[i] = i/3;
            }
        }
        if (i % 2 == 0) {
            if (min_steps[i/2] < min_steps[i]) {
                min_steps[i] = min_steps[i/2] + 1;
                predecessor[i] = i/2;
            }
        }
    }

    std::cout << min_steps[number] << "\n";

    std::list<int32_t> sequence;
    for (int32_t i = number; i != 0; i = predecessor[i]) {
        sequence.push_front(i);
    }
    for (auto it = sequence.begin(); it != sequence.end(); ++it) {
        std::cout << *it  << " ";
    }
}

For number = 96234, this reduced the execution time
from 0.13 seconds with your code to about 0.001 seconds.

The measurement was done on a MacBook with this simple code:

int32_t number = 96234;
const clock_t begin_time = clock();
primitive_calculator(number);
const clock_t end_time = clock();
std::cout << "\ntime: " << float( end_time - begin_time ) /  CLOCKS_PER_SEC << "\n";


Looking through a directory tree for certain folders I've written a script that recursively goes through a directory tree and looks for folders containing folders with NOT INVOICED in the folder name. If it finds one it prints the name of the containing folder and the names of any folders inside it with NOT INVOICED in the folder name.

The output is:

FOLDER 1
    NOT INVOICED FOLDER 1
    NOT INVOICED FOLDER 2
FOLDER 3
    NOT INVOICED FOLDER 1

The idea is that FOLDER 1 contains a few other folders that don't have NOT INVOICED in their name, FOLDER 2 doesn't contain any folders with NOT INVOICED in their name, and so on.

It works, but the only problem is this is what it looks like:

import os
import re

rootDir = '.'

with open('names.txt', 'w') as f:
        for dirName, subdirList,fileList in os.walk(rootDir):
                for fname in subdirList:
                        if re.match("NOT INVOICED", fname):
                                f.write(dirName + '\n')
                                for i in subdirList:
                                        if re.match("NOT INVOICED", i):
                                                f.write('\t%s\n' % i)
                                        else:
                                                pass
                        else:
                                pass

There's got to be a way to write this without six indentations.

It looks like your script's output will look more like this:

FOLDER 1
    NOT INVOICED FOLDER 1
    NOT INVOICED FOLDER 2
FOLDER 1
    NOT INVOICED FOLDER 1
    NOT INVOICED FOLDER 2
FOLDER 3
    NOT INVOICED FOLDER 1

It happens because for each subdirectory which contains NOT INVOICED you'll print containing directory name and all subdirectories. It can be easily fixed by adding an extra break in the end of the first if.

Now to the question. First of all, PEP 8 recommends using 4 spaces per one ident, neither tabs, nor 8 spaces. Also, you are free to omit the else branch in your ifs - no need in pass and extra lines.

I would separate checking of whether the directory should be printed (with some subdirectories) and the actual printing - that would kill inner for loop. Moreover, it will also make it impossible to output a directory twice:

import os
import re

rootDir = '.'

with open('names.txt', 'w') as f:
    for dirName, subdirList,fileList in os.walk(rootDir):
        shouldPrint = False
        for fname in subdirList:
            if re.match("NOT INVOICED", fname):
                shouldPrint = True
        if shouldPrint:
            f.write(dirName + '\n')
            for i in subdirList:
                if re.match("NOT INVOICED", i):
                    f.write('\t%s\n' % i)

Next things that I'd do are:

Calculate not invoiced subdirectories just once with list-comprehension. That will remove redundant for+if pair and also lower the risk of misspelling one of regexes.
Replace fileList variable with wildcard as it's unused
Run pep8 tool (can be installed with pip install pep8) on the code to make sure it corresponds with PEP 8. It'll report some whitespace issues in the very first for loop.
Make sure that only only style of quotes is used across the code - ' or ".

And now we have:

import os
import re

rootDir = '.'

with open('names.txt', 'w') as f:
    for dirName, subdirList, _ in os.walk(rootDir):
        notInvoiced = [fname for fname
                       in subdirList
                       if re.match('NOT INVOICED', fname)]
        if notInvoiced:
            f.write(dirName + '\n')
            for i in notInvoiced:
                f.write('\t%s\n' % i)

Also, if you're Python 3 user, I'd recommend using str.format() instead of % operator - the latter is kind of deprecated.


Python program to lock and unlock files The code takes srcPaths and actionType as parameters. The former is a string with tab-delimited file paths, while the latter is a string that may have values of "Lock", "Unlock", and "Toggle", which is chosen by the user.

#!/usr/bin/python

import os
import sys
import ntpath
import subprocess

srcPaths = sys.argv[1]
actionType = sys.argv[2]

srcPaths = srcPaths.split("\t")
srcPathsCount = len(srcPaths)

lockCount = unlockCount = 0

def fileIsLocked(srcPath):

    cmd1 = subprocess.Popen(["ls", "-ldO", srcPath], stdout=subprocess.PIPE)
    cmd2 = subprocess.Popen(["awk", "{ print $5 }"], stdin=cmd1.stdout, stdout=subprocess.PIPE)
    cmd1.stdout.close()

    if "uchg" in cmd2.communicate()[0]:
        return True
    else:
        return False

def setLock(flagStr, srcPath):

    global lockCount, unlockCount

    subprocess.call(["chflags", flagStr, srcPath])

    if flagStr == "uchg":
        lockCount += 1
    else:
        unlockCount += 1

for srcPath in srcPaths:

    if actionType == "Lock":

        if not fileIsLocked(srcPath):
            setLock("uchg", srcPath)

    elif actionType == "Unlock":

        if fileIsLocked(srcPath):
            setLock("nouchg", srcPath)

    else:

        if fileIsLocked(srcPath):
            setLock("nouchg", srcPath)
        else:
            setLock("uchg", srcPath)

sys.stdout.write("{}/{}/{}".format(lockCount, unlockCount, srcPathsCount))

The function fileIsLocked(srcPath) checks if the file is locked. It does this by using ls -ldO and awk to get the file flags. If it finds "uchg" there, then the function returns True, and vice versa. The function setLock(flagStr, srcPath) executes the shell script command to lock/unlock the file depending on the conditions, and keeps count of files that have been locked and unlocked.

Naturally, if the user wants to Lock the files, the file paths will be checked for unlockedness, and will proceed to lock those that are unlocked. If files are to be Unlocked, vice versa. If the user wants to Toggle, then if the files are locked, they are unlocked, and vice versa.

I'm not certain that the code is as concise as it needs to be. It seems to me like the code might be shortened or functions might be joined together. But I haven't been able to modify the code and still keep it working as intended.

What do you think could be done to improve the logic of the code?

I have absolutely no idea what the purpose of this code is. Something to do with locking and unlocking files, but why? Your code doesn't say, and neither does your post. It's hard to review code when I have no idea what it is supposed to do.
There are no docstrings. What do these functions do? What arguments should I pass?
Running shell commands and parsing their output is complex and slow. Instead, get the flags for a file by calling os.stat and set the flags for a file by calling os.chflags. Use the constants in the stat module, for example stat.UF_IMMUTABLE for the uchg flag.

To read the flags for a file:

import os, stat
flags = os.stat(filename).st_flags

To determine if a file is immutable, evaluate:

bool(flags & stat.UF_IMMUTABLE)

To set the immutable flag, call:

os.chflags(filename, flags | stat.UF_IMMUTABLE)

To clear the immutable flag, call:

os.chflags(filename, flags & ~stat.UF_IMMUTABLE)

Since you don't say what the purpose of this code is, I can't tell if it is a good implementation or not. If you are planning to use this mechanism for mutual exclusion (e.g. to prevent two instances of a program running at the same time) then it won't work, because it has race conditions (the flags might change between getting them and setting them).


Unit testing code that calls RESTful API I'm writing a library which calls a RESTful API and then performs some work on the data.

I've architected my code like this:

I have a class (let's call it APIRequestor) that initiates a call to the API via an injected HTTPRequestor library (which basically wraps a cURL request). That class, after retrieving the response, will perform some operations on that data and then return an APIResult object to the caller.

In order to test the APIRequestor, I created a HTTPRequestorMock which extends HTTPRequestor that, rather than wrapping a cURL request, it simply reads a result from a file I have on disk that way I can test different scenarios.

Below is the code for the APIRequestor object:

<?php
namespace APITests;

require_once "PHPUnit/Autoload.php";
require_once "../classes/APIRequestor.php";
require_once 'HTTPRequestorMock.php';

class APIRequestor extends \PHPUnit_Framework_TestCase {

    public function setUp() {}
    public function tearDown() {}

    public function getMockedRequestor() {
        $requestor = new \APIClasses\APIRequestor();
        $requestor->HTTPLibrary = new HTTPRequestorMock();
        return $requestor;
    }

    public function testResponseIDsNoResults() {
        $requestor = $this->getMockedRequestor();
        $requestor->HTTPLibrary->overrideFilepath = "./mock_assets/ResponseIDsNoResults.xml";
        $responseIDs = $requestor->getResponseIDsSinceResponseID(0);
        $this->assertTrue(count($responseIDs) === 0, "More than zero results returned.");
    }

}

Is this a good design?
Can I improve this design in order to make future additions more testable?
Is this the common practice for removing a dependency on an external server for the sake of my unit tests?

Yes, it looks like a reasonable design and approach.

At the nit-pick level getMockedRequestor should be private or protected, to document that it is a helper for the tests. Also, if every test is going to be calling it, some people might move that code into setUp(). But that only saves one line ($requestor = $this->getMockedRequestor();) in each unit test, so I'd keep it where it is: I like to avoid using member variables, but that is personal preference.

You could use the phpUnit mock interface, but you don't have to. It gives you some very useful things, such as when you say you expect a function to be called once with "Hello World":

$requestor->expects($this->once())
  ->method('afunction')
  ->with($this->equalTo('Hello World'));

Then it will not only complain if afunction() never gets called, but also if it gets called twice.

But structure like this also brings restrictions. Have a poke around some of the phpunit mock questions on StackOverflow to see if you will hit them.


Gin framework project with an endpoint to return a list of users based on their score My relevant work experience is with Java (mostly web development with Dropwizard/Spring) so I can not avoid using the same practices as I do in a Java project. The project is there but I am going to copy it here:

Project

A very simple project with Gin framework with an endpoint to return a list of users based on their score.

Structure

liveops-tool
-- user
---- model
------ user.go
---- service
------ userService.go
------ userDirectory.go (mock the db)
-- main.go

main.go

r.GET("/competition/users", func(c *gin.Context) {
        numUsers, err := strconv.Atoi(c.DefaultQuery("numusers", "6"))
        minScore, err := strconv.Atoi(c.DefaultQuery("minscore", "0"))
        maxScore, err := strconv.Atoi(c.DefaultQuery("maxscore", "2000"))
        fmt.Printf("numUsers: %d , minScore: %d , maxScore: %d \n", numUsers, minScore, maxScore)

        if err == nil {
            var res = userService.GenerateUserListByScore(numUsers, minScore, maxScore)
            c.JSON(http.StatusOK, gin.H{"users": res})
        } else {
            c.JSON(http.StatusBadRequest, gin.H{"users": "no valid"})
        }

    })

user.go

package user

type User struct {
    ID         string `json:"id"`
    Name       string `json:"name"`
    Country    string `json:"country"`
    TotalScore int    `json:"totalScore"`
}

userService.go

package user

import (
    user "liveops-tool/user/model"
)

// GenerateUserListByScore returns a list of users for a tournament
func GenerateUserListByScore(numUsers, minScore, maxScore int) []user.User {
    return searchUsers(numUsers, minScore, maxScore)
}

userDirectory.go

package user

import (
    "fmt"
    userModel "liveops-tool/user/model"
)

var users = []userModel.User{}

func init() {
    initUsers()
}

func searchUsers(numUsers, minScore, maxScore int) []userModel.User {
    return filterUsersByScore(numUsers, minScore, maxScore)
}

func filterUsersByScore(numUsers, minScore, maxScore int) []userModel.User {
    var filteredUsers = []userModel.User{}
    var countUsers = 0
    if len(users) < numUsers {
        numUsers = len(users)
    }
    for i := range users {
        if users[i].TotalScore >= minScore && users[i].TotalScore <= maxScore {
            filteredUsers = append(filteredUsers, users[i])
            countUsers++
        }

        if countUsers >= numUsers {
            break
        }
    }
    return filteredUsers
}

func initUsers() {
    user1 := userModel.User{ID: "1", Name: "John", Country: "UK", TotalScore: 500}
    user2 := userModel.User{ID: "2", Name: "Frank", Country: "ES", TotalScore: 1500}
    user3 := userModel.User{ID: "3", Name: "Bob", Country: "UK", TotalScore: 2000}
    user4 := userModel.User{ID: "4", Name: "Anna", Country: "FR", TotalScore: 3000}
    users = append(users, user1)
    users = append(users, user2)
    users = append(users, user3)
    users = append(users, user4)
    fmt.Printf("users: %+v\n", users)
}

Questions

I am mocking the user list and I would like the method initUsers would not be accesible from userService. Golang doesn't have public and private methods because the visibility depends on the package. The same for the function filterUsersByScore. How can I improve it?
User (model): I need to put the attributes with the first letter in uppercase to make it exportable when I return the JSON. Is it possible to have the attributes as private to access them through getters and setters and also return them in the JSON response?

Any other advice about the code is well received.

Let's start by addressing your 2 main questions:

1. Visibility (exporting) of functions

Yes, there's a very easy way of doing this, by using receiver functions (methods), and interfaces. Golang interfaces are incredibly powerful if you understand where they actually differ from "traditional" languages with a full-blown OOP model. you could have a directory type, and expose an interface to the service, which only declares the functions you want to use. Suppose you want your service to access GetUsers, but not a FilterByX, then just pass this directory type in as an interface type limiting how you can use it:

// in service
type Directory interface {
    GetUsers() []User
}

type service struct {
    dir Directory // interface type
}

func New(dir Directory) *service {
    return &service{
        dir: dir,
    }
}

The interface is declared in the package that uses it, not in the package implementing said interface. This makes unit-testing a lot easier. There's quite a few articles about this around. It's a bit odd at first, if you're used to OO, but it makes sense quite quickly. Don't try to make go look like C++, C#, or Java... it's its own thing, and though not perfect, it's got a lot of good things going for it.

As for the initUsers: that function is completely pointless. You could move the entire body of that function into your init function from the get-go. But why stop there? You can simply initialise your variable straight up, rather than creating 2 function calls to do the same thing:

var users = []User{
    {
        ID:         "1",
        Name:       "John",
        Country:    "UK",
        TotalScore: 500,
    },
    {
        ID:         "2",
        Name:       "Frank",
        Country:    "ES",
        TotalScore: 1500,
    },
    {
        ID:         "3",
        Name:       "Bob",
        Country:    "UK",
        TotalScore: 2000,
    },
    {
        ID:         "4",
        Name:       "Anna",
        Country:    "FR",
        TotalScore: 3000,
    },
}

That's it, all done.

2. Marshalling unexported fields.

Yes, this is possible, but I'd strongly advise against it. However, here's a couple of ways to do it. In both cases, we're relying on the fact that golang supports a JSON Marshaller and Unmarshaller interface out of the box.

type User struct {
    id      string
    name    string
    country string
    score   int
}

// manually creating a map
func (u User) MarshalJSON() ([]byte, error) {
    data := map[string]interface{}{
        "id":      u.id,
        "name":    u.name,
        "country": u.country,
        "score":   u.score,
    }
    return json.Marshal(data)
}

// now with some pointer magic... mimics the omitempty tag
func (u User) MarshalJSON() ([]byte, error) {
    data := map[string]interface{}{
        "id":      &u.id,
        "name":    &u.name,
        "country": &u.country,
        "score":   &u.score,
    }
    return json.Marshal(data)
}

Now for unmarshalling:

// note a pointer receiver is required here, we're changing the receiver state
func (u *User) UnmarshalJSON(v []byte) error {
    data := map[string]interface{}{} // empty map
    if err := json.Unmarshal(v, &data); err != nil {
        return err
    }
    if id, ok := data["id"]; ok {
        // technically we need to check the type assert, I'll do it once:
        strId, ok := id.(string)
        if !ok {
            return errors.New("id value is not a string!")
        }
        u.id = strId
    }
    if name, ok := data["name"]; ok {
        u.name = name.(string)
    }
    if country, ok := data["country"]; ok {
        u.country = country.(string)
    }
    if score, ok := data["score"]; ok {
        u.score = score.(int)
    }
    return nil
}

As you can probably see now, each field you add will add more code that needs to be written, changing the names of fields is a right PITA... it's better to just not bother IMO.

You could use the pointer trick from the marshaller here to avoid the type assertions, though:

func (u *User) UnmarshalJSON(v []byte) error {
    data := map[string]interface{}{
        "id":      &u.id,
        "name":    &u.name,
        "country": &u.country,
        "score":   &u.score,
    }
    // unmarshals into map[string]<pointer to fields on struct>
    return json.Unmarshal(v, &data)
}

Still, this doesn't look nice to me. I'd just stick with exported fields + json tags. If you really don't want people to access fields in certain places, just use interfaces as explained above.

Something you really ought not to do, is your over-use of package aliases. You may, or may not have noticed that I've always used the User type, without a package name. You've got aliases like:

user "liveops-tool/user/model"
// and the even worse
userModel "liveops-tool/user/model"

This is what sometimes is referred to as stuttering names. Read this aloud and ask yourself whether your package name is giving the user of your code any clearer understanding of what the User type represents:

u := user.User{} // u is an instance of User from the user package

What's in the user package? What is this User? I don't know. I'm just annoyed at having to type user twice.

u := userModel.User{} // u us a user from the userModel package

OK, now I know User is probably some kind of data structure. Good, but did I really need to know that I'm getting a User form the userModel package? I'm still annoyed at having to type user twice. The file itself is in the directory called model (perhaps models would be better). That's the bit of information that I want, so why not just:

import (
    "liveops-tool/user/model"
)

func foo() {
    u := model.User{} // u is a user model
}

That's a lot better, isn't it? You may want to have a look at the golang code review comments page. There's a lot of stuff in there that makes your code nicer. It's mainly sensible stuff: keep package aliases to a minimum, keep package names short, but communicative. Avoid name stutter, etc...

A particular bug-bear of mine is when people have a package like handler, and have a New function in there like this:

package handler

func NewHandler() (*h, error) {
    return &h{}, nil
}

Forcing me to write handler.NewHandler(). There's a reason why people find Java overly verbose (SomeFooBarImpl foobar = new SomeFooBarImpl();). What else do you expect handler.New() to return, other than a new handler?

/rant...

Might revisit this one later on, I'm heading home for the day now :P


HackerRank challenge to count ways to climb a staircase, solved using Java Fork Join I'm trying to solve Stair case recursion problem on HackerRank. The problem is to find all possible ways for a child to climb a stair case with height n given that he can jump 1, 2 or 3 steps at a time.

I'm using java Fork join to enhance performance but unfortunately it still slow.  My approach is as follows:

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.RecursiveTask;

public class RecursiveStairCase extends RecursiveTask<Integer> {

/**
 *
 */
private static final long serialVersionUID = 1L;
private Integer stairHeight;

public RecursiveStairCase(Integer stairHeight) {
    this.stairHeight = stairHeight;
}

@Override
protected Integer compute() {
    return getStairClimbPossibleWays(stairHeight);
}

static int getStairClimbPossibleWays(int stairHeight) {

    if (stairHeight < 0)
        return 0;

    if (stairHeight == 1 || stairHeight == 0)
        return 1;

    return getStairClimbPossibleWays(stairHeight - 1)
            + getStairClimbPossibleWays(stairHeight - 2)
            + getStairClimbPossibleWays(stairHeight - 3);
}

private List<RecursiveStairCase> createSubtasks() {
    List<RecursiveStairCase> subtasks = new ArrayList<RecursiveStairCase>();

    RecursiveStairCase subtask1 = new RecursiveStairCase(stairHeight - 1);
    RecursiveStairCase subtask2 = new RecursiveStairCase(stairHeight - 2);
    RecursiveStairCase subtask3 = new RecursiveStairCase(stairHeight - 3);

    subtask1.fork();
    subtask2.fork();
    subtask3.fork();

    subtasks.add(subtask1);
    subtasks.add(subtask2);
    subtasks.add(subtask3);

    return subtasks;
}

public static void main(String[] args) {
    Integer result = 0;
    long startTime = System.currentTimeMillis();
    int[] numbers = new int[] { 32, 33, 35, 36, 36 };
    for(int i=0;i<numbers.length;i++){
    result = 0;
    final RecursiveStairCase recursiveStairCase = new RecursiveStairCase(36);
    List<RecursiveStairCase> recursiveStairCases = recursiveStairCase
            .createSubtasks();
    for (RecursiveStairCase recursiveStairCaseThread : recursiveStairCases) {
        result += recursiveStairCaseThread.join();
    }
    System.out.println(result);
    }
    long stopTime = System.currentTimeMillis();
    long elapsedTime = stopTime - startTime;
    System.out.println("Consumed:" + elapsedTime);
}
}

I recorded the following test cases/time consumed pairs (which is refused by HackerRank (timed out)).

Test Case #5 {35,30,33,36,20}-->9.47 sec
Test Case #8 {32,33,35,36,36}-->15.531 sec

I need your advice to make it perform faster.

The reason your code still runs slow is the huge increase in how often you calculate the smaller steps.

If you want to find the number of ways for N steps. You calculate (N-1) once, (N-2) 2 times, (N-3) 4 times ..., (N-10) 274 times, ... (N-24) over a million times

Fun fact: the number of times you calculate (N-x) = calculateNumberOfWays(x).

oopexpert solved this by storing all the previous steps in a cache, so you calculate each step exactly once. This means it now takes O(N) recursive calls the first time the algorithm is called. And only 1 call afterwards (since it now became a lookup instead of a calculation).

The downside is that you actually store all intermediate results. An alternative solution is to not use recursion at all but work your way from the bottom up and store only the last 3 steps. An implementation could look like this:

public static int calculateNumberOfWays(int stairHeight){
    if(stairHeight < 0){
        return 0;
    }
    if(stairHeight < 2){
        return 1;
    }
    int twoStepsBack = 0;
    int previousStep = 1;
    int currentStep = 1;
    for(int i = 2; i <= stairHeight; i++){
        int nextStep = twoStepsBack + previousStep + currentStep;
        twoStepsBack = previousStep;
        previousStep = currentStep;
        currentStep = nextStep;
    }
    return currentStep;
}

This also has the next big advantage of not using recursion at all. Recursion in java is only good if you have a limited number of recursive steps. Otherwise you'll quickly run into a StackOverFlowError.

For example, running the following tiny program:

public static void main(String[] args) {
    recursionTest(0);
}

public static void recursionTest(int step){
    System.out.println(step);
    recursionTest(step+1);
}

Threw an error after printing 11412. For your problem giving input of over 10000 would be insane so it's probably not really an issue though which brings me to the next problem with your solution.

Both our solutions can only solve up to stairHeight 36. At 37 we get an integer overflow. Changing it from int to long allows up to 72.

If you want to work with really BIG integers, java has a BigInteger class. I'll leave the conversion to BigInteger to you (it's not that hard, as long as you remember to use someBigNumber.add(otherBigNumber) instead of the normal +).

Just for fun I tried to run it with n=10000 but the result is huuuuge (as expected). To give an idea, if I take result.toString().lenth() it's still 2647. But at least the program calculates this almost instantly :)


Connecting to a database I'm working on a web application that I inherited from a colleague long gone. To connect to the MySQL database I use the following classes:

statement.php

<?php
//class sql
//{
    class Statement
    {
        private $m_connection;

        function __construct($connection)
        {
            $this->m_connection = $connection;
        }

        function query($query)
        {
            $connection = $this->m_connection;
            $handle = $connection->getHandle();

            $result = mysql_query($query,$handle);

            if ($result === FALSE)
                throw new Exception(mysql_error());

            $instance = new Result($this);
            $instance->setHandle($result);

            return $instance;
        }
    }
//}
?>

result.php

<?php
//class sql
//{
    class Result
    {
        private $m_handle;
        private $m_statement;

        function __construct($statement)
        {
            $this->m_statement = $statement;
        }

        function setHandle($handle)
        {
            $this->m_handle = $handle;
        }

        function fetch()
        {
            $handle = $this->m_handle;
            $row = mysql_fetch_array($handle);

            return $row;
        }
    }
//}
?>

connection.php

<?php
include_once("statement.php");
include_once("result.php");

//class sql
//{
    class Connection
    {
        private $m_handle;

        function __construct($server,$username,$password)
        {
            $handle = mysql_connect($server,$username,$password);
            $this->m_handle = $handle;
        }

        function __destruct()
        {
            $handle = $this->m_handle;
            @mysql_close($handle);
        }

        function createStatement()
        {
            return new Statement($this);
        }

        function getHandle()
        {
            return $this->m_handle;
        }
    }
//}
?>

named.php

<?php
include_once("connection.php");

function createNamedConnection($name)
{
    if ($name == "dbalias1")
    {
        $connection = new Connection("first.host.com","user","uspw");

        $statement = $connection->createStatement();
        $statement->query("USE db1");

        return $connection;
    }
    if ($name == "dbalias2")
    {
        $connection = new Connection("second.host.com","user2","nouse");

        $statement = $connection->createStatement();
        $statement->query("USE db2");

        return $connection;
    }

    return null;
}
?>

To have a connection I can use the following script:

$connection = createNamedConnection("dbalias1");
$statement = $connection->createStatement();
$query = "SELECT   *
          FROM     tContent c
          WHERE    c.cID > 100";
$result = $statement->query($query);
while(($row = $result->fetch()) !== FALSE){
    //do something
}

The problem is that those classes caused me a lot of trouble, and I'm sure there is work to be done on them. I don't know where to begin to make those classes more secure and easy to use.

Consider using mysqli instead of mysql:

mysqli

Mysqli (the "i" stands for "improved") is more OO-friendly and has better security.  It also seems like the standard mysql functions are being deprecated (some of them anyway).


Vectorizing in Python I have the following code:

for j in range(rows):
    for i in range(cols):
        k = j * rows + i

        k1 = k + 1 if i < cols - 1 else k - 1
        k2 = k - 1 if i > 0 else k + 1
        k3 = k + cols if j < rows - 1 else k - cols
        k4 = k - cols if j > 0 else k + cols

        w1 = U[k]
        w2 = U[k-1] if i > 0 else U[k]
        w3 = V[k]
        w4 = V[k-cols] if j > 0 else V[k]

        zarray[k] = (w1 + w2 + w3 + w4) * parray[k] - (w1 * parray[k1] + w2 * parray[k2] + w3 * parray[k3] + w4 * parray[k4])

I want to know if there is a way to vectorize this loops, because I think that exists a kind of "convolution" for zarray.

U and V are arrays representing 2D matrices with 512x512 elements, and parray is also a 2D representation with 512x512 elements.

In a previous post vectorization was recommended, but now I can not figure out how to vectorize when I have different indices operations.

I was the one who recommended vectorizing.  I think the first approach would be to convert your code to using 2D indexes.  I think this will make the vectorizing clearer:

for j in range(rows):
    for i in range(cols):
        i1 = i+1 if i<cols-1 else i-1
        i2 = i-1 if i>0 else i+1
        j1 = j+1 if j<rows-1 else j-1
        j2 = j-1 if j>0 else j+1

        w1 = U[j, i]
        w2 = U[j, i-1] if i > 0 else U[j, i]
        w3 = V[j, i]
        w4 = V[j-1, i] if j > 0 else V[j, i]

        p = parray[j, i]
        p1 = parray[j, i1]
        p2 = parray[j, i2]
        p3 = parray[j1, i]
        p4 = parray[j2, i]

        zarray[j, i] = (w1 + w2 + w3 + w4)*p - (w1*p1 + w2*p2 + w3*p3 + w4*p4)

So, if I am reading this code right, you are shifting some rows and columns around.  So the next step is to re-implement this by making copies of the arrays that follow the same patterns:

U1 = np.empty_like(U)
V1 = np.empty_like(V)
parray1 = np.empty_like(parray)
parray2 = np.empty_like(parray)
parray3 = np.empty_like(parray)
parray4 = np.empty_like(parray)

U1[:, 1:] = U[:, :-1]  # this is w2
V1[1:, :] = V[:-1, :]  # this is w4
U1[:, 0] = U[:, 0]
V1[0, :] = V[0, :]

parray1[:, :-1] = parray[:, 1:]  # this is the result of k1
parray2[:, 1:] = parray[:, :-1]  # this is the result of k2
parray3[:-1, :] = parray[1:, :]  # this is the result of k3
parray4[1:, :] = parray[:-1, :]  # this is the result of k4

parray1[:, -1] = parray[:, -2]
parray2[:, 0] = parray[:, 1]
parray3[-1, :] = parray[-2, :]
parray4[0, :] = parray[1, :]

zarray = (U+U1+V+V1)*parray - (U*parray1 + U1*parray2 + V*parray3 + V1*parray4)

You could also use np.hstack and np.vstack instead of slices:

U1 = np.hstack([U[:, :1], U[:, :-1]])
V1 = np.vstack([V[:1, :], V[:-1, :]])

parray1 = np.hstack([parray[:, 1:], parray[:, -2:-1]])
parray2 = np.hstack([parray[:, 1:2], parray[:, :-1]])
parray3 = np.vstack([parray[1:, :], parray[-2:-1, :]])
parray4 = np.vstack([parray[1:2, :], parray[:-1, :]])

zarray = (U+U1+V+V1)*parray - (U*parray1 + U1*parray2 + V*parray3 + V1*parray4)