Tail-recursive query string builder in F# I have a situation where I'm generating a URL based on a set of query string parameters for use in an API. I know ahead of time that the values passed to this function are URL-safe and the tuples are complete, so I don't need to serialize the data or do data validation beyond just checking the length of the array.

This is a function where I'm generating a url from a (string * string)[]. I'm trying to avoid using mutable values, so this uses a tail-recursive function to build the string, with the URL being the "accumulator" value for the tail-recursion.

What I'm looking for is ways to better organize the function, flaws in the logic of the workflow, ways to make the function more readable, and just improvements in general.

let ComposeUrl (valuePairs:(string * string)[]) =
    let Composer (valuePairs:(string * string)[]) =
        let rec ComposerRec (valuePairs:(string * string)[]) (url:string) =
            match valuePairs.Length > 0 with
            | false -> url
            | true  ->
                let tail = valuePairs.[1..valuePairs.Length - 1]
                let url = url + sprintf "&%s=%s" (fst valuePairs.[0]) (snd valuePairs.[0])
                ComposerRec tail url

        let tail = valuePairs.[1..valuePairs.Length - 1]
        let url = sprintf "?%s=%s" (fst valuePairs.[0]) (snd valuePairs.[0])
        ComposerRec tail url

    match valuePairs.Length > 0 with
    | false -> ""                     // No query string parameters
    | true  -> Composer valuePairs

As Mauricio suggested:

let ComposeUrl valuePairs =
    if Seq.isEmpty valuePairs then ""
    else
        let values = valuePairs |> Seq.map (fun (k, v) -> sprintf "%s=%s" k v)
        "?" + String.Join("&", values)


Server side validation for reCAPTCHA V2 or Invisible reCAPTCHA with Java (Servlet) The main reason for this post is that I spent quite a lot of time searching the easiest way to validate recaptcha V2. So I'm going to share my knowledge to avoid further time wastage of developers.

I'm using org.json library for this. Get the .jar file from here or read the docs. Add the .jar. file to your project and import the following classes.

import java.io.BufferedReader;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.net.URL;
import java.nio.charset.Charset;
import org.json.JSONObject;

Use the following method to for validation:

/**
 * Validates Google reCAPTCHA V2 or Invisible reCAPTCHA.
 * @param secretKey Secret key (key given for communication between your site and Google)
 * @param response reCAPTCHA response from client side. (g-recaptcha-response)
 * @return true if validation successful, false otherwise.
 */
public static boolean isCaptchaValid(String secretKey, String response) {
    try {
        String url = "https://www.google.com/recaptcha/api/siteverify?"
                + "secret=" + secretKey
                + "&response=" + response;
        InputStream res = new URL(url).openStream();
        BufferedReader rd = new BufferedReader(new InputStreamReader(res, Charset.forName("UTF-8")));

        StringBuilder sb = new StringBuilder();
        int cp;
        while ((cp = rd.read()) != -1) {
            sb.append((char) cp);
        }
        String jsonText = sb.toString();
        res.close();

        JSONObject json = new JSONObject(jsonText);
        return json.getBoolean("success");
    } catch (Exception e) {
        return false;
    }
}

Call the above method as shown:

if(isCaptchaValid("enter_your_key_here", request.getParameter("g-recaptcha-response"))){
    //valid
}

One small criticism of you're code that I'll offer is that the loop in the middle which reads a stream could be usefully factored out and put in a general utility.  Reading a stream is a common problem and could be made more general.

In fact I have a small routine to do that sitting here on my hard drive.  Here's my version:

public class IoUtils
{

   private IoUtils() {}

   public static CharSequence readFully( Reader reader )
           throws IOException
   {
      char[] buffer = new char[ 1024 ];
      StringBuilder stringBuilder = new StringBuilder();
      for( int length; ( length = reader.read( buffer ) ) != -1; )
         stringBuilder.append( buffer, 0, length );
      return stringBuilder;
   }

   public static ByteArrayOutputStream readFully( InputStream ins )
           throws IOException
   {
      ByteArrayOutputStream bos = new ByteArrayOutputStream();
      byte[] bytes = new byte[ 1024 ];
      for( int length; ( length = ins.read( bytes ) ) != -1; )
         bos.write( bytes, 0, length );
      return bos;
   }
}

Use it something like this:

    InputStream res = new URL(url).openStream();
    BufferedReader rd = new BufferedReader(new InputStreamReader(res, Charset.forName("UTF-8")));

    CharSequence cs = IoUtils.readFully( rd );

    JSONObject json = new JSONObject( cs.toString() );

Reading a stream as UTF-8 is so common you might even want to make a separate method just for that.  Then you could do

JSONObject json = new JSONObject( IoUtils.readFullyUtf8(
         new URL(url).openStream() ).toString() );

Some might find a single line of code more clear to read.


Python code to continue if a certain type is seen, otherwise try again to get valid input I am very very new to programming, and this is my first day using Python.

I want to check to see if an input is of type 'int' and to proceed with my program if the user has entered an 'int', otherwise to print some error message, followed by a prompt to the user to try again.

What I have:

user_in = input('Input an integer value')
if type(user_in) == int:
    a = user_in
else:

I'm really not sure what to do here, or if its even possible to achieve this way, hence the blank line after 'else:'! I tried it with a while loop with try and excepts such as this:

while True:
    try:
        user_in = int(input('Enter an integer value'))
        a = user_in
    except:
        print('Invalid input, please try again')

Which works good for the exception case (because of the while True essentially acting infinitely), but of course, does not move on to the next part of the code in the case where an integer has successfully been inputted.

Your first code snippet has the problem that (in Python 3) input always returns a string, so type(user_in) will always be str, which will never compare equal to int.

Your second code snippet solves the problem in the correct way: try to convert the result to an integer, and catch the exception in case that fails.

Fixing that code is as simple as adding a break statement:

while True:
    try:
        user_in = int(input('Enter an integer value'))
        break
    except ValueError:
        print('Invalid input, please try again')
# do something with user_in here

There's no reason to assign the result to user_in and then to a. If you want it to be in a variable called a, just assign it to a to begin with.

Also, you should catch the specific exception that you are looking for, in this case ValueError. Catching every exception is almost never a good idea, because this code could fail for reasons other than the user entering something invalid. For example, if you inadvertently used input as a variable elsewhere in the same function, this call of it will probably raise a TypeError or NameError, and you'll want to see that error, not an infinite loop of "Invalid input, please try again".


Catching API changes for a service that doesn't version its API I'm using the old Google Data API to access several spreadsheets and put their contents in a database using Flask and SQLAlchemy.  Each run deletes the contents of the database tables and repopulates them with results of the API call.

I'm trying to detect API changes and abort the operation to prevent leaving the tables empty or filling them with garbage.  I do this first by checking the status code of the API request.  If it's anything other than 200 I abort the operation.  To catch when the URL is valid but the JSON result has a new format, I catch KeyErrors when parsing the JSON.

Each time one of these conditions occurs I rollback the database session and exit the function with 1.

I'm worried that my detection coverage is spotty and I'm left feeling there should be a better way...

def get_google_content(key, db_table):
    if DEBUG:
        print 'Fetching content from Google Drive'
    spreadsheet_url = ('https://spreadsheets.google.com/feeds/worksheets/'
                       '{}/public/basic?alt=json'.format(key))
    spreadsheet_req = requests.get(spreadsheet_url)
    if spreadsheet_req.status_code != 200:
        print 'Failed to retrieve spreadsheet from {}'.format(spreadsheet_url)
        print 'Google Spreadsheet returned {}'.format(
                spreadsheet_req.status_code)
        db.session.rollback()
        return 1

    sheet_ids = list(OrderedDict.fromkeys(re.findall(r'/public/basic/(\w*)',
                     spreadsheet_req.text, flags=0)))

    # The only trivial way to catch removed spreadsheet data is to delete the
    # table and fill it all out again
    db.session.query(db_table).delete()

    for sheet_id in sheet_ids:
        worksheet_url = ('https://spreadsheets.google.com/feeds/list/{}/{}/'
                         'public/values?alt=json'.format(key, sheet_id))
        worksheet_req = requests.get(worksheet_url)
        if worksheet_req.status_code != 200:
            print 'Failed to retrieve spreadsheet from {}'.format(worksheet_url)
            print 'Google Spreadsheet returned {}'.format(
                    worksheet_req.status_code)
            db.session.rollback()
            return 1

        try:
            sheet_name = worksheet_req.json()['feed']['title']['$t']
            for row in worksheet_req.json()['feed']['entry']:
                db_row = db_table(column1=sheet_name,
                                  column1=row['gsx$location']['$t'],
                                  column2=row['gsx$caption']['$t'])
                db.session.add(db_row)
        except KeyError:
            print 'KeyError accessing Google Spreadsheet JSON from {}'.format(
                worksheet_url)
            print 'API changed?'
            db.session.rollback()
            return 1

        db.session.commit()
        return 0

Here's the relevant route:

@app.route('/admin/refresh_content')
def refresh_content():
    spreadsheet_key1 = 'key_1'
    spreadsheet_key2 = 'key_2'
    spreadsheet_key3 = 'key_3'

    sheet1_status = get_google_content(spreadsheet_key1, Table1)
    sheet2_status = get_google_content(spreadsheet_key2, Table2)
    sheet3_status = get_google_content(spreadsheet_key3, Table3)

    if sheet1_status + sheet2_status + sheet3_status > 0:
        return '
Failure

Unable to update spreadsheets'
    else:
        return '
Success

Updated spreadsheets'

Some comments:

Instead of using print statements, the logging module could be used
The return value of get_google_content should be a boolean value (True on success, False on failure)

Regarding ways to make the refresh_content method a little bit more pythonic, I would probably use a for loop and the all built-in function to make sure that all the calls to get_google_content  were successful.

@app.route('/admin/refresh_content')
def refresh_content():
    spreadsheet_keys = ['key_1', 'key_2', 'key_3']
    tables = [Table1, Table2, Table3]
    sheet_statuses = [
      get_google_content(spreadsheet_key, table)
      for spreadsheet_key, table
      in zip(spreadsheet_keys, tables)
    ]

    if all(sheet_statuses):
        return '
Success

Updated spreadsheets'
    else:
        return '
Failure

Unable to update all spreadsheets'

There are other strategies, but that depends on different things like whether you want to stop on the first failure or not.

I hope this helps.


Calling methods of another module from a module, from a class extending both? module A
  def methodInA
    # do stuff
    methodInB
    # do stuff
  end
end

module B
  def methodInB
    puts "this is a B method!"
  end
end

class C
  extend A
  extend B

  def initialize
    methodInA
  end
end

c = new C

I'm not feeling good about this.  Is this a good idea in general?  Any pointers as to where I can learn the best pattern for this?

Since inheritance and interface both has some problems of their own, high level languages like ruby, python supports a different way for organizing code- Mixin.

The code you have written involves mixin, you can see this as interface with implemented methods.When a class can inherit features from more than one parent class, the class is supposed to show multiple inheritance.

Ruby does not suppoprt mutiple inheritance directly but Ruby Modules have another, wonderful use. At a stroke, they pretty much eliminate the need for multiple inheritance, providing a facility called a mixin.

Mixins give you a wonderfully controlled way of adding functionality to classes. However, their true power comes out when the code in the mixin starts to interact with code in the class that uses it.

say we have a module for debugging. we want to use this in many classes and modules. we can just include this debug module to our classes and all methods for debugging will be added in the code as own method of our class.

see the code bellow:

module Debugger
  def log
    #do stuff
    puts "#{Time.now}: This is a log"
  end
end

class C
  include Debugger

  def initialize
    #do stuff
  end

  def run
    #do stuff
    log
  end

end

c =  C.new
c.run


Is this a thread safe way to control access to a reusable resource? Is there a better way? public static class ThreadStatic<T> where T : new()
{
    [ThreadStatic]
    private static T instance;

    public static T Instance
    {
        get { return instance ?? (instance = new T()); }
    }
}

Used like this:

var probability = ThreadStatic<Random>.Instance.NextDouble();

Can this be broken? (Like, am I missing a volatile somewhere?)
Is there an existing pattern for this?  Is there a better way to do this?
What would be an elegant way to allow for constructor parameters to be specified?
Would this be better suited to an IOC container?

I'm not sure to be honest, the contained resource would be specific per thread and created upon first read so volatile may be unnecessary as I believe that's for multiple threads reading and writing to the same field.
It looks like you are trying to make a singleton version of the ThreadLocal class, unless you have a good reason I would suggest that you just use that instead.
The ThreadLocal class allows for a factory method (as a Func<T>) to be supplied to create a more complex object.
It depends on the type of resource you want to use in this manner, if it is a service or resource then yes an IOC may be a better option as most containers allow an instance per thread. However if you just need per thread storage of a field, then ThreadLocal is a better option. It also allows the resource to be properly disposed if required too.


Simple method to detect int overflow To detect int overflow/underflow in C, I use this code. What might be a simpler and more portable way of coding this (that is, fewer conditions)?

Assume 2's complement and don't use wider integers.

int a,b,sum;
sum = a + b;
// out-of-range only possible when the signs are the same.
if ((a < 0) == (b < 0)) {
  if (a < 0) {
    // Underflow here means the result is excessively negative.
    if (sum > b) UnderflowDetected();
  }
  else {
    if (sum < b) OverflowDetected();
  }

It's not possible to avoid undefined behaviour by testing for it after the fact! If the addition overflows then there is already undefined behaviour here:

sum = a + b;

so attempting to test afterwards is too late. You have to test for possible overflow before you do a signed addition. (If you're puzzled by this, read Dietz et al. (2012), "Understanding Integer Overﬂow in C/C++". Or even you're not puzzled: it's an excellent paper!)

If it were me, I'd do something like this:

#include <limits.h>

int safe_add(int a, int b) {
    if (a > 0 && b > INT_MAX - a) {
        /* handle overflow */
    } else if (a < 0 && b < INT_MIN - a) {
        /* handle underflow */
    }
    return a + b;
}

but I'm not entirely sure what the point of having separate cases for overflow and underflow is.

I also use Clang's -fsanitize=undefined when building for test.


Scraping content from yellowpage and write them to a csv file I've written a script in python to scrape names and phone numbers of a certain search from yellowpage traversing multiple pages. The scraper has the ability to collect all the links connected to all the names. Once the links are collected, the scraper goes to the target page to parse the name and website address. Finally, it will produce a csv file with data filled-in. Although the scraper is doing it's job errorlessly, I'm little dubious as to whether I used the best way to write items in a csv file. Any betterment to the pattern will be highly appreciated.

Here is what I've written:

import requests
import csv
from urllib.parse import urljoin
from bs4 import BeautifulSoup

url = "https://www.yellowpages.com/search?search_terms=+Injury+Law+Attorneys&geo_location_terms=California"

def get_links(session,link):
    session.headers['User-Agent'] = 'Mozilla/5.0'
    res = session.get(link)
    soup = BeautifulSoup(res.text,"lxml")
    for items in soup.select(".info h2 a[data-analytics]"):
        get_info(session,urljoin(url,items.get("href")))

    nextpage = soup.select_one("a.next")
    if nextpage:  #If there is no more next page link, it should break
        nexturl = urljoin(link,nextpage.get("href"))
        print(nexturl)        #check to see which page I'm on
        get_links(session,nexturl)  #supply the newly produced next page link to grab the next one

def get_info(session,tlink):
    session.headers['User-Agent'] = 'Mozilla/5.0'
    res = session.get(tlink)
    soup = BeautifulSoup(res.text,"lxml")
    name = soup.select_one("#main-header .sales-info h1").get_text(strip=True)
    website = soup.select_one("#main-header a.website-link").get("href") if soup.select_one("#main-header a.website-link") else ""
    print(name,website)
    writer.writerow([name,website])  #writing data in a csv file

if __name__ == '__main__':
    with open("yellowpageinfo.csv","w",newline="") as infile: #creating a csv file to write populated results
        writer = csv.writer(infile)
        writer.writerow(['Name','Website'])
        with requests.Session() as session:
            get_links(session,url)

Your description and your code don't seem to match. In the code I don't see anything related to phone numbers. I also don't understand what "The scraper has the ability to collect all the links connected to all the names." means. With that said, I'm not sure if any of my critique is valid, but here I go any way.

To answer your question about writing to csv, I think you are doing it right. In fact, you are doing it exactly as shown in documentation, so no issues here.

Now I'd like to get into details.

Minor issues

Format your code

Run autopep8 and isort on your code before submitting it. Makes it easier to quickly scan through it.
Use consistent string quoting

It doesn't matter whether you use single or double quotes. What matters is that you pick one and stick with it.
Get rid of useless comments

Consider:

writer.writerow([name, website])  # writing data in a csv file

This comment is virtually useless, because the code right beside it says the same thing. Comments like this explain nothing  and should be deleted.
Variable naming

Spare extra characters to rename res to response
Avoid generic names (e.g. rename item to something like search_result)
Use underscore to separate individual words in variable name (e.g. nextpage →next_page)

Good variable names often eliminate the need for comments. Effort spent on coming up with concise and descriptive names goes a long way towards improving readability.

More substantial issues

This line is way too long.

 website = soup.select_one("#main-header a.website-link").get("href") if soup.select_one("#main-header a.website-link") else ""

Consider rewriting it as an ordinary if statement. Save the result of soup.select_one(...) into a variable to avoid repeated function call.
Use of recursion in get_links is confusing. Consider rewriting it using a while loop.

Some things I might have misunderstood

There doesn't seem be any need to use requests.Session and on top of that reset headers before every call to get. If you really need a persistent session, it'd be a good idea to leave a comment explaining why.
In your code you seem to be only grabbing name and website for each search result. If that's the case, loading the details page for each result is redundant as all desired information is present in the search results themselves.


String compression using repeated character counts Implement a method to perform basic string compression using the counts of
  repeated characters. For example, the string aabcccccaaa would become
  a2blc5a3. If the "compressed" string would not become smaller than the original string, your method should return the original string.

Can this be implemented in a better way performance-wise?

package string;

public class CompressChar {

    int[] seq = new int [256];

    public String compressString(String str){

        StringBuffer strComp = new StringBuffer();
        for( char c : str.toCharArray()){
            seq[c]++;
        }
        for (char c : str.toCharArray()){
            if(seq[c]>0){
                strComp.append(c).append(seq[c]);
                seq[c]=0;//so that it does not enter , when char occurs again
            }
        }

        if(str.length()<strComp.length()){
            return str;
        }
        return strComp.toString();

    }
    public static void main(String[] args) {
        CompressChar ch = new CompressChar();
        System.out.println(ch.compressString("abbcdrfac"));
    }
}

Your solution here is wrong, it produces incorrect results when it compresses chars that are in two different places in the string:

For the input: aaaaabbbbbcdrfaaaaaaccccc your program produces:

a11b5c6d1r1f1

when it should produce:

a5b5c1d1r1f1a6c5

Because the a and c repeat in two different places, you include them all in the first count.

Since this is homework, writing out the full solution would be counter-productive, but, if you have:

StringBuilder sb = new StringBuilder();

and two variables to contain the previous char and the previous char count, you could just go through each character in the string, if it's the same as the previous char, you increment the count. If it's different, you add the previous char and it's count to the StringBuilder, and reset the previous and count to 1.

Then, when your loop is done, if the StringBuilder is smaller than the input, return the StringBuilder version.

This type of compression is called 'run length encoding' by the way.


IEnumerable of classes that implement a given interface at runtime I am implementing the command pattern in a project that I am working on and I have an interface, ICommandFactory that all of my commands are implementing.

When I run the application I want to dynamically gather all classes in the current assembly that implement that interface, thus producing a list of all possible commands.

So I came up with the method below. This was the first thing that I could get to work. Is there a better way to do this?

    private static IEnumerable<ICommandFactory> GetAvailableCommands()
    {
        var interfaceType = typeof (ICommandFactory);
        var implementors = Assembly.GetExecutingAssembly().GetTypes().Where(
            i => interfaceType.IsAssignableFrom(i) && i != interfaceType);

        var commands = new List<ICommandFactory>();

        foreach(var implementor in implementors)
        {
            var ctor = implementor.GetConstructor(Type.EmptyTypes);
            if (ctor == null) continue;
            var instance = ctor.Invoke(null) as ICommandFactory;
            if (instance == null) continue;
            commands.Add(instance);
        }

        return commands;
    }

I only have a few minor comments about your code:

This code:

var ctor = implementor.GetConstructor(Type.EmptyTypes);
if (ctor == null) continue;

has the potential to mask a bug. Imagine you add a constructor to one of your command-factory types and accidentally forget that this implicitly removes the default constructor. This code will then silently swallow this mistake and simply not list that particular command factory. It would be preferable to throw in this case so that you notice it immediately. Even better, of course, would be to use a post-build event to turn this error into a compile-time check so that you can’t even run the program when it is invalid.
This code:

var instance = ctor.Invoke(null) as ICommandFactory;
if (instance == null) continue;

has exactly the same problem, but the consequences are much more subtle. The if (instance == null) condition should never fire because the code was written so that the instance cannot be of any other type. Imagine you have a bug in that code. This code swallows this bug and you won’t notice it directly. You will only notice some subtle/weird effects that occur much later, and it will be a pain to trace it back to this code here. You could make it throw like above, but personally I think you should change this to var instance = (ICommandFactory) ctor.Invoke(null); so that it will automatically throw if it’s not of the right type.
Your code does not allow for a class that implements ICommandFactory but is not intended to appear in your UI. If you are sure that you don’t want that to be possible, then your code is fine. Otherwise, I would declare a custom attribute — either one that allows you to explicitly exclude an ICommandFactory from appearing in the UI, or one that is required on all the ICommandFactory types to be included.
Very very minor nitpick:

i => interfaceType.IsAssignableFrom(i) && i != interfaceType

You should perform the simpler check first:

i => i != interfaceType && interfaceType.IsAssignableFrom(i)


Project Euler # 24 Lexicographic permutations in Python A permutation is an ordered arrangement of objects. For example, 3124 is one possible permutation of
the digits 1, 2, 3 and 4. If all of the permutations are listed numerically or alphabetically, we call
it lexicographic order. The lexicographic permutations of 0, 1 and 2 are:

012   021   102   120   201   210

What is the millionth lexicographic permutation of the digits 0, 1, 2, 3, 4, 5, 6, 7, 8 and 9?

import itertools

def lexicographic(n, digits):
    """Assumes digits a string.
    returns the nth item of the lexicographic permutation of digits."""
    return list(itertools.permutations(digits))[n - 1]

if __name__ == '__main__':
    permutation = list(lexicographic(1000000, '0123456789'))
    print(''.join(permutation))

You’re building up a huge list of permutations in order to get to exactly one permutation. This is needlessly using time (computing all permutations that come before it) and memory (storing all those permutations in a list.

If you have 9 objects, you can have 9! = 3,628,800 permutations.  So with the 10 digits, 0 through 9, the first 362,880 permutations will be 0#########, the next 362,880 permutations will be 1#########, and you’ll reach the millionth permutation 274,240 permutations in to the 2#########’s.  So, 1 digit down, 9 to go.

With 8 objects, you can have 8! = 40,320 permutations.  Again, you can determine you’ll cross the 274,240’th permutation during the 7th group of permutations of the 9 objects: 013456789 ... so the your millionth permutation will be of the form 27########.

Repeat for the rest of the digits: a calculator works fine.   Or write a program to solve this, and the answer will be returned directly in a fraction of a second.


Is there a more elegant way to catch a generic exception than using try-catch block? Here are two sample blocks of my code:

function FindTheNumberOfAddedItems ( PageObj, ItemType )
{
    var localPageObj = PageObj;
    var localItemType = ItemType;
    var numberOfAddedItems = 0;
    var NROCssSelector  = "div[class='assessment-stub list-top-border-colour-1 clearfix box-shadow'] strong";
    var OROCssSelector  = "div[class='assessment-stub list-top-border-colour-2 clearfix box-shadow'] strong";
    var RCCssSelector  = "div[class='assessment-stub list-top-border-colour-3 clearfix box-shadow'] strong";

    try {
        switch (localItemType)
        {
            case "NRO":
                numberOfAddedItems = ParseNumberOutOfString(localPageObj.QuerySelector(NROCssSelector).innerText);
                break;
            case "ORO":
                numberOfAddedItems = ParseNumberOutOfString(localPageObj.QuerySelector(OROCssSelector).innerText);
                break;
            case "RC":
                numberOfAddedItems = ParseNumberOutOfString(localPageObj.QuerySelector(RCCssSelector).innerText);
                break;
            default:
                Log.Message("Invalid item type");
        }
    }
    catch (err){
        Log.Message("Can not find then number of added items.");
    }
    return numberOfAddedItems;
}

function ParseNumberOutOfString ( TextString )
{
    var localTextString = TextString;
    var numberOnlyRegexPattern = /\d+/g;

    try {
        return localTextString.match(numberOnlyRegexPattern);
    }
    catch (err){
        Log.Message("Can not parse number out of string.");
    }
}

I have used try-catch blocks for both functions (and for the rest of all of my functions) in case there is anything wrong with my code.

Come to think about it, having try-catch blocks everywhere is kind of messy. Is there another more elegant way to do it?

@HristoYank is correct, you shouldn't catch exceptions that you can't handle. What you are doing is basically just ignoring the error, which will make it difficult to trace bugs.

If I call a method called ParseNumberOutOfString, I will expect to get a number back. If the function can't return a number, I need to know that, so that I can react appropriately.

Depending on the situation, I may want to use a default value, ask the user for new input, or abort with an appropriate error message.

If the function just swallows the error and logs it somewhere, I will continue processing with invalid data (null in this case), which will result in all sorts of bugs down the line. As only a generic error message and no stracktrace is logged, debugging will also be rather difficult.

Misc

match doesn't actually throw an exception, so you will never get into the catch.
don't uppercase your variable names.


SASS Code Structure / Readability I'm trying out a new approach to SASS stylesheets, which I'm hoping will make them more organizined, maintainable, and readable.

I often feel like there is a thin line between code that is well structured and code that is entirely convoluted. I would appreciate any thoughts as to which side of the line this code falls.

I don't want to tell you too much more about what these styles are intended to produce -- my hope is that the code will explain this for itself. Also note that this is part of a larger project, so don't worry about missing dependencies, etc.

Questions for review

How would you make this code easier to read/maintain?
Can you understand what these styles are trying to produce?
Is the purpose of the mixins/placeholders clear?

File structure:

theme/sass/partials/
    widget/
        collapsable/
            _appendicon.scss
            _closeall.scss
            _toggleswitch.scss
        collapsable.scss
        collapsablered.scss
    _button.scss

collapsable.scss

/**
 * Collapsable widget.
 *
 * The widget has "open" and "closed" states.
 * The widget has a Toggle Switch, which is visible in
 * both open and closed states.
 *    All other content is hidden in the closed state.
*/
@mixin setOpenState {
  &,
  &.state-open {
    @content;
  }
}

@mixin setClosedState {
  &.state-closed {
    @content;
  }
}

@mixin setToggleSwitchStyles {
  &>h1:first-child, .collapseableToggle {
    @content;
  }
}

@import "collapsable/closeall";
@import "collapsable/appendicon";
@import "collapsable/toggleswitch";

%collapsable {
  @include setOpenState {
    @include setToggleSwitchStyles {
      @extend %toggleSwitch;
    }
  }

  @include setClosedState {
    @extend %closeAllExceptToggle;

    @include setToggleSwitchStyles {
      @extend %toggleSwitchClosed;
    }
  }
}

collapsablered.scss

@import "collapsable";
@import "../button";

%collapsableRed {
  @extend %collapsable;

  @include setOpenState {
    @include setToggleSwitchStyles {
      @extend %buttonWithRedBg;
    }
  }

  @include setClosedState {
    @include setToggleSwitchStyles {
      @extend %buttonWithDarkBg;
    }
  }
}

collapsable/_closeall.scss

%closeAllChildren {
  * {
    display: none;
  }
}

%closeAllExceptToggle {
  @extend %closeAllChildren;

  @include setToggleSwitchStyles {
    display: block;

    .icon-sprite {
      display: inline-block;
    }
  }
}

collapsable/_appendicon.scss

@import "compass/utilities/general/clearfix";
@import "../../icon";

@mixin appendIcon {
  @include pie-clearfix;

  .icon-sprite {
    margin-right: 5px;
    vertical-align: -3px;
  }

  &:after {
    content: '';

    position: relative;
    top: 2px;

    float: right;

    @content;
  }
}

%withCloseIcon {
  @include appendIcon {
    @extend .icon-close;   // defined in _icon.scss
  }
}

%withOpenIcon {
  @include appendIcon {
    @extend .icon-rChevronDk;   // defined in _icon.scss
    top: 1px;
  }
}

collapsable/_toggleswitch.scss

%toggleSwitch {
  cursor: pointer;
  @extend %withCloseIcon;
}

%toggleSwitchClosed {
  @extend %toggleSwitch;
  @extend %withOpenIcon;
}

partials/_button.scss

@import "typography";

%buttonWithRedBg {
  @extend %textOnRedBg;  // defined in _typography.scss

  cursor: pointer;

  &:hover {
    background-color: $redDk;
  }

  &:active {
    background-color: $black;
  }
}

%buttonWithDarkBg {
  @extend %textOnDarkBg;  // defined in _typography.scss

  cursor: pointer;

  &:hover {
    background-color: #000;
  }

  &:active {
    background-color: $redDk;
  }
}

Overall, your naming conventions are pretty good.  I don't feel like I need to go look at mixins themselves to figure out what their purpose is.  The extensive use of extends does concern me, since it can lead to larger CSS rather than smaller like you might expect (see: Mixin, @extend or (silent) class?).

Your %textOnDarkBg and %textOnRedBg extend classes might be redundant.  If you're not already using Compass, you might want to take a look at it.  It offers a function as well as a mixin for setting a good contrasting color against your desired background color (see: http://compass-style.org/reference/compass/utilities/color/contrast/).  Highly useful if your project is intended to be themed.

Generally speaking, using colors for class names isn't very clear unless the content is about color (eg. a color wheel or a rainbow).  What is red for?  Is it for errors?  Or maybe a call to action?  The same thing goes for dark.  Using inverted or closed might be better choices.  If the site's design is already dark, a dark button probably doesn't make much sense.

Your code only allows the user to have exactly 2 colors (default and red), which seems more limited than it needs to be.  You could easily make it very flexible by making use of lists (or maps, which will be in the next version of Sass).  Here's an example from my own project:

//      name     dark    light
$dialog-help:    #2E3192 #B9C2E1 !default; // purple
$dialog-info:    #005FB4 #BDE5F8 !default; // blue
$dialog-success: #6F7D03 #DFE5B0 !default; // green
$dialog-warning: #A0410D #EFBBA0 !default; // orange
$dialog-error:   #C41616 #F8AAAA !default; // red

$dialog-attributes:
    ( help    nth($dialog-help, 1) nth($dialog-help, 2)
    , info    nth($dialog-info, 1) nth($dialog-info, 2)
    , success nth($dialog-success, 1) nth($dialog-success, 2)
    , warning nth($dialog-warning, 1) nth($dialog-warning, 2)
    , error   nth($dialog-error, 1) nth($dialog-error, 2)
    ) !default;

@each $a in $dialog-attributes {
    $name: nth($a, 1);
    $color: nth($a, 2);
    $bg: nth($a, 3);

    %dialog-colors.#{$name} {
        color: $color;
        background-color: $bg;
    }

    %dialog-colors-inverted.#{$name} {
        color: $bg;
        background-color: $color;
    }

    %badge-colors.#{$name} {
        background-color: $color;
        color: $background-color;
    }

    %button-colors.#{$name} {
        @include button($base: $bg) {
            @include button-text($color, inset);

            @include button-states;
        }
    }

    %button-colors-inverted.#{$name} {
        @include button($base: $color) {
            @include button-text($bg, inset);

            @include button-states;
        }
    }

    %button-colors-faded.#{$name} {
        @include button($base: fade($bg, 10%)) {
            color: #CCC;

            @include button-states;
        }
    }
}

In case you're wondering why I'm using multiple classes, I've setup a short demo:  http://sassmeister.com/gist/7792677


Unscramble words faster in Python So I'm currently using python to unscramble words that are retrieved from an OCR program (pytesseract).

The current code I am using to unscramble words is this:

import numpy as nm
import pytesseract
import cv2
import ctypes
from PIL import ImageGrab

def imToString():

    # Path of tesseract executable
    pytesseract.pytesseract.tesseract_cmd =r'C:\Program Files (x86)\Tesseract-OCR\tesseract'
    while(True):

        # ImageGrab-To capture the screen image in a loop.
        # Bbox used to capture a specific area.
        cap = ImageGrab.grab(bbox =(687, 224, 1104, 240))

        # Converted the image to monochrome for it to be easily
        # read by the OCR and obtained the output String.
        tesstr = pytesseract.image_to_string(
                cv2.cvtColor(nm.array(cap), cv2.COLOR_BGR2GRAY),
                lang ='eng')
        checkWord(tesstr)

def checkWord(tesstr):

    dictionary =['orange', 'marshmellow']

    scrambled = tesstr
    for word in dictionary:
        if sorted(word) == sorted(scrambled):
            print(word)

imToString()

I wanna know if there is anyway to reduce the time it takes to:

Scan/Process the image.
Look through the 'dictionary', as there are many more words to go through. Or another more efficient alternative.

Thanks

You are sorting your dictionary each time you use it, do it once and store it.

Also, you are sorting your scrambled word more than once, only do it once:

import numpy as nm
import pytesseract
import cv2
import ctypes
from PIL import ImageGrab

def imToString(dictionary):

    # Path of tesseract executable
    pytesseract.pytesseract.tesseract_cmd =r'C:\Program Files (x86)\Tesseract-OCR\tesseract'
    while(True):

        # ImageGrab-To capture the screen image in a loop.
        # Bbox used to capture a specific area.
        cap = ImageGrab.grab(bbox =(687, 224, 1104, 240))

        # Converted the image to monochrome for it to be easily
        # read by the OCR and obtained the output String.
        tesstr = pytesseract.image_to_string(
                cv2.cvtColor(nm.array(cap), cv2.COLOR_BGR2GRAY),
                lang ='eng')
        checkWord(tesstr, dictionary)

def checkWord(tesstr, dictionary):

    scrambled = tesstr
    sorted_scrambled = sorted(scrambled) # Only do this 1 time
    if sorted_scrambled in dictionary:
        print(dictionary[sorted_scrambled]

# ... Create your dictionary somewhere else and pass it in:
dictionary ={sorted('orange'): 'orange',sorted('marshmellow'):  'marshmellow'}

imToString(dictionary)


Seeking proper system for pairing types My code below ensures that only land animals can be a "representatives" for Land while only ocean animals can be representatives for Ocean, and so forth for Air, Underground, etc...  Just want some feedback about whether this system I've come up with is any good or if there is a better one.  Note:  I'm using structs instead of classes to keep it simple.

#include <iostream>
#include <list>

struct IAnimal { virtual ~IAnimal() = default; };

template <typename T>
struct Animal : virtual IAnimal {};  // Animal<T> can only be pair with T, where T is derived from Place.

struct Place {
    IAnimal* representative;
    virtual bool setRepresentative (IAnimal* a) {
        representative = a;  std::cout << "Done.\n";  return true;
    }
    virtual bool canUse (const IAnimal*) const = 0;
};

template <typename Derived>
struct PlaceCRTP : Place {
    virtual bool setRepresentative (IAnimal* a) override {
        if (canUse(a))
            return Place::setRepresentative(a);
        std::cout << "Cannot be done.\n";
        return false;
    }
    virtual bool canUse (const IAnimal* a) const override {
        return dynamic_cast<const Animal<Derived>*>(a) != nullptr;
    }
};

struct Land : PlaceCRTP<Land> {};
struct Ocean : PlaceCRTP<Ocean> {};

struct Elephant : Animal<Land> {};
struct Moose : Animal<Land> {};
struct Deer : Animal<Land> {};
struct Whale : Animal<Ocean> {};
struct Shark : Animal<Ocean> {};
struct Crab : Animal<Ocean>, Animal<Land> {};  // Note: Crab can represent Ocean or Land!

int main() {
    Land land;
    Elephant elephant;
    Whale whale;
    land.setRepresentative(&elephant);  // Done.
    land.setRepresentative(&whale);  // Cannot be done.

    std::list<IAnimal*> allAnimals, landCandidates;
    for (int i = 0; i < 10; i++) {
        allAnimals.push_back (new Elephant);
        allAnimals.push_back (new Whale);
        allAnimals.push_back (new Moose);
        allAnimals.push_back (new Shark);
        allAnimals.push_back (new Crab);
    }
    for (IAnimal* a : allAnimals)
        if (land.canUse(a))
            landCandidates.push_back(a);
    std::cout << allAnimals.size() << std::endl;  // 50
    std::cout << landCandidates.size() << std::endl;  // 30 (10 Elephants, 10 Moose, 10 Crabs)
}

One drawback I can think of right away, is that the repeated dynamic casts cost performance, but looking up a map might be slower.

The design of Place.canUse(Animal) suggests that places use animals.
But in reality it's the other way around.
The question is not if land can use a whale,
but if a whale can "use" land.
I suggest to invert the design to Animal.canUse(Place) (or possibly even canInhabit).

If performance is important,
then you can add in Animal one method for each Place type,
for example canUseLand, canUseOcean.
After all, do you seriously need to consider future expansion of place types?
It's the animal kingdom that's likely to grow.
And the canUseX methods only need to be implemented in higher level classes,
the vast majority will be happy to inherit.


Learning F# - Porting C# Function to F# I'm an advanced C# programmer learning F#. As an exercise I'm porting a function that calculates the check digit of a US ABA (routing) number. Here are 2 C# implementations:

int CalcCheckDigit(string rt)
{
    var s = new[]{3,7,1,3,7,1,3,7}
        .Zip(rt, (m,d) => m * int.Parse(d.ToString()))
        .Sum();

    return (int)Math.Ceiling(s / 10.0) * 10 - s;
}

int CalcCheckDigitOldSchool(string rt)
{
    int[] mults = new[]{3,7,1,3,7,1,3,7};
    int s = 0;
    for (int i = 0; i < mults.Length; i++)
    {
        int digit = int.Parse(rt[i].ToString());
        s += digit * mults[i];
    }

    double nextMultOfTen = Math.Ceiling(s / 10.0) * 10;
    return (int)nextMultOfTen - s;
}

And here's my crack at it with F#:

let calcCheckDigit rt =
    let s =
        rt
        |> Seq.zip [3;7;1;3;7;1;3;7]
        |> Seq.map (fun (a,b) -> a * int(string b))
        |> Seq.sum |> float
    int (ceil (s / 10.0) * 10.0 - s)

How might the F# version be improved?

This isn't really F# specific because you could do the same thing in your C# code, but converting each character to a string so you can parse it as an integer seems inefficient to me. I'd be inclined to use Char.GetNumericValue instead.

Also, you could eliminate a step in your method chain by using Seq.sumBy instead of Seq.map + Seq.sum.

I might write your code like this:

open System

let calcCheckDigit rt =
    let s =
        rt
        |> Seq.map (Char.GetNumericValue >> int)
        |> Seq.zip [3;7;1;3;7;1;3;7]
        |> Seq.sumBy (fun (a,b) -> a * b)
        |> float
    (s / 10.0 |> ceil) * 10.0 - s |> int

In my opinion, separating out the "convert a character to an integer" from the multiplication makes it easier to read. I'm using the function-composition operator to join together the Char.GetNumericValue and int functions in sequence, and then passing the resulting combined function into Seq.map.

On the last line, it's a little ambiguous just what value is being passed into ceil. It would be easy for someone unfamiliar with the code to assume that the result of the entire expression (s / 10.0) * 10.0 - s is being passed to ceil. I would be inclined to write that as I did above (obviously) but if you don't like that use of the pipe operator then I think this would also be a little more clear: int ((ceil (s / 10.0)) * 10.0 - s). Personally I'm not a fan of lots of nested parens, but to each their own.

(Before I edited this post, I mistakenly assumed exactly what I just mentioned, and so I wrote (s / 10.0) * 10.0 - s |> ceil |> int. Then I realized it wouldn't produce the same output as the C# version.)


A prefix notation calculator that asks about everything step by step I made a prefix notation calculator.

Prefix notation, aka Polish notation, is a form of notation for logic, arithmetic and algebra, like infix and postfix (reverse Polish) notation.

The expression '+ 3 4' in prefix notation is as valid as '3 + 4' in infix notation, as well as '3 4 +' in postfix notation.

The calculator asks for one value per step, and asks about everything step by step.

"""
Pfxcalc
Programmed by zyabin101
This code is hereby released into the public domain
"""

import ast

def lit_input(prompt):
    """
    Safely evaluate input. Only returns the following Python literal
    structures: strings, numbers, tuples, lists, dicts, booleans, and 'None'.

    Raises ValueError for malformed strings.
    """
    return ast.literal_eval(raw_input(prompt))

def lorr_input(prompt, result):
    inputted = raw_input(prompt)
    return result if inputted == "result" else ast.literal_eval(inputted)

# Initialization
x = None

# Print welcome message
print "Welcome to Pfxcalc. Type 'help' for more information."

# Main program loop
while True:
    # Get user input
    cmd = raw_input(">> ")
    # Process input
    if cmd == "quit":
        break
    elif cmd == "result":
        print x
    elif cmd == "help":
        helpmsg = """
        Pfxcalc is a prefix notation calculator.
        Prefix notation, aka Polish notation, is a form of notation for logic,
        arithmetic and algebra, like infix and postfix (reverse Polish)
        notation.

        The expression '+ 3 4' in prefix notation is as valid as '3 + 4' in
        infix notation, as well as '3 4 +' in postfix notation.

        Every computation starts with entering a command and the result is
        printed *and* saved in a variable that can be recalled by entering
        'result'. The variable can even be used as an argument for functions,
        using the same command.

        List of commands:
        quit - self explanatory
        result - recall the result variable
        help - print this message
        + - add two numbers or sequences
        - - subtract two numbers
        * - multiply two numbers, or a sequence by a number
        / - divide two numbers
        """
        print helpmsg
    elif cmd == "+":
        x = (lorr_input("Left side addend? ", x) +
             lorr_input("Right side addend? ", x))
        print x # Implicitly output result
    elif cmd == "-":
        x = lorr_input("Minuend? ", x) - lorr_input("Subtrahend? ", x)
        print x # Implicitly output result
    elif cmd == "*":
        x = (lorr_input("Left side factor? ", x) *
             lorr_input("Right side factor? ", x))
        print x # Implicitly output result
    elif cmd == "/":
        x = lorr_input("Dividend? ", x) / lorr_input("Divisor? ", x)
        print x # Implicitly output result
    else:
        print "Unrecognized command."

I'm interested in code style and problems the code has. What can be fixed here?

Use a shebang at the beginning

This is something like #!/usr/bin/env python2, and helps interpreters know how to run it.

Remove some of the unnecessary comments

Comments are good. Excessive comments are just hard to read.

e.g.

elif cmd == "+":
    x = (lorr_input("Left side addend? ", x) +
         lorr_input("Right side addend? ", x))
    print x # Implicitly output result

Turn that into:

elif cmd == "+":
    x = (lorr_input("Left side addend? ", x) +
         lorr_input("Right side addend? ", x))
    print x

And:

# Get user input
cmd = raw_input(">> ")
# Process input
if cmd == "quit":
    ....

Into:

cmd = raw_input(">> ")
# Process user input
if cmd == "quit":
    ....

Empty lines for readability

Using the previous example:

cmd = raw_input(">> ")
# Process user input
if cmd == "quit":
    ....

Into:

cmd = raw_input(">> ")

# Process user input
if cmd == "quit":
    ....

if __name__ == "__main__" check

This just checks if you are running this directly, or from a module import/something else.

Maybe put the while loop into a function called start() and use:

if __name__ == "__main__":
    start()

Don't assign unnecessary variables

The helpmsg var is pretty much useless, just print the plain string.

The lit_input function is not used

Either use it, or remove it. Maybe make a variation of the docstring for the lorr_input?

Other notes:

Nice job with the ast safe eval, but a method that doesn't involve eval would also be nice. This is a lot of work, probably unnecessary.
I would set x to 0 at the beginning, but that's personal preference.
I'm not sure, but you appear to have limited to 87 chars per line (or under). That is a PEP guideline, good job.


Writing an MVC project for the first time - how close am I? I've recently started programming (learning Ruby) and wrote a small game. Now I've started reading about MVC and am trying to refracture my code to follow that architecture. The only classes with big changes so far are the intended controller and view (which is completely temporary.

# This file will handle the Demigod class, which is the player controlled class
#
# =============================================================================

require_relative 'UiHandler'
require_relative 'GameData'
require_relative 'World'

class Demigod
  attr_reader :resources
  @world

  # Starts a new game - each demigod has one world to rule
  # accepts different starting resources as a parameter
  def initialize (world, resources = GameData::STARTING_RESOURCES)
    @world = world
    @resources = resources
  end

  # Handles a turn, main function of the game
  def turn

    # calculates new amount of resources using the production function
    # world.production accepts a resources hash and returns a newly
    # calculated resource hash based on production
    @resources = @world.production(@resources)
    p @resources

    #do_event # ========= TODO ========= #

    # part 2 of every turn
    UiHandler.print_world(@world)
    UiHandler.print_turn_message

    # Asks the user for a tile to do his action on
    decision = UiHandler.get_decision()

    until (@world.valid?(decision) || decision == '')
      UiHandler.print_error(UiHandler::NO_TILE)
      UiHandler.print_turn_message
      decision = UiHandler.get_decision()
    end

    unless decision == ''
      tile = @world.get_tile decision # returns the tile at decision

      # checks for legality of move on tiles using tile.accepts? and tile.check_cost
      until tile.accepts?(decision)
        break if decision == ''
        UiHandler.print_tile_options(tile)
        decision = UiHandler.get_decision()
        if (!tile.accepts?(decision))
          UiHandler.print_error(UiHandler::INVALID)
        elsif (!tile.check_cost(decision, @resources))
          decision = nil
          UiHandler.print_error(UiHandler::RESOURCES)
        end
      end

      if decision != ''
        price = GameData.get_price(decision)
        @resources = reduce(price)
        @world.advance(tile, decision)
      end
    end

    # Clears the screen
    UiHandler.clear_messages()
  end

private

  # calls an new event         # === TODO === #
  def do_event
  end

  # Reduces resources TOBEMOVED to world
  def reduce(amount)
    amount.each do |name, value|
      @resources[name] -= value
    end
    @resources
  end

end

Stuff I realized while writing this question:

resources really shouldn't be here, though I'm not sure if they deserve their own class or should just be in the world (the main model).
I think that the loop dealing with incorrect input should be part of the view, since it interacts directly with the user for input, but I'm not sure how to do it.

I would appreciate a review of how the entire code fits MVC, but I don't expect it.

This is a little abstract, without much actual code, but perhaps it's useful nonetheless.

A game is actually a little tough to apply "stringent" MVC to. It's not quite clear cut what belongs where. The world is obviously a model, resources are obviously models, and tiles are too. But is a player a controller or a model? That's more difficult.

I'd say that you shouldn't worry about what the player is. Instead you should add an overall "game controller". Its responsibility would be to update resources before and after #turn is called on a player. The game controller would also be responsible for printing the world, etc., while the player would only responsible for actually making a turn (or, given that there seems to be 2 distinct parts to a turn, make those 2 separate methods). Return a decision (or two), and let the game controller and world sort out how to change the game state (reduce resources, update tiles, etc.).

You might also wait until this point to check the validity of a decision. Let the player make a decision first, and if it's nonsense, let the player try again. The player doesn't necessarily have to know the rules of the game, as long as the game controller knows 'em.

Never mind if the player is M, V, or C. Approach it simply as a "decision maker". It may be AI or a human player, but the point is that it returns a decision. It's sort of a controller, but I'd avoid having it update models or updating the view. It's basically a service object. It could even be completely stateless: Give it a world and a set of resources, and let it come up with a decision somehow.

In terms of code review:

My gut reaction is that there's too much going on in #turn. This is irrespective of the MVC design; the method is just doing too much. (In Ruby you often aim for methods that are super, super short. If you go above 5 lines, you need a reason. Above 10 lines, and you need a very good reason).

Many, if not all, of the UIHandler methods might work better as a module. They appear state-less, so it might well be simpler to just include them where necessary.

There are also some mixed responsibilities. You have tile.check_cost(decision, @resources) but you also have price = GameData.get_price(decision). If you can get the price yourself, you might as well check the cost yourself too. The check_cost method is nicely abstract - you don't need to worry about the cost, just if you can afford it. But that's undermined by you having to fetch the price later anyway.

On a micro level: To get a decision from the command line, I'd recommend something like

def get_decision(&block)
  begin
    input = gets.chomp # or however you read input
  end until !block_given? || yield(input)
  input
end

which you then call like

decision = get_decision { |input| input.empty? || possible?(input) }

where possible? would be a method that checks tile.accepts?(input) and whether it fits the available resources.

Most of all, since there seems to be two steps in a "turn", I'd split those into separate methods. Again, have a game controller and let it call one method first, then the other.


Get count of digits of a number that divide the number So, here's a program in python to get the count of digits that divide the number. If you give the program a list like [24,10245], you get an output of:

2
2

This is the code:

def count_div_digits(l):
    for number in l:
        count=0
        digits=str(number)
        for digit in digits:
            if digit=='0':
                continue
            else:
                if number%(int(digit))==0:
                    count+=1
        print count

The above code obviously works. How can I further optimize this, make it short or do it differently?

Do one thing

Let's start with your top level:

def count_div_digits(l):
    for number in l:

Firstly, l is a terrible variable name. It looks like 1. Avoid it at all costs. Secondly, this function is less useful than it could be simply because it requires a list of numbers. It'd be better if the function took a single number, then you could call it on a list separately if you want:

def count_div_digits(number):
    ...

for number in lst:
    count_div_digits(number)

Return, don't print

Right now, your count_div_digits simply prints its result. That's all well and good, but it'd be better if you just returned the number. If you want to print it, you can always just do:

print count_div_digits(number)

And this way, you can store the results somewhere else for further analysis later.

Avoid continue:

The less weird hops your loops make, the easier it is to follow along the logic. Right now you have:

if digit=='0':
    continue
else:
    # stuff

But it'd be clearer to just do:

if digit != '0':
    # same stuff

Proposed Solution

def count_div_digits(number):
    count = 0
    for digit in str(number):
        if digit != '0' and number % int(digit) == 0:
            count += 1
    return count

Generator Expressions

You could also write the above with the help of a generator expression by factoring out a length:

def sizeof(iterable):
    return sum(1 for _ in iterable)

def count_div_digits(number):
    return sizeof(digit for digit in str(number)
                  if digit != '0' and number % int(digit) == 0)

Whether you consider that an improvement or not is a matter of opinion.


FizzBuzz in accordance with standards I wrote FizzBuzz. My aim is to use C# Code Standards correctly, and write flexible code. How can I improve my solution according to code standards? How can it be better in general?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Windows.Forms;

namespace WindowsFormsApplication2
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
        }

        private IEnumerable<int> Sequence(int startPoint, int endPoint)
        {
            return
                Enumerable
                    .Range(Math.Min(startPoint, endPoint), Math.Abs(startPoint - endPoint) + 1);
        }

        private bool IsDivisible(int number, int divisor)
        {
            return (number % divisor).Equals(0);
        }

        private static SortedDictionary<int, string> GetRules()
        {
            var rules = new SortedDictionary<int, string>
            {
                [3] = "Fizz",
                [5] = "Buzz"
            };

            return rules;
        }

        private bool IsInRules(int number)
        {
            return
                GetRules()
                    .Any(rule => IsDivisible(number, rule.Key));
        }

        private string FizzBuzz(int number)
        {
            if (IsInRules(number))
            {
                var result =
                    GetRules()
                        .Where(rule => IsDivisible(number, rule.Key))
                        .Aggregate(new StringBuilder(), (stringBuilder, rule) => stringBuilder.Append(rule.Value));

                return Convert.ToString(result);
            }

            return Convert.ToString(number);
        }

        private void button1_Click(object sender, EventArgs e)
        {
            var result =
                Sequence(1, 20)
                    .Select(FizzBuzz);

            listBox1.DataSource = result.ToArray();
        }
    }
}

This is how you end up with projects with forms with 25,000 lines of code.  It's also how you end up with duplicated code.

Your (poorly named) Form1 class should only be responsible for stuff directly related to the form it's displaying.  Otherwise, complicated forms end up with even more complicated code.

And what happens if you have another form that wants to do some of the same FizzBuzz things?  Well, now you have to copy & paste all of your code.  As soon as you start to copy & paste anything while programming, that should be a massive red flag that you're absolutely doing something wrong.  You need to take a step back and figure out how to do it without copy & pasting.

In this case, we need a separate class to hold all of our FizzBuzzing logic.  Now, any form or anything else, that wants to use our FizzBuzz code instantiates our FizzBuzz class and uses its method (although perhaps, we've implemented as a static class and we just call its methods without instantiation).


Execute edit in a stored procedure based on database value I have code below which is set to check the date of DateToComplete, and if the date is 2 weeks or more ago, change the status of Complete from 3 to 2.

Is this the best way?

USE [DB]
GO
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
Create PROCEDURE [dbo].[spChanger]

AS
BEGIN

Execute ('UPDATE [TblActions] SET Complete = 2 WHERE DateToComplete < Date.Now.AddDays(14) AND Complete = 3' )

END

Create PROCEDURE [dbo].[spChanger]

AS
BEGIN

Execute ('UPDATE [TblActions] SET Complete = 2 WHERE DateToComplete < Date.Now.AddDays(14) AND Complete = 3' )

END

Your casing is inconsistent. If you prefer UPPERCASE keywords, stick to uppercase :)

CREATE PROCEDURE [dbo].[spChanger]

AS
BEGIN

EXECUTE ('UPDATE [TblActions] SET Complete = 2 WHERE DateToComplete < Date.Now.AddDays(14) AND Complete = 3' )

END

The name of the procedure is potentially problematic. "Changer" says nothing about what's changing, and as your database grows you'll certainly end up wondering why you didn't call it something along the lines of spUpdateTblActionsCompleteStatusCode.

Why are you executing a string? Why not just do this?

CREATE PROCEDURE [dbo].[spChanger]

AS
BEGIN

    UPDATE TblActions
      SET Complete = 2
    WHERE DateToComplete < Date.Now.AddDays(14)
      AND Complete = 3

END

Now you get IntelliSense in SSMS (assuming SQL Server) and it's much harder to make a typo on a column name.

I don't think this CREATE PROCEDURE script can run though. Date.Now.AddDays(14) isn't valid T-SQL.

That said I think you're missing opportunity for some parameters. I'd do it like this:

CREATE PROCEDURE [dbo].[spUpdateTblActionsCompleteStatusCode]
    @completeStatusValue INT = 2,
    @daysDiff INT = 14,
    @completeStatusFilter INT = 3
AS
BEGIN

    UPDATE TblActions
      SET Complete = @completeStatusValue
    WHERE DATEDIFF(d, DateToComplete, DATEADD(d, @daysDiff, GETDATE())) < @daysDiff
      AND Complete = @completeStatusFilter

END

When your code runs this stored procedure, if no parameters are passed it will just use the default values, and you have the flexibility to pass different parameters if/when you need to.

Also I'd recommend scripting your T-SQL as a DROP+CREATE, so the full script would look like this:

USE [DB]
GO
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO

IF EXISTS (SELECT * FROM sys.objects WHERE object_id = OBJECT_ID(N'[dbo].[spUpdateTblActionsCompleteStatusCode]') AND type IN (N'P', N'PC'))
DROP PROCEDURE [dbo].[spUpdateTblActionsCompleteStatusCode]
GO

CREATE PROCEDURE [dbo].[spUpdateTblActionsCompleteStatusCode]
    @completeStatusValue INT = 2,
    @daysDiff INT = 14,
    @completeStatusFilter INT = 3
AS
BEGIN

    UPDATE TblActions
      SET Complete = @completeStatusValue
    WHERE DATEDIFF(d, DateToComplete, DATEADD(d, @daysDiff, GETDATE())) < @daysDiff
      AND Complete = @completeStatusFilter

END


Upload Image From URL via PHP (In a safe way) I'm trying to make a simple code to upload an image directly from a url. However I have concerns about the safety of my code since a user can submit any url. How I can make it more safe?
All the good examples I found are always related to files uploaded via a form.
So I'm having difficulties in improving this code.
$img_url = "https://i.ytimg.com/vi/wSdT-SArM2Q/maxresdefault.jpg";

function getimg($url)
{
  $headers[] = 'Accept: image/gif, image/x-bitmap, image/jpeg, image/pjpeg';
  $headers[] = 'Connection: Keep-Alive';
  $headers[] = 'Content-type: application/x-www-form-urlencoded;charset=UTF-8';
  $user_agent = 'php';
  $process = curl_init($url);
  curl_setopt($process, CURLOPT_HTTPHEADER, $headers);
  curl_setopt($process, CURLOPT_HEADER, 0);
  curl_setopt($process, CURLOPT_USERAGENT, $user_agent); //check here
  curl_setopt($process, CURLOPT_TIMEOUT, 30);
  curl_setopt($process, CURLOPT_RETURNTRANSFER, 1);
  curl_setopt($process, CURLOPT_FOLLOWLOCATION, 1);
  $return = curl_exec($process);
  curl_close($process);
  return $return;
}

$imagename = basename($img_url);
if (file_exists('./upload/' . $imagename)) {
  //
}

$image = getimg($img_url);
file_put_contents('upload/' . $imagename, $image);

I'm trying to make a simple code to upload an image directly from a
url.

You mean download.

How I can make it more safe?

Is there any reason why you want to retain the original file name, after applying the basename function ? This should protect against basic path traversal attempts like ../../
but you could as well rename the file altogether. If you are storing a record in a database, using an ID would be a good choice + adding the file extension (but not allow any file extension - see below).
This would be better than taking a chance and figuring out the possible pitfalls with your function. You are not controlling the file name and trusting user input is where the danger lies.
Not to mention that there may some creative attempts involving Unicode characters or whatever. Easier said than done, but I can't guarantee your code is 100% safe at this point.
The biggest problem here is that someone could supply the URL of a webshell with a .php extension, that your script will happily download to the upload folder. If that folder is under your website root, an attacker may then be able to call http://example.com/upload/shell.php to and take over your server. It's something you can test easily.
Because that's what your script does, it will download anything upon request.
At a minimum, that upload folder should not be under the website root. So the problem is not just the file name but the extension, and where that upload folder resides.
Also, there is no guarantee that the filename will be unique. You seem to be handling the scenario in some way that we don't know about.


Enum in PowerShell Class I am playing with learning PowerShell classes, and I have a validation situation that kind of lends itself to Enums, as shown in the first example.
With Enum

Enum pxPathType {
  FileSystem_Folder = 0
  FileSystem_File   = 1
  Registry_Key      = 2
  Registry_Property = 3
}

class PxConstant {
    # Static Properties
    static [string] $RegExPathWithWildcard = '\\\*\.\*$|\\\*|\\.\.\*$|\*\.(\?[a-zA-Z0-9\?]{2}|[a-zA-Z0-9\?]\?[a-zA-Z0-9\?]|[a-zA-Z0-9\?]{2}\?)$|\\[^\\]*\.(\?[a-zA-Z0-9\?]{2}|[a-zA-Z0-9\?]\?[a-zA-Z0-9\?]|[a-zA-Z0-9\?]{2}\?)$|\\[^\\]*\.\*$'
}

class PxPath {
    # Properties
    [string]$Type = $null

    # Constructors
    PxPath ([string]$path) {
        $this.Type = [PxPath]::PathType($path)
    }
    PxPath ([string]$path, [string]$pathType) {
        #$pathTypeEnum = try {
        $this.Type = try {
            [pxPathType] $pathType
        } catch {
            Throw "Not a valid path type: $pathType"
        }
    }

    static [String] PathType ([String]$path) {
        [string]$pathType = $null
        if ($path -match [PxConstant]::RegExPathWithWildcard) {
            $pathWithoutWildcard = $path -replace [regex]::escape($matches[0]), ''
            Write-Host $pathWithoutWildcard
        }
        return $pathType
    }
}

$pathType = 'FileSystem_Folders'
$path = try {
    [PxPath]::New("C:\", $pathType)
} catch {
    Write-Host "$($_.Exception.Message)"
}
$path = try {
    [PxPath]::New("\\Server\Folder\*")
} catch {
    Write-Host "$($_.Exception.Message)"
}

The issue I have is that the Enum is only EVER used in the PxPath class, and I feel like that means I should have the Enum in the class, so it's self contained. However, PS5 doesn't allow an Enum in a class. The workaround I have come up with is to instead use an array as a hidden Property of the class instead, as in this second example.

With Array in Class

class PxConstant {
    # Static Properties
    static [string] $RegExPathWithWildcard = '\\\*\.\*$|\\\*|\\.\.\*$|\*\.(\?[a-zA-Z0-9\?]{2}|[a-zA-Z0-9\?]\?[a-zA-Z0-9\?]|[a-zA-Z0-9\?]{2}\?)$|\\[^\\]*\.(\?[a-zA-Z0-9\?]{2}|[a-zA-Z0-9\?]\?[a-zA-Z0-9\?]|[a-zA-Z0-9\?]{2}\?)$|\\[^\\]*\.\*$'
}

class PxPath {
    # Properties
    hidden [string[]]$validPathTypes = @('FileSystem_Folder', 'FileSystem_File', 'Registry_Key', 'Registry_Property')
    [string]$Type = $null

    # Constructors
    PxPath ([string]$path) {
        $this.Type = [PxPath]::PathType($path)
    }
    PxPath ([string]$path, [string]$pathType) {
        if ($this.validPathTypes -contains $pathType) {
            $this.Type = $pathType
        } else {
            Throw "Not a valid path type: $pathType"
        }
    }

    static [String] PathType ([String]$path) {
        [string]$pathType = $null
        if ($path -match [PxConstant]::RegExPathWithWildcard) {
            $pathWithoutWildcard = $path -replace [regex]::escape($matches[0]), ''
            Write-Host $pathWithoutWildcard
        }
        return $pathType
    }
}

$pathType = 'FileSystem_Folders'
$path = try {
    [PxPath]::New("C:\", $pathType)
} catch {
    Write-Host "$($_.Exception.Message)"
}
$path = try {
    [PxPath]::New("\\Server\Folder\*")
} catch {
    Write-Host "$($_.Exception.Message)"
}

I'm wondering if there is a performance or functional argument for one over the other? Or perhaps a strong Best Practice argument in favor? Or is this coders choice really?

The best argument for using an enum in this context is the ability to:

Early input validation
Allowing users to discover valid inputs

In order to take advantage of that, your first example needs a slight change to use the enum type as the parameter type:

class PxPath {
    PxPath ([string]$path, [pxPathType]$pathType) {
        # if user input a string that doesn't correspond
        # to a [pxPathType] name, we won't even get this far!
    }
}

Now, your user has the option to pass either a [pxPathType] value or a corresponding string value:

$pxPath = [PxPath]::new("some\path", [pxPathType]::FileSystem_Folder)
# or
$pxPath = [PxPath]::new("some\path", 'FileSystem_Folder')

But this would fail even before we reach inside the constructor:

$pxPath = [PxPath]::new("some\path", 'not_a_valid_enum_name')


More efficient and readable nested loop to compare keywords I've created an algorithm which weighs the relevance of a list of articles against two lists of keywords that correlate to attributes of the article.

It works great and is super efficient... but it's a mess. It's not terribly readable, so it's difficult to discern what's going.

The operation in pseudo code goes something like this:

Loop through every article in a list called articles(List<Article>)
For every article, loop through every role in a list of roles (List<string>)
Check to see if the current article has any roles (Article.Roles = List<string>)
If yes, then loop through each role in the article and try to match a role in the article to the role in the current loop
If a match is found, add weight to the article. If the index of the role on the article and the role in the roles list are both index 0 (in primary position) add extra weight for two matching primaries
Repeat for topics, but with no bonus for primary matches

What would be a better way to write the following code? I can't use foreach except in one or two places, because I need to match indexes to know what value to add on a match.

private static List<Article> WeighArticles(List<Article> articles, List<string> roles, List<string> topics, List<string> industries)
{
    var returnList = new List<Article>();
    for (int currentArticle = 0; currentArticle < articles.Count; currentArticle++)
    {
        for (int currentRole = 0; currentRole < roles.Count; currentRole++)
        {
            if (articles[currentArticle].Roles != null && articles[currentArticle].Roles.Count > 0)
            {
                for (int currentArticleRole = 0; currentArticleRole < articles[currentArticle].Roles.Count; currentArticleRole++)
                {
                    if (articles[currentArticle].Roles[currentArticleRole].ToLower() == roles[currentRole].ToLower())
                    {
                        if (currentArticleRole == 0 && currentRole == 0)
                            articles[currentArticle].Weight += 3;
                        else
                            articles[currentArticle].Weight += 1;
                    }
                }
            }
        }
        for (int currentTopic = 0; currentTopic < topics.Count; currentTopic++)
        {
            if (articles[currentArticle].Topics != null && articles[currentArticle].Topics.Count > 0)
            {
                for (int currentArticleTopic = 0; currentArticleTopic < articles[currentArticle].Topics.Count; currentArticleTopic++)
                {
                    if (articles[currentArticle].Topics[currentArticleTopic].ToLower() == topics[currentTopic].ToLower())
                    {
                        articles[currentArticle].Weight += 0.8;
                    }
                }
            }
        }
        returnList.Add(articles[currentArticle]);
    }

    return returnList;
}

//Article Class stub (unused properties left out)
public class Article
{
    public List<string> Roles { get; set; }
    public List<string> Topics { get; set; }
    public double Weight { get; set; }
}

Note: last refactoring with rich domain model approach is at the bottom. But you can see my 3 refactoring steps which lead to nice object-oriented design.

You can use Linq to make your code more readable. Also you can give names for intermediate results:

var articlesWithRoles = from a in articles
                        where a.Roles != null && a.Roles.Any()
                        select a;

var primaryRole = roles.First().ToLower();

var highWeightArticles = from a in articlesWithRoles
                         where a.Roles.First().ToLower() == primaryRole
                         select a;

foreach(var article in highWeightArticles)
    article.Weight += 3;

var lowWeightArticles = from a in articlesWithRoles.Except(highWeightArticles)
                        from ar in a.Roles
                        join r in roles on ar.ToLower() equals r.ToLower()
                        select a;

foreach(var article in lowWeightArticles)
    article.Weight += 1;

var articlesWithTopics = from a in articles
                         where a.Topics != null
                         from at in a.Topics
                         join t in topics on at.ToLower() equals t.ToLower()
                         select a;

foreach(var article in articlesWithTopics)
    article.Weight += 0.8;

I also would recommend to use named constants instead of magic numbers 3, 1, 0.8. Also maybe you don't need to create new list, because it will share same articles as list yo passed to method.

Further improving readability could involve splitting this method into three methods:

 AddRolesWeights(articles, roles);
 AddPrimaryRoleWeights(articles, roles.First());
 AddTopicsWeights(articles, topics);

Which could look this way:

void AddRolesWeights(IEnumerable<Article> articles, IEnumerable<Role> roles)
{
    const double RoleWeight = 1;

    var nonPrimaryArticles =
        from a in articles
        where a.Roles != null && a.Roles.Any() &&
        from ar in a.Roles
        join r in roles on ar.ToLower() equals r.ToLower()
        select a;

    foreach(var article in primaryArticles)
        article.Weight += RoleWeight;
}

Yes, its less efficient. But you always should chose between readability and performance:

void AddPrimaryRoleWeights(IEnumerable<Article> articles, string primaryRole)
{
    const double PrimaryArticleWeight = 2;
    var primaryArticles =
       from a in articlesWithRoles
       where a.Roles != null && a.Roles.Any() &&
           String.Equals(a.Roles[0], primaryRole, StringComparison.CurrentCultureIgnoreCase)
       select a;

    foreach(var article in primaryArticles)
        article.Weight +=  PrimaryArticleWeight;
}

Same way you can update articles with selected topics.

Final solution

Further refactoring can include moving weights calculation logic to article class, thus making it reach instead of anemic, and increasing cohesion:

 // add this to Article class
 public double Weight { get; private set; } // probably you don't need setter

 public void AddWeights(IEnumerable<Role> roles)
 {
     const double RoleWeight = 1;
     const double PrimaryRoleWeight = 3;

     if (!roles.Any())
        return;

     if (Roles == null || !Roles.Any())
         return;

     var pirmaryRole = roles.First();
     if (String.Equals(Roles[0], primaryRole, StringComparison.CurrentCultureIgnoreCase))
     {
         Weight += PrimaryRoleWeight;
         return;
     }

     foreach(var role in roles)
        if (Roles.Contains(role, StringComparer.CurrentCultureIgnoreCase))
            Weight += RoleWeight;
 }

Adding topics weights:

 public void AddWeights(IEnumerable<Topic> topics)
 {
     const double TopicWeight = 0.8;

     if (Topics == null || !Topics.Any() || !topics.Any())
        return;

     foreach(var topic in topics)
        if (Topics.Contains(topic, StringComparer.CurrentCultureIgnoreCase))
            Weight += TopicWeight;
 }

And your original code will look like:

 foreach(var article in articles)
 {
     article.AddWeights(roles);
     article.AddWeights(topics);
 }

I think it's a little more readable now :)


Sum of numbers separated with comma I am very new to HTML and I've decided to attempt to put what I know into an HTML page that allows you to enter any amount of numbers, separated by commas, and would display the sum of all the numbers below.

<!DOCTYPE html>

  <title>Sum of two numbers</title>

Enter any amount of numbers separated by a comma:

  <input id="nums" type="text" required="required" />
  <button onclick="sum()">Get sum</button>

Are there any bad practices in my code?

HTML or XHTML? Which version? Be clear.

Your file is ambiguous. The usage of the empty element tag syntax <input ... */>* suggests (not mandates) that it's XHTML, whereas the absence of a namespace declaration suggests its not.

I recommend that documents are always clear about whether they're HTML or XHTML. And I recommend XHTML because parsing XHTML is simpler than parsing HTML, and HTML5 is defined in a way that XHTML5 documents are (more or less) a subset of HTML5 documents. The keyword is polyglot syntax.

Maybe in your context it's clear, but there is not sufficient information for us, the reviewers, whether your server would serve it as text/html or application/xhtml+xml.

Note that <!DOCTYPE html> does not sufficiently declare HTML5. The HTML5 specification says that an HTML5 document should have a doctype declaration of the form <!DOCTYPE html>. But it does not declare that everything that has such a doctype declaration is HTML5. It also does not (and cannot) prevent any other specification from also using the <!DOCTYPE html> declaration for some other variant of HTML than HTML5. Furthermore, it permits XHTML5 to also use the very same <!DOCTYPE html> declaration. See this.

I recommend to use the XHTML5 with the polyglot syntax, which is the "common subset" of HTML5 and XHTML5. It is basically HTML5 parsable with an XML parser. I recommend this for multiple reasons.

The parsing rules of XML are simpler, therefore automated processing of documents with tools such as XSLT is simpler / possible with the XML syntax.
You can create DTDs or XML Schemas that further validate your syntax. XML Schema with XML namespaces allows you to validate the mix of different XML-based languages such as for example XHTML and SVG within a single document.

(It's a pitty that the guys of WhatWG are so ignorant towards XML and its capabilities. It's a pitty that the guys of WhatWG had to reinvent the wheel and declare HTML5 as a markup language of its own right besides SGML and XML. That was completely unnecessary and it only confuses people, as we can see with this discussion about the <!DOCTYPE>.)

Declare encoding

It's recommended to always declare the encoding. And the encoding should be declared within the first 1024 bytes.

There are 4 ways how the server can declare the encoding of a (X)HTML document to the client:

HTTP Content-Type header: The server would send something like Content-Type: text/html; charset=UTF-8 or Content-Type: application/xhtml+xml; charset=UTF-8 to the client.
XML declaration (only for XHTML, not HTML), as the first line of the document: <?xml version="1.0" encoding="UTF-8"?>.
Meta element declaring the Content-Type, like this: <meta http-equiv="Content-Type" content="text/html; charset=UTF-8" /> resp. <meta http-equiv="Content-Type" content="application/xhtml+xml; charset=UTF-8" />
Meta element declaring the Charset, like this: <meta charset="UTF-8" />

See this for more information.

Document Outline

The outline of your document looks like this:

Enter any amount of numbers separated by a comma:

(level skipped)

(empty headline)

I doubt that this outline makes sense.

Keep in mind that
,
 etc. are for headlines. If you just want a paragraph with big text but without the semantics of a headline, use a normal paragraph instead and style it with CSS.

Also, skipping levels is not recommended. For a <h*N*/> element, the next heading should have a level not greater than N+1.

How to declare the scripting language

The correct way to declare the scripting language on a

If you know that the User Agent supports HTML5 or behaves as expected, you can actually omit the declaration of the scripting language:

  The default, which is used if the attribute is absent, is "text/javascript".

The lang attribute is actually meant for describing the content language, like this:

Hi, Hola, Hallo und Servus!

What you did, actually declares the contents of the


OpenMP SIMD Euclidean Distance I'm using cv::norm() to compute the L2 distance between row vectors in thousands of dimensions (at least 4096 dimensions). I'm using an AVX2 machine and icpc version 2017.

Since I'm computing this function millions of times, I need the possible most efficient version for this function and I highly doubt that cv::norm() exploits vectorization properly.

So I implemnted this function:

#include <opencv2/opencv.hpp>

float euclDist (const cv::Mat1f &first, const cv::Mat1f &second){
    float dist = 0;
    const float *firstp = first.ptr<float>(0);
    const float *secondp = second.ptr<float>(0);
    #pragma omp simd reduction(+:dist)
    for(size_t i=0; i<first.cols; i++)
        dist += std::pow(firstp[i]-secondp[i],2);
    return std::sqrt(dist);
}

int main (int argc, const char *argv[]){
    int codeSize = 4096;
    cv::Mat1f mat(1, codeSize);
    float low = 0;
    float high = 2.0;
     srand (0);
     cv::theRNG().state = cv::getTickCount() ;
    randu(mat, cv::Scalar(low), cv::Scalar(high));
    euclDist(mat, mat);
    return 0;
}

The example above is similar what the actually program does: it generates a row vector and it computes the distance with itself (i.e. 0). In the actual program generates a matrix and it randomly compute the distance between some rows. I don't think that I can get only that part of the code easily.

I already noticed an improvement w.r.t. cv::norm(), but scales badly when computing multiple distances in parallel. According to Intel VTune I fully use all the processors, but I get poor memory performance, as it shown from this general exploration analysis:

This is the result from: sudo lshw -short -C memory

H/W path       Device      Class          Description
=====================================================
/0/0                       memory         128KiB BIOS
/0/4/b                     memory         32KiB L1 cache
/0/4/c                     memory         256KiB L2 cache
/0/4/d                     memory         6MiB L3 cache
/0/a                       memory         32KiB L1 cache
/0/2a                      memory         12GiB System Memory
/0/2a/0                    memory         4GiB SODIMM DDR3 Synchronous 1600 MHz (0.6 ns)
/0/2a/1                    memory         DIMM [empty]
/0/2a/2                    memory         8GiB SODIMM DDR3 Synchronous 1600 MHz (0.6 ns)
/0/2a/3                    memory         DIMM [empty]

What I could do?

I had to adapt the test program to get meaningful results using /usr/bin/time:

#include <iostream>
int main()
{
    static const int codeSize = 4096;
    cv::Mat1f mat1(1, codeSize);
    cv::Mat1f mat2(1, codeSize);
    float low = 0;
    float high = 2.0;
    srand(0);
    cv::theRNG().state = cv::getTickCount() ;

    randu(mat1, cv::Scalar(low), cv::Scalar(high));
    randu(mat2, cv::Scalar(low), cv::Scalar(high));

    double d = 0;
    for (auto i = 0;  i < 1000000;  ++i) {
        d += euclDist(mat1, mat2);
    }

    std::cout << d << std::endl;
}

(I used two different inputs to avoid a predictable result of 0, made sure I used the result to prevent optimising out, and executed a million iterations to get the run time over a second).

Here's my compilation flags (used with my standard Makefile):

167501: PKGS += opencv
167501: CXXFLAGS += -O3 -march=native
167501: CXXFLAGS += -fopenmp

Having done that, I tried a couple of variations, all compiled with GCC version 7.1.0, and executed them on an Intel i7-6700K with 128KiB/1MiB/8MiB L1d/2/3 caches.

The original clocks in at 14.6 seconds user time without the OpenMP directive and 4.5 seconds with it.

Changing the std::pow() call to a plain multiply improved those times to 4.2 and 0.6 seconds respectively.

I got another factor of three improvement in wall-clock time, at the expense of much more processor time, by distributing across 8 cores with

#pragma omp parallel for simd reduction(+:dist)

For this code, the loop is a bit small for parallelization, it seems.

float euclDist(const cv::Mat1f& first, const cv::Mat1f& second)
{
    float dist = 0;
    const float *const firstp = first.ptr<float>(0);
    const float *const secondp = second.ptr<float>(0);
#pragma omp simd reduction(+:dist)
    for(auto i = 0;  i < first.cols;  ++i) {
        auto const d = firstp[i] - secondp[i];
        dist += d * d;
    }
    return std::sqrt(dist);
}


Append /src to an Optional path My method getProjectDir() will return an Optional which contains the path to the project directory, or an empty container:

Optional<Path> getProjectDir() { ... }

I have another method, getSrcDir() which depends on getProjectDir():

Optional<Path> getSrcDir() {
    Optional<Path> projectDir = getProjectDir();
    if (projectDir.isPresent()) {
        return Optional.of(projectDir.get().resolve("src"));
    }

    return Optional.empty();
}

Everything works, but I feel the if statement is clunky. Is there any single statement which says "If the project dir exists, append "/src" to it, else return an empty optional. Please let me know if and how it can be done.

Sure there is! You're looking for Optional.map

  If a value is present, apply the provided mapping function to it, and if the result is non-null, return an Optional describing the result. Otherwise return an empty Optional.

Optional<Path> getSrcDir() {
    return getProjectDir().map(p -> p.resolve("src"));
}


Swift format string with separator every n characters where n changes In Swift I have a string where a dash needs to be inserted after every 3 characters, if there are 4 characters or 2 characters at the end of the string they should be shown as "-xx-xx" or "-xx" respectively.

E.g "203940399345" to "203-940-399-345"
    "2039403993454" to "203-940-399-34-54"
    "20394039934546" to "203-940-399-345-46"
    "2039403993454699409399" to "203-940-399-345-469-940-93-99"

I have the following solution:

func format(_ unformatted:String) -> String {

  var formatted = ""
  let count = unformatted.characters.count

  unformatted.characters.enumerated().forEach {
    if $0.offset % 3 == 0 && $0.offset != 0 && $0.offset != count - 1 || $0.offset == count - 2 && count % 3 != 0 {
      formatted += "-" + String($0.element)
      return
    }
    formatted += String($0.element)
    return
  }
  return formatted
}

It will add a dash before each third char unless it is the first or last char or it is two from the end but only if the char count is divisible by 3 then it adds a char to the formatted string as normal.

Is there a more optimal way to achieve this formatting? What approach should I be taking with this kind of problem?

There is one small bug in your code, a two-character string is formatted with an initial dash:

print(format("12")) // -12

There are various ways to fix  that, e.g. by replacing the condition

$0.offset == count - 2 && count % 3 != 0

with

$0.offset == count - 2 && $0.offset % 3 == 2

Now some suggestions to simplify the code and make it more readable.
Instead of

unformatted.characters.enumerated().forEach {
     // ... use `$0.offset` and `$0.element` ...
}

I would use for ... in, so that we can give the loop variables names:

for (offset, char) in unformatted.characters.enumerated() {
     // ... use `offset` and `char` ...
}

There is no need to convert $0.element to a String because you can
append a character directly:

formatted.append($0.element)

There is also no need to call return at the end of the iteration block.

The condition

if $0.offset % 3 == 0 && $0.offset != 0 && $0.offset != count - 1 || $0.offset == count - 2 && $0.offset % 2 == 2

is quite complex, I would  split it into two conditions with if/else if.

Putting it all together, the function would then look like this:

func format(_ unformatted: String) -> String {

    var formatted = ""
    let count = unformatted.characters.count

    for (offset, char) in unformatted.characters.enumerated() {
        if offset > 0 && offset % 3 == 0 && offset != count - 1 {
            formatted.append("-")
        } else if offset % 3 == 2 && offset == count - 2 {
            formatted.append("-")
        }
        formatted.append(char)
    }

    return formatted
}

The condition whether to insert the separator at an offset is quite complex and
it is easy to make an error. A completely different approach would be
a recursive implementation, which is (almost) self-explaining:

func format(_ str: String) -> String {

    switch str.characters.count {
    case 0...3:  // No separators for strings up to length 3.
        return str
    case 4: // "abcd" -> "ab-cd"
        let idx = str.index(str.startIndex, offsetBy: 2)
        return str.substring(to: idx) + "-" + str.substring(from: idx)
    default: // At least 5 characters. Separate the first three and recurse:
        let idx = str.index(str.startIndex, offsetBy: 3)
        return str.substring(to: idx) + "-" + format(str.substring(from: idx))
    }

}

More suggestions:

Choose a different name instead of format() which indicates the purpose of the formatting.
Make the separator character a parameter of the function.


Checking if a string is a palindrome This is the code for checking if a string is a palindrome. The execution time, for sample cases, is 0.40358 seconds. What must be done to increase the performance?

int main(){
    int c,flag = 0;
    char *ch, *h;
    ch = (char *)malloc(sizeof(char)*101);
    h = ch;
    while((c=getchar())!=EOF){
        *ch = c;
        ch++;
    }
    ch--;
    while(ch>=h){
        if(*ch-- != *h++){flag = 1; break;}
    }
    flag == 1?(printf("NO")):(printf("YES"));
}

This is the code with an execution time of 0.4017 seconds for same test cases:

void main()
{
    char a[100],b[1000];
    int n,i,j=0,f=0;
    scanf("%s",a);
    n=strlen(a);
    for(i=n-1;i>=0;i--)
    {
        b[j]=a[i];
        j++;
    }
    for(i=0;i<n-1;i++)
    {
        if(a[i]==b[i])
        {
            f++;

        }
    }
    if(f==n-1)
    {
        printf("YES");
    }
    else
    {
        printf("NO");
    }
}

Why is the former slower than the latter? Are the library functions faster?

You can't really call a difference of 1% slower or faster. You should consider that both codes have exactly the same performance.

The only explanation for a speed difference between those two applications comes from the malloc (which is slightly slower than an array).

IMO, first code is better than second; the second one is quite hard to get (you are basically reversing a string, then comparing the reversed string with the normal one). I actually had to read the code 3 times to understand what was done.

About code review in itself:

First code sample:

You should indent your code in a consistent manner.

int c,flag = 0;
char *ch, *h;

Those variables are poorly named; flag could be replaced by is_palindromic, h could be string_start/str_start/str, and ch string_from_end/string_reverse etc....

while((c=getchar())!=EOF){

It's usually a good idea to separate comparators and equal sign from their operands, so it'd be something like this:

while((c = getchar()) != EOF){

Second code sample:

char a[100],b[1000];
int n,i,j=0,f=0;

Same as before, you should also avoid declaring 4 variables on the same line (especially considering you have both initialized and non-initialized variables).


Object Oriented programming in lua in a functional fashion I've discovered (and written) a way to object oriented programming in lua in a functional fashion instead of the standard metatable method. It carries much more functionality (no pun intended) but I'm afraid it might dent performance. My goal is to experiment with servers written in lua and I wanted to use this OOP solution. Anyways here's my class.lua which holds the functions for creating new objects and subclasses.

local classes = setmetatable({}, {__mode = "k"}) -- Allow the GC to empty this as needed.

function class(f, super)
    classes[f] = {super = super}
end

function new(f, obj, ...)
    local fenv = getfenv(f)
    if type(obj) ~= "table" then
        error("bad argument: expected table, got " .. type(obj) , 2)
    end

    if classes[f] and classes[f].super then
        new(classes[f].super, obj, ...)
        local super = obj
        obj = setmetatable({}, { __index = super })
        obj.super = super
    else
        setmetatable(obj,{__index = fenv})
    end

    obj.this = obj
    setfenv(f, obj)
    f()
    setfenv(f, fenv)

    if obj.init then
        obj.init( ... )
    end

    return obj
end

Usage is fairly simple. Take the following example:

function Person()
    local privateVar = math.random()
    age, name, gender = nil, nil, nil

    function init(age, name, gender)
        this.age = age
        this.name = name
        this.gender = gender
    end

    function getPrivateVar()
        return privateVar
    end
end

And to create the object

obj = new(Person, {}, "John", 30, "male")

Subclassing is also simple

function Female()
    function init(name, age)
        super.init(name, age, "female")
    end
end

class(Female, Person)

Note that you only have to call a function to make something a class if you want to subclass.

When you call new, you pass in the class, object, and arguments. The object's metatable is set to have an index of the class's original environment. Then the super is determined and created. Then the class function is run to create all instance values. Next, init is called.

One advantage of this over metatables are that other code can't change the class and have it change all instances of that class that already exist and that will exist. Now the only way to do something like that is to swizzle and hack around in environments to make sure all references to the class refer to the hacker's dummy class.

On top of that, it supports private variables. Just declare them locally in your class function and you're good to go.

But I'm wondering if the fact that it creates all the instance methods once for every instantiation is going to cause performance problems. Will this eat memory? What can go wrong here?

Also, there's an issue. While this supports multiple inheritance, if you call a super method, and the super calls a method, it will call the method of the super class, not the instantiated class. Any ideas how to make it call from the instantiated class?

Even though this kind of inheritance is perfectly valid example of OOP designing in language like Lua; it is considered a bad practice of programming.
According to Programming in Lua on Object-Oriented Programming.

This kind of function is almost what we call a method. However, the use of the global name Account inside the function is a bad programming practice.

This function will work only for this particular object.
Even for this particular object the function will work only as long as the object is stored in that particular global variable; if we change the name of this object, withdraw does not work any more.

The example they've used to depict is also quite to the point and small.
You should also take a look at how they've explained creation of classes; their inheritance; public, private and protected datatypes/variables and many other OOP concepts using metamethods and tables depending on their usage.
Here's the index of OOP concepts incorporated in Lua.
Useful Links

Object Orientation Tutorial
Inheritance Tutorial
Metamethods Tutorial