Playing a sound depending on the currently displayed flash card image I've developed a fully working flash card app for kids. It has a UIImageView that cycles through 26 cards (abcs) via a gesture click and a music button that will play a sound for each image. Right now I have this 100% working, but the sound plays in an IF statement that has added an additional 400 lines of code.

How I load each card into the UIImageView from a gesture tap:

        // if the tapped view is a UIImageView then set it to imageview
        if (gesture.view as? UIImageView) != nil {
            if segControl.selectedSegmentIndex == 0 && segControl2.selectedSegmentIndex == 0 {
                imageView.image = UIImage(named: "card\(number)")
                number = number % 26 + 1
            }
            else if segControl.selectedSegmentIndex == 0 && segControl2.selectedSegmentIndex == 1 {
                imageView.image = UIImage(named: "upper\(number)")
                number = number % 26 + 1
            }

Music button:

    if (imageView.image?.isEqual(UIImage(named: "card1")))! {
        do { audioPlayer = try AVAudioPlayer(contentsOf: URL(fileURLWithPath: aSound))
            audioPlayer.play()
        } catch {
            print("Couldn't load sound file")
        }
    }
    else if (imageView.image?.isEqual(UIImage(named: "card2")))! {
        do { audioPlayer = try AVAudioPlayer(contentsOf: URL(fileURLWithPath: bSound))
            audioPlayer.play()
        } catch {
            print("Couldn't load sound file")
        }
    }
    else if (imageView.image?.isEqual(UIImage(named: "card3")))! {
        do { audioPlayer = try AVAudioPlayer(contentsOf: URL(fileURLWithPath: cSound))
            audioPlayer.play()
        } catch {
            print("Couldn't load sound file")
        }
    }

I know I could set a sound array with the sound files in it, but how would I tie it back to the card? I can't set a tag property on an asset image, can I?

I'm looking for a way to shorten the current code I have.

You shouldn't determine the sound to play from the image displayed.  Instead you should use the same piece of information to set both the image name and the sound path.

Create a property of your ViewController to keep track of the card number that is being displayed:

var displayedCard = 1

and set it whenever you assign a new image to the imageView:

// if the tapped view is a UIImageView then set it to imageview
if (gesture.view as? UIImageView) != nil {
    if segControl.selectedSegmentIndex == 0 && segControl2.selectedSegmentIndex == 0 {
        imageView.image = UIImage(named: "card\(number)")
        displayedCard = number
        number = number % 26 + 1
    }
    else if segControl.selectedSegmentIndex == 0 && segControl2.selectedSegmentIndex == 1 {
        imageView.image = UIImage(named: "upper\(number)")
        displayedCard = number
        number = number % 26 + 1
    }

Then use it to create the String you pass to URL(fileWithPath:):

// create the soundPath using displayedCard
let letter = String(UnicodeScalar(96 + displayedCard)!)
let soundPath = Bundle.main.path(forResource: "\(letter)Sound.m4a", ofType:nil)!

do { audioPlayer = try AVAudioPlayer(contentsOf: URL(fileURLWithPath: soundPath))
    audioPlayer.play()
} catch {
    print("Couldn't load sound file")
}


Generalized Sql Query Function One thing I realized while going through code yesterday at my internship is that I run sql queries quite often. So I decided to write a function  to keep all of the calls in one place and reduce the number of lines of code. It doesn't cover all possible sql calls but it does cover the basic calls I believe.

So my question is, are there any coding practices I could improve upon or have violated while writing this?

EDIT: I broke the function up into four separate functions. When it comes to PDO my bosses said they did that behind the scenes with each $db_function(there is a different sequence for each server they access) that is called. I'm simply preparing the string that they manipulate. Also I've always viewed associative arrays as field value pairs so I welcome any suggestions in alternative names.

    <?
     *  INPUT:
     *  (string) Db_function: Which sql function to call
     *  (string) Table: The table to use
     *  (string/array) Fields: The values to be manipulated along with valid sql statements/modifiers(e.g DISTINCT or aliases) ,
     *                         its a string for SELECT/DELETE or an array for INSERT/UPDATE.
     *  (string) Conditional: Statements that go at the end of the query such as WHERE, ORDER BY, JOIN etc.
     *
     *  OUTPUT:
     *          TRUE for SELECT/DELETE or the query results for INSERT/UPDATE */

    function sql_Select($Db_Function,$Table, $Fields, $Conditional = NULL){

        $sql = "SELECT $Fields FROM $Table";

          if(!is_null($Conditional)){
                $sql .= $Conditional;
                return $Db_Function($sql,1,1,1);
            }
            return $Db_Function($sql,1,1,1);
    }

    function sql_Update($Db_Function,$Table, $Fields, $Conditional = NULL){

        $sql = "UPDATE $Table SET";

             foreach($Fields as $field => $value){
                $sql .= is_numeric($value) ? " $field = $value , " : " $field = '$value' , " ;
                }
                $sql = preg_replace('/,$/', '', trim($sql)); //Removes the extra ','

         if(!is_null($Conditional)){
            $sql .= $Conditional;
            return $Db_Function($sql,0,0,0);
            }
        return $Db_Function($sql,0,0,0);
    }

    function sql_Insert($Db_Function,$Table, $Fields, $Conditional = NULL){

        $Field_Name = implode(',',array_keys($Fields));
        $sql = "INSERT INTO $Table ($Field_Name) VALUES ( ";

        foreach($Fields as $field => $value){
                        $sql .= is_numeric($value) ? " $value, " : " '$value', " ;
                    }
                    $sql = preg_replace('/,$/', '', trim($sql)); //Removes the extra ','
                    $sql .= ')';

         if(!is_null($Conditional)){
            $sql .= $Conditional;
            return $Db_Function($sql,0,0,0);
            }
        return $Db_Function($sql,0,0,0);
    }

    function sql_delete($Db_Function,$Table, $Fields, $Conditional = NULL){

        $sql = "DELETE FROM $Table";

          if(!is_null($Conditional)){
                $sql .= $Conditional;
                return $Db_Function($sql,0,0,0)
            }
            return $Db_Function($sql,0,0,0)
    }

    ?>

Argggh, my eyes burn!

SQL Injections

You should understand SQL injections.  This code is likely to promote SQL injections.  Read up about prepared statements.  It looks like you are probably passing in a mysql_* function as a string.  This stops you from using prepared statements.

Please, don't use mysql_* functions in new code. They are no longer maintained and the deprecation process has begun on it. See the red box? Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial.

One Size Fits None

Your function is overly complex.  It should be broken into methods for each action.  This will remove the need for the big switch and if statements and make it more readable.  Your code will be easier to test.

This also causes you to have a variable function call $Db_Function($sql, 0, 0, 0);.  It is easy to have errors by calling non-existent functions with this.

Response To Edit: It looks better with separate functions now.

Method Naming

You really have an awful naming scheme.  Is this dictated to you by your bosses?  Personally I already think they are silly for wanting a class like this that would promote SQL Injections.  You have a mixed pascal case name aaaa_Bbbb_Cccc?  Sane choices are camelCase, UpperCamelCase, Pascal_Case, lower_pascal_case.

The methods should not have sql_ preceding them.  Their usage should be like this:

$sql = new sql; // Or whatever your class is called.
$sql->insert(/* params go here */);

It should be obvious from the object that you create that you have an sql type object.  The sql_ in front of the method names then just gets in the way.

Unused Parameter

The sql_delete method does not use the $Fields parameter and it should therefore be removed.

Magic Numbers

Magic 0's and 1's are a sign of poor design.

Consistency

Please find a consistent way to name variables.  $sql, $fields, $value are in the minority.  I dislike Pascal_Case for variables in PHP, but inconsistency is even worse.


Filtering lines in a log file using multiple regexes In python2.7:
2 for loops are always a little inefficient, especially in python. Is there a better way to write the following filter function?
It tags a line from a log file, if it is useful. Otherwise the line will be ignored. Because there are different possible interesting lines, it tries different compiled regexes for each line, until it finds one. Note that no more regexes are checked for a line, after the first one successfully matched.

def filter_lines(instream, filters):
    """ignore lines that aren't needed

    :param instream: an input stream like sys.stdin
    :param filters: a list of compiled regexes
    :yield: a tupel (line, regex)
    """
    for line in instream:
        for regex in filters:
            if regex.match(line):
                yield (line,regex)
                break

(The "tagging" is done with the regex object itself, because it can be used later on for retrieving substrings of a line, like filename and row number in an occuring error)

I wouldn’t worry about performance of the loop here. The slow thing isn’t the loop, it’s the matching of the expressions.

That said, I’d express the nested loops via list comprehension instead.

def filter_lines(instream, filters):
    return ((line, regex) for regex in filters for line in instream if regex.match(line))

Or alternatively, using higher-order list functions:

def filter_lines(instream, filters):
    return filter(lambda (line, rx): rx.match(line), itertools.product(instream, filters))


C++ Crypto: Part 2- HMAC Looking around for modern Crypto libraries.
Could not find anything good.
I know I probably did this all wrong so work with me here. There will be four different reviews for four structures that build on each other:

Hashing
Hashed Key
Password Key
Salted Challenge Response

The data structures and implementation presented in these questions are based on RFC2104 and this post on codeproject.
This review is for an implementation of HMAC. This is a technique for hashing password using a Key.
Usage Exmple:
Digest<HMac<Sha1>>      digest;
HMac<Sha1>              hasher;

hasher.hash("This is the Key", "This is the message", digest);

hmac.h
#ifndef THORS_ANVIL_CRYPTO_HMAC_H
#define THORS_ANVIL_CRYPTO_HMAC_H

#include "hash.h"

// HMAC: Keyed-Hashing for Message Authentication RFC-2104
namespace ThorsAnvil::Crypto
{

// Look in hash.h for good examples of THash
// ThorsAnvil::Crypto::Sha1
template<typename THash>
struct HMac
{
    static constexpr std::size_t digestSize = THash::digestSize;
    using Hash        = THash;
    using DigestStore = typename Hash::DigestStore;

    void hash(std::string_view key, std::string_view message, DigestStore& digest)
    {
        Hash    hasher;

        enum { BLOCK_SIZE     = 64 };

        /* STEP 1 */
        std::array<Byte, BLOCK_SIZE>   SHA1_Key{'\x00'};
        if (key.size() > BLOCK_SIZE)
        {
            hasher.hashUnsafe(key, &SHA1_Key[0]);
        }
        else
        {
            std::copy(std::begin(key), std::end(key), &SHA1_Key[0]);
        }

        /* STEP 2 */
        std::string     ipad;
        std::string     opad;

        ipad.reserve(BLOCK_SIZE + std::size(message));
        opad.reserve(BLOCK_SIZE + digestSize);
        ipad.resize(BLOCK_SIZE, '\x36');
        opad.resize(BLOCK_SIZE, '\x5c');

        for (int i=0; i< BLOCK_SIZE; i++)
        {
            ipad[i] ^= SHA1_Key[i];
            opad[i] ^= SHA1_Key[i];
        }

        /* STEP 3 */
        std::copy(std::begin(message), std::end(message), std::back_inserter(ipad));

        /* STEP 4 */
        opad.resize(BLOCK_SIZE + digestSize);
        hasher.hashUnsafe(ipad, reinterpret_cast<Byte*>(&opad[BLOCK_SIZE]));

        /* STEP 5 */
        // Moved XOR of opad to STEP 2

        /* STEP 6 */
        // Don't need to copy the hash of ipad onto opad as we hashed
        // into the correct destination.

        /*STEP 7 */
        hasher.hash(opad, digest);
    }
};

}

#endif

Avoid using the same type to store the results of plain digests and HMACs
You can't (or at least should never be able to) compare a HMAC to a plain digest. So would be good if the type system could catch that potential mistake. Instead of having a DigestStore<Hash> class that is used both for plain digests and HMACs, I would just have Digest<Hash> and HMAC<Hash> each store their own result directly.
Allow adding data to the HMAC in multiple steps
As mentioned in the review for part 1, it is not uncommon to have to add multiple, discontiguous pieces of data to be added to the HMAC, so have a member function add() that can update the HMAC. This would mean splitting the creation of the HMAC in three parts:

The key material is prepared as part of the constructor
The message is added to the hash, either in one go or using multiple function calls
The final value is calculated

I would structure the class like so:
template<typename Hash>
class HMAC {
    Digest<Hash> outer_digest;
    Digest<Hash> inner_digest;

public:
    HMAC(std::string_view key) {
        // Add key XOR opad to outer_digest
        // Add key XOR ipad to inner_digest
    }

    // Convenience constructor to do a one-shot HMAC creation
    HMAC(std::string_view key, std::string_view message): HMAC(key) {
        add(message);
        finish();
    }

    void add(std::string_view message) {
        // Add message to inner_digest
    }

    void finish() {
        // Finish inner_digest, add it to outer_digest
        // Finish outer_digest
    }

    // Something to get the bits out
    const auto &get() {
        return outer_digest.get();
    }
};

You might also want to add some way to prevent finish() from being called more than once.
Avoid unsafe operations
Your own words:

I just hate reading these badly written C++ (which is why I started this hack) projects that are crappy wrappers around C rather than using good type safety and nice clean C++ interfaces and techniques.

You want good type safety, but I also think you want good safety in general. Creating unsafe functions goes against that goal. If you store the results of a hash in a Digest object, and have a way to get a const reference to the data it stores, then you don't need a hashUnsafe() function.


Storing file input into dictionary I have written small program in Python which reads the following file and stores the result into a dictionary. I am getting expected output but I think it can be done in a better way. I am using Python 3.
Input
check_name : xa25
not run:
del_l6w_
dl_l4w_
dl_l22w_
de_l3w_
ckt_pw_
ckt_pw_
run:
inv_w_
buf_w_
End

Code
import collections
def main():
    chk_cell = "input.txt"
    chk_cell_data = collections.defaultdict(dict)
    with open(chk_cell, "r") as fchk_cell:
        check_name = ""
        not_run_flag = False
        run_flag = False
        for data in fchk_cell.readlines():
            data = data.strip()
            if not data:
                continue
            if data.startswith('check_name'):
                data = data.split(":")
                check_name = data[1].strip()
                continue
            elif data.startswith('End') or '-' in data:
                check_name = ""
                not_run_flag = False
                run_flag = False
                continue
            elif data.startswith('not run'):
                chk_cell_data[check_name]['not_run'] = []
                not_run_flag = True
                continue
            elif data.startswith('run'):
                chk_cell_data[check_name]['run'] = []
                not_run_flag = False
                run_flag = True
                continue

            if not_run_flag:
                chk_cell_data[check_name]['not_run'].append(data)

            elif run_flag:
                chk_cell_data[check_name]['run'].append(data)
    print(chk_cell_data)

if __name__ == "__main__":
    main()

Type strength
Unless you have a really (really) good reason to use a dict - such as planning to immediately serialize to JSON for some network operation - dict is a poor choice for an internal representation. Use a @dataclass.
Parsing strictness
You should be a little more strict about your parsing; your current implementation considers
check_name_for_something_that_makes_no_sense

to be a check_name heading, and a data entry under the run: heading called
check_name

to be a heading instead of a data entry. Instead, just compare the whole line.
Your example data do not show any reason for your - check to be there, so I see no reason for it to exist.
Parse state
Instead of hanging onto flags to remember parse state, you can simply assign a reference to the list currently being populated.
Explicit iteration
No need to call readlines. Just iterate over the file object itself.
Example implementation
from dataclasses import dataclass
from typing import TextIO, List

@dataclass
class RunData:
    name: str
    run: List[str]
    not_run: List[str]

    @classmethod
    def from_file(cls, f: TextIO) -> 'RunData':
        run = []
        not_run = []
        current_list = None
        name = None

        for line in f:
            line = line.rstrip()
            if line == 'run:':
                current_list = run
            elif line == 'not run:':
                current_list = not_run
            elif line.startswith('check_name :'):
                name = line.split(': ', 1)[1]
            elif line == 'End':
                return cls(name, run, not_run)
            else:
                current_list.append(line)

def main():
    chk_cell = "input.txt"
    with open(chk_cell, "r") as fchk_cell:
        chk_cell_data = RunData.from_file(fchk_cell)
    print(chk_cell_data)

if __name__ == "__main__":
    main()

prints
RunData(name='xa25', run=['inv_w_', 'buf_w_'], not_run=['del_l6w_', 'dl_l4w_', 'dl_l22w_', 'de_l3w_', 'ckt_pw_', 'ckt_pw_'])


Safe composition in python def json_response(response):
    assert response.code == 200, 'bad http response'
    return json.loads(response.body)

def custom_json_response(response):
    response = json_response(response)
    assert 'answer' in response, 'invalid custom json'
    return response['answer']

The code looks straightforward but I wonder if there any chance to rewrite it to achieve the following:

Replace json_response direct call with composition.
Code should not become a message from Sirius: everybody should still be able to understand it just without any efforts.
It should be robust (composition type safety) yet easy extensible.

Any ideas?

I would not rely on assert. If Python is started with the -O option, then assertions will be stripped out and not evaluated. Better to raise an Exception here.


List replication problem from Hacker rank Problem:

Given a list, repeat each element in the list n amount of times.

Solution:

def listReplication(num:Int,arr:List[Int]):List[Int] = {
      arr.flatMap(each => (1 to num).map(n => each))
}

Need review.

Use scala.collection.generic.SeqFactory.fill(n:Int)(elem: =>A) instead of your self written one:

def listReplication(num:Int,arr:List[Int]):List[Int] = {
      arr.flatMap(each => List.fill(num)(each))
}

Also note that listReplication is a nice candidate for generics:

def listReplication[A](num:Int,arr:List[A]):List[A] = {
      arr.flatMap(each => List.fill(num)(each))
}

That way you can also handle lists that contain something else than Int.


Rails configuration variants for production / development environments How can duplication be removed from this?

class Authentication
  PROVIDERS = if Rails.env.production?
                %w(weibo open_wechat wechat)
              else
                %w(weibo open_wechat wechat developer)
              end.freeze
end

Rails already has environment-specific configuration via the files in config/environments. As explained in the Rails Guides, you can simply do

# in config/application.rb (the default config)
config.authentication_providers = %w(weibo open_wechat wechat)

and

# in config/environments/development.rb (overrides the default)
config.authentication_providers = %w(weibo open_wechat wechat developer)

Edit: Or, as 200_success points out in the comments, you can simply add "developer" to the existing list:

# add another item to the default config instead of redefining it completely
config.authentication_providers += %w(developer)

or

config.authentication_providers << "developer"

Note that the strictest configuration is used as the default - the less strict one you have to explicitly set for a given environment (such as development or test).

You code then becomes:

class Authentication
  PROVIDERS = Rails.configuration.authentication_providers
end

Or you can simply skip the constant, since there's no need for it anymore; the list isn't being defined in class at all.

But another, and often times better, way to handle stuff like this is to use environment (ENV) variables, as explained in chapter 3 of "The 12 Factor App" (read it). You'll set the variables one way for production, another way for development, yet another for test perhaps, and so on if you have more environments.

Another great explanation, although it's now subscription-only*, is this Railscast episode. While the episode isn't brand-new anymore, the advice in it is absolutely still valid.

Anyway, one major point is that ENV configuration do not need to be included in source control, which is great if the configuration contains sensitive things like API keys or passwords. Rails already hits upon this point for the secret_key_base variable, where, in config/secrets.yml, it says simply:

# Do not keep production secrets in the repository,
# instead read values from the environment.
production:
  secret_key_base: <%= ENV["SECRET_KEY_BASE"] %>

There are many ways to set ENV vars; personally, I often use this gem, but there are many others. And of course the ENV handling built into you OS.

Again, there are many way to handle it. The basic premise is that stuff that changes from environment to environment should be accessed through the ENV. That's what it's for, after all. Again, read this.

*) Well worth the $9! Since Railscasts on indefinite - possibly permanent at this point - hiatus, you'll only be charged $9 for the first month, but there won't be a monthly charge (unless Ryan decides to start back up again). See this announcement for details.


Same method to export different txt files C# I have a basic method to export files, however I have several arrays to export and I need the files to have these names, I'm trying to make it by the enum list, however when I do it, the file isn't created and I'm wondering is there a way to combine the directory+name of array+.txt? Here's method that doesn't export nothing at all. Thank you for looking at the code.

enum exportByName { Client, Appointment, Address,Location,Historic }

        static string askPath()
        {
            var dir = "";

            do
            {
                Console.Clear();
                Console.Write("Insert path to export the txt file: ");
                dir = Console.ReadLine();
            } while (String.IsNullOrEmpty(dir));

            var userPath = Path.Combine(dir,Enum.GetName(typeof(exportByName),0),".txt");
            return userPath;
        }

I don't have much reputations to comment so trying to help for what I know and modified your code according to the need -

There are two issues I observed -

The below line in your code -

  var userPath = Path.Combine(dir,Enum.GetName(typeof(exportByName),0),".txt");

will generate the output "directory+name of array\.txt" instead of
  "directory+name of array+.txt"

You mentioned the point saying "the file is not created", actually your code
is just returning the path using Path.Combine you need to create file for each of the ENUM values using the File.WriteAllText method.

I have modified your code and placed below. Please check

class Program
    {
        enum exportByName { Client, Appointment, Address, Location, Historic }

        static string askPath(int count)
        {
            var dir = "";

            do
            {
                Console.Clear();
                Console.Write("Insert path to export the txt file: ");
                dir = Console.ReadLine();
            } while (String.IsNullOrEmpty(dir));

            var userPath = Path.Combine(dir, Path.GetFileName((Enum.GetName(typeof(exportByName), count))) + ".txt");
            return userPath;
        }
        static void Main(string[] args)
        {

            for (int i = 0; i < 4; i++)
            {
                var path = askPath(i);
                if (!File.Exists(path))  // if it doesn't exist, create

                    File.WriteAllText(path, "add text here");
            }
        }

    }

Hope this helps!


Getting a single value from the DB This is supposed to get a single value from the DB.

What should I consider next? How can I upgrade it?

public static String GetConnectionString()
        {
            return ConfigurationManager.ConnectionStrings["xxx"].ConnectionString;
        }

        public static String GetProviderName()
        {
            return ConfigurationManager.ConnectionStrings["xxx"].ProviderName;
        }

        private Object GetValue(string cmdTxt)
        {
            try
            {
                using (SqlConnection connection = new SqlConnection(GetConnectionString()))
                using (SqlCommand command = new SqlCommand(cmdTxt, connection))
                {
                    connection.Open();
                    return command.ExecuteScalar() ?? null;
                }
            }
            catch (Exception e)
            {
                try
                {
                    log.Error(string.Format("SELECT User: {0} Base: {3} SQL:{2} Error:{1}", (Session["UserData"] as UserData).id_user, e.Message, cmdTxt, Session["ClientBASE"].ToString()));
                }
                catch { log.Error(string.Format("SELECT User: {0} SQL:{2} Error:{1}", (Session["UserData"] as UserData).id_user, e.Message, cmdTxt)); }
                return null;
            }
        }

        public bool GetValueAsBoolean(string cmdTxt)
        {
            return Convert.ToBoolean(this.GetValue(cmdTxt) ?? false);
        }

        public string GetValueAsString(string cmdTxt)
        {
            return (this.GetValue(cmdTxt) ?? String.Empty).ToString();
        }

        public int GetValueAsInt(string cmdTxt)
        {
            return Convert.ToInt32(this.GetValue(cmdTxt) ?? 0);
        }

        public double GetValueAsDobule(string cmdTxt)
        {
            return Convert.ToDouble(this.GetValue(cmdTxt) ?? 0.0);
        }

public static String GetConnectionString()

You shouldn't have to use this outside of the class so make it private. Furthermore I would turn it into a property and set it from the static constructor -- that way you avoid having to query your configuration each time you want to make a new call.

This is assuming you don't do it deliberately like this so you can change the connectionstring without halting the application.

cmdTxt

What's that, commandtext? Something more descriptive would be helpful, perhaps just "command"  or "query" would be okay.

return command.ExecuteScalar() ?? null;

"if execution returns null, return null".

Might as well just return command.ExecuteScalar();.

string.Format("SELECT User: {0} Base: {3} SQL:{2} Error:{1}"

Keep the indices in ascending order -- it's what everybody expects and you won't have silly mistakes because the arguments passed in expected 0-1-2-3 instead.

Why use a try-catch and not a simple if(Session["ClientBase"] == null)? Exception handling is expensive and shouldn't be used for normal application flow.

GetValueAsBoolean
GetValueAsString
GetValueAsInt
GetValueAsDobule

I would consider making this a generic GetValue<T> and do the conversion inside that method while throwing a NotSupportedException for types that you don't want to support. The above idiom smells too much like Java and we're better than that.

Notice also the typo Dobule.


List creation, manipulation, user interaction, loops, and just winging some fun stuff for a game I'm trying to learn Python and this is my fist functioning little bit I wrote for a campaign I'm doing in D&D. I was wondering if I was doing anything wrong or if anyone had tips on how to make it more readable, function smoother, or be more interactive.

from random import randint
spells = '''Rol
#A LOT OF TEXT FROM HERE

I copied this part from roll20 website list of spells then just broke it up biased on the line breaks.
#to here...
'''

spells=spells.split('''\n\n''')

for i in [348, 332, 315, 294, 262, 224, 192, 149, 94, 44, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0]:
  del spells[i]

def ask():
  print('"So ya done goofed I see, usin magic where there not be, I have your roll and the spell out of control, just tell me what you need!"')
  while True:
    print()
    print('if you need a random spell type random or r, if you need a specific spell enter a number from 1 to '+str(len(spells)))
    need=input()
    if need.isdigit():
      need=int(need)
      if need > len(spells)  :
        print('"THERES NOT THAT MANY SPELLS YE TROLLUP"')
      elif need<1:
        print('"thee list start at ONE ye trollup"')
      else:
        print(spells[need-1])

    elif need.lower().startswith('r'):
      need=randint(0,len(spells)+1)
      need=need-1
      print(spells[need])

    else:
      print('FOLLOW MY INSTRUCTS WITH INTIGER OR RANDOM YE GOD FEARING REBEL')

ask()

Also, to get that fun little list that I made to get rid of useless strings in my spells list, I went ahead and wrote this:

guess=0
correctGuess=[]
for i in spells:
  if len(i)<15: #because I knew the strings I wanted to remove were under 15 in length
    print(i)
    x=i
    while x != spells[guess]:
      guess=guess+1
      if x==spells[guess]:
        print('That spell is '+str(guess))
        correctGuess.append(guess)

#reversed list so that when it removes the specific threads it isn't changing the length that the next thread to be removed is at
revCorrectGuess=list(reversed(correctGuess))

Then I manually copied the output list and took out the bit to make my code a little shorter.

Full code

As a beginner Pythonista, one of the easiest ways to get tips on writing Python is to run your script(s) through pep8. That said, here are some non-PEP8-related tips:

Avoid pre-processing constant data on every run. After deleting what I assume are non-applicable spells simply replace the original array with the output of print(spells).
Reusing variables like you use spells and need is a code smell - it's hard to read the code and understand immediately what the type is at any given point. Instead you can either use a separate variable to avoid confusion or (in the need case) reverse the random/int check:

if need == 'r':
    […]
else:
    try:
        spell_number = int(need)
    except ValueError:
        print("Arr or a number, dammit!")

randint "[returns] a random integer N such that a <= N <= b." So you should print(spells[randint(0, len(spells)-1)]).
Your code is screaming for a class like Spellbook to reuse within other code.
Ditto for a main function with some argparse magic to enable scripting.
An IDE like PyCharm will do spell checking for you (snarky slang not included in the last community edition).


JavaScript change image every three seconds I have a function that changes the image on a webpage every three seconds. I would like feedback on efficiency, good code practices, and anything else that can improve the quality of this code. It works, I just want any insight if it can work any better. Any and all feedback is welcomed and considered.

Slideshow Script

HTML Images accessed

The script tag default type is text/javascript and thus is not needed.
Keep your script out of the global scope. This can become a problem when more JS is added to the page and you get name conflicts. The easiest way to encapsulate your code is to wrap it in a function. In this case it would be best to wait for the page to load before starting image cycle. You can use either DOMContentLoaded or load events but if you use the first be sure that the CSS has loaded.
Drop the comments they add nothing that is not self evident in the code itself, and are but noise. If you find that the code does not contain the information needed to understand it, first can you rename to make it clear, if not then comments are the last resort.
Define a CSS class for hidden images rather than setting the style properties directly. (see example)
Use the CSS class to define style properties. Do not set style properties in the HTML document. Its just easier to manage.
Use querySelectorAll it is more flexible in the long run.
Use for of loop rather than for(;;) loops as it cleaner and requires less code.
You don't need to query for the images every 3 seconds. Do it once at the start.
Use const for variables that do not change.
The variable name x is a bad name. slideImages of slides would be more fitting. Note that I use the plural signifying that it is array, or array like.
Start with the images hidden, then ever 3 seconds all you need to do is hide one and show one.
Use the remainder operator % to cycle a value (see example)

Example


Accounts payable buckets (Current,30,44,48,60) The following method is designed to display what we owe to a vendor based on 5 buckets. Basically, an aged trial balance (totals only). It also needs to be able to display the data based on any date. It shows me what the trial balance would have looked like on April 1st. By default it displays based on the date that it is run. The if and elif branching "smells".

def _get_balances(self,ref_date=None):
        if not ref_date:
            ref_date = datetime.date.today()
        totals = [Decimal('0.0'),Decimal('0.0'),Decimal('0.0'),Decimal('0.0'),Decimal('0.0')]
        for invoice in self.invoices:
            if invoice.date:
                payments = Decimal('0.0')
                for payment in invoice.payments:
                    if payment.check.date <= ref_date:
                        payments += payment.amount
                invoice_total = invoice.amount - payments
                if invoice.date >= ref_date - relativedelta(days=29):
                    totals[0] += invoice_total
                elif invoice.date <= ref_date - relativedelta(days=30) and \
                    invoice.date >= ref_date - relativedelta(days=43):
                        totals[1] += invoice_total
                elif invoice.date <= ref_date - relativedelta(days=44) and \
                    invoice.date >= ref_date - relativedelta(days=47):
                        totals[2] += invoice_total
                elif invoice.date <= ref_date - relativedelta(days=48) and \
                    invoice.date >= ref_date - relativedelta(days=59):
                        totals[3] += invoice_total
                elif invoice.date <= ref_date - relativedelta(days=60):
                    totals[4] += invoice_total
        return tuple(totals)

I wrote methods for determining if the date falls within the desired range. This doesn't really solve the cluster of if else statements, but it is more readable.

I also used a few built-in Python functions, filter and reduce. These are tailor-made for handling these kinds of boiler-plate tasks.

I also changed if ref_date to if ref_date is None. Quoted from PEP 8

  beware of writing if x when you really mean if x is not None

Finally, I replaced your list initialization for totals to according to this SO question.

def _get_balances(self,ref_date=None):
    if ref_date is None:
        ref_date = datetime.date.today()
    totals = [Decimal('0.0')] * 5
    for invoice in self.invoices:
        if invoice.date is not None:
            payments = filter(lambda: payment.check.date <= ref_date)
            total_amount_paid = reduce(lambda: total, payment: payment.amount + total, payments, 0)
            invoice_total = invoice.amount - total_amount_paid
            if self.__invoice_date_is_before(invoice.date, ref, date, 29):
                totals[0] += invoice_total
            elif self.__invoice_date_is_between(invoice.date, ref_date, 30, 43):
                totals[1] += invoice_total
            elif iself.__invoice_date_is_between(invoice.date, ref_date, 44, 47):
                totals[2] += invoice_total
            elif self.__invoice_date_is_between(invoice.date, ref_date, 48, 59):
                totals[3] += invoice_total
            elif self.__invoice_date_is_after(invoice.date, ref_date, 60):
                totals[4] += invoice_total
    return tuple(totals)

def __invoice_date_is_between(self, invoice_date, ref_date, start_of_period, end_of_period):
    return invoice_date <= ref_date - relativedelta(days=start_of_period) and \
                invoice_date >= ref_date - relativedelta(days=end_of_period)

def __invoice_date_is_after(self, invoice_date, ref_date, end_of_period):
    return invoice_date <= ref_date - relativedelta(days=end_of_period):

def __invoice_date_is_before(self, invoice_date, ref_date, start_of_period):
    return invoice_date >= ref_date - relativedelta(days=start_of_period)


Split a file into individually allocated lines This is part of a larger project that is to run on POSIX systems. My concerns with it are the lack of comments, best practices with the filesystem, and naming things. Of course all feedback is great.

fileio.h:

#ifndef FILEIO_H
#define FILEIO_H

char **readlines(const char *path);
void freelines(char **lines);

#endif

fileio.c:

#include <unistd.h>
#include <fcntl.h>
#include <sys/file.h>
#include <sys/stat.h>
#include <stdlib.h>
#include <string.h>
#include "fileio.h"

static off_t getsize(int fd);
static char *getfile(int fd, off_t *size);
static long countlines(char *buf, off_t size);
static char *allocline(char *buf, off_t size, char **next);
static char **getlines(char *buf, off_t size);

char **readlines(const char *path)
{
    int fd, ret;
    off_t size;
    char *buf;
    char **tmp, **lns = NULL;

    fd = open(path, O_RDONLY);
    if (fd == -1)
        goto out;

    ret = flock(fd, LOCK_NB | LOCK_EX);
    if (ret == -1)
        goto out;

    size = getsize(fd);
    if (size == -1)
        goto close;

    buf = getfile(fd, &size);
    if (buf == NULL)
        goto close;

    (void)flock(fd, LOCK_UN);

    tmp = getlines(buf, size);
    if (tmp == NULL)
        goto free;

    lns = tmp;

free:
    free(buf);
close:
    close(fd);
out:
    return lns;
}

void freelines(char **lines)
{
    char **ln = lines;
    while (*ln != NULL)
        free(*ln++);
    free(lines);
}

static off_t getsize(int fd)
{
    int ret;
    struct stat sb;

    ret = fstat(fd, &sb);
    if (ret == -1)
        return -1;
    else
        return sb.st_size;
}

static char *getfile(int fd, off_t *size)
{
    ssize_t ret;
    char *tmp, *buf;

    buf = malloc(*size);
    if (buf == NULL)
        return NULL;

    ret = read(fd, buf, *size);
    if (ret != *size)
        goto fail;

    if (buf[*size - 1] != '\n') {
        tmp = realloc(buf, ++*size);
        if (tmp == NULL)
            goto fail;
        buf = tmp;
    }

    return buf;

fail:
    free(buf);
    return NULL;
}

static long countlines(char *buf, off_t size)
{
    long n = 0;
    char *cur = buf;

    #define REMAINING ((buf) + (size) - (cur))

    do {
        cur = memchr(cur, '\n', REMAINING);
        n++; cur++;
    } while (REMAINING > 0);

    #undef REMAINING

    return n;
}

static char *allocline(char *buf, off_t size, char **next)
{
    char *ln, *end;

    end = memchr(buf, '\n', size);
    if (end == NULL)
        return NULL; /* programmer error */

    #define LENGTH ((end) - (buf) + 1)

    ln = malloc(LENGTH + 1);
    if (ln == NULL)
        return NULL;
    ln[LENGTH] = 0;
    (void)memcpy(ln, buf, LENGTH);

    #undef LENGTH

    if (next != NULL)
        *next = end + 1;
    return ln;
}

static char **getlines(char *buf, off_t size)
{
    long i, n;
    char **lns;
    char *cur;

    n = countlines(buf, size);

    lns = calloc(n + 1, sizeof(char *));
    if (lns == NULL)
        return NULL;
    lns[n] = NULL;

    for (i = 0, cur = buf; i < n; i++) {
        lns[i] = allocline(cur, buf + size - cur, &cur);
        if (lns[i] == NULL)
            goto fail;
    }

    return lns;

fail:
    freelines(lns);
    return NULL;
}

Bugs

If flock() fails, you don't close the file descriptor.  Instead of goto out it should be goto close.
If the file does not end in a newline, you allocate extra space for it but you don't actually fill it in.  This means that you leave your buffer with an uninitialized last character and no null terminator.  Later, in countlines(), your call to memchr() will return NULL if it doesn't find a newline on the last line, and your program will crash.

Simplification #1

This code:

tmp = getlines(buf, size);
if (tmp == NULL)
    goto free;

lns = tmp;

could be simplified to:

lns = getlines(buf, size);

Simplification #2

Instead of allocating *size bytes and then reallocating one extra byte if you need to add an extra newline, just allocate the one extra byte to start with.

Simplification #3

Instead of defining and undefining a macro like this:

#define LENGTH ((end) - (buf) + 1)

ln = malloc(LENGTH + 1);
if (ln == NULL)
    return NULL;
ln[LENGTH] = 0;
(void)memcpy(ln, buf, LENGTH);

#undef LENGTH

just use a variable:

size_t length = end - buf + 1;

ln = malloc(length + 1);
if (ln == NULL)
    return NULL;
ln[length] = 0;
memcpy(ln, buf, length);


Boolean class using shared referenced memory To improve my knowledge I am trying to make a bool class from scratch, or derivative of. I am using MinGW g++11 compiler on a Windows 7 laptop.

#include <iostream>
#include <bitset>

class Zeroth{

    uint8_t* MEM = nullptr;

    public:
        const static uint8_t NullSet; // if data is declared sttic it is shared acros all instances, const means it can not be changed
        const static uint8_t UnitSet;

        /*
            the idea is that instead copying integers i can define a constant shared memories [ generated on a fly or in a header ]
            and then just store memory of the value that is pointed to
            so if object is present: point to memory of UnitSet
            if object isn't present: point to memory of NullSet

            therefore i have 1 8 bit space instanced and deleted with every value, but all those spaces point to the same constant memories
        */

        Zeroth():MEM(new uint8_t){ this->MEM = (uint8_t*)&Zeroth::NullSet; }
        Zeroth(int VAL):MEM(new uint8_t) {
            this->MEM = (uint8_t*)( VAL&0x1 ? &Zeroth::UnitSet : &Zeroth::NullSet );
        }
        ~Zeroth(){ delete MEM; }

        const bool isPresent() const {
            return (int)&Zeroth::UnitSet == (int)&(*this->MEM);
        }
        const bool isNotPresent() const {
            return (int)&Zeroth::NullSet == (int)&(*this->MEM);
        }

        friend std::ostream& operator<<( std::ostream& os, const Zeroth& data );
};
std::ostream& operator<<( std::ostream& os, const Zeroth &data ){
    return os << ( data.isPresent() ? 'T' : 'F');
}

const uint8_t Zeroth::NullSet = 0;
const uint8_t Zeroth::UnitSet = 1;

int main(){

    Zeroth Az(3);
    std::cout << Az << std::endl;
    std::cout << Az.isPresent() << std::endl;
    std::cout << Az.isNotPresent() << std::endl;

    Zeroth Bz;
    std::cout << Bz << std::endl;
    std::cout << Bz.isPresent() << std::endl;
    std::cout << Bz.isNotPresent() << std::endl;

    return 0;
}

And result is :

CD: F:\cppDMmods
Current directory: F:\cppDMmods
g++  -Wno-unused-variable -O2 F:\cppDMmods\test.cpp -o F:\cppDMmods\test.exe
Process started (PID=4296) >>>
<<< Process finished (PID=4296). (Exit code 0)

IF: "Exists" != "Exists" goto NOEXE
test.exe
Process started (PID=5516) >>>

T // Az printout
1 // Az "presence" :> aka true
0 // Az "negated presence" :> aka !true
F // Bz printout
0 // Bz "presence"
1 // Bz "negated presence"

<<< Process finished (PID=5516). (Exit code 0)
cmd /c del test.exe
Process started (PID=6124) >>>
<<< Process finished (PID=6124). (Exit code 0)
NO EXE FILE TO DELETE
================ READY ================

I am worried that due to my presumed lack of knowledge there might be some memory leaks, massive future bugs and etc.
Any suggestions, recommendations and pointed out issues are welcomed.

I'm sorry to be so straightforward: This is madness! Your code seems to indicate a sincere lack of several language features.

Pointer to const

You define MEM to be of type uint8_t*, but it actually points to  values of const uint8_t. To account for this difference you cast the const away in both constructors. If you'd ever (accidentally) write to the memory MEM points to, undefined behavior might ensue. So it should be at least of type uint8_t const *.

Memory leak in constructor

As you have suspected, your code leaks memory. Both constructors assign new uint8_t to MEM before overwriting it directly afterwards. Now there is no way to reference the values created on the heap with new. Both allocations are not necessary since MEM always points to one of the two static variables. Therefore you should also not try to delete them once Zeroth goes out of scope.

Since we are at the constructor: why do you accept an 32bit integer as argument when all you care about is one bit and all your internal values are stored in an uint8_t? I also find it unintuitive to treat all the even numbers as false, and all the odd ones as true (this is basically a consequence of your bit mask). Often all values except 0 are treated as truthy values (exempt of this rule are exit codes, where 0 is usually seen as success).

isPresent/isNotPresent

Those functions do not even compile on a 64bit platform, since the completely unnecessary cast to int narrows the bidwidth of the pointer type from 64bit to 32bit. Apart from that, there is a whole lot of other weird things going on here.

Let's try to break this down:

(int)&(*this->MEM)

this->MEM is a pointer. You then for whatever reason decided to dereference (*this->MEM) it to get the value it's pointing to. After that, you take the address of the value (&(*this->MEM)), which should be the same as this->MEM. And if all of this weren't enough, you then try to cram the address into an int ((int)&(*this->MEM)).

Same procedure for the value you try to compare it with:

(int)&Zeroth::UnitSet

You have the value Zeroth::UnitSet, take the address of it &Zeroth::UnitSet and then try to cram this address into a 32bit integer.

You get the same effect using

bool isPresent() const { return &Zeroth::UnitSet == this->MEM; }
bool isNotPresent() const { return &Zeroth::NullSet == this->MEM; }

without all this madness.

Memory footprint

The internal state of your class is represented by a pointer to a const uint8_t. As I have already tried to tell you in the comments, the size of the pointer is 32bit or 64bit depending on which type of architecture your compiler tries to target. This 4 to 8 times larger than the 8bit you would need to store the value. If you stick with the value, there are far less possibilities to shoot yourself in the foot.

class ZerothVal {

public:
    ZerothVal() = default;  // see https://en.cppreference.com/w/cpp/language/default_constructor

    ZerothVal(uint8_t VAL) {
        val = (VAL & 0x1 ? ZerothVal::UnitSet : ZerothVal::NullSet);
    }

    bool isPresent() const { return val == ZerothVal::UnitSet; }
    bool isNotPresent() const { return val == ZerothVal::NullSet; }

    friend std::ostream& operator<<(std::ostream& os, const ZerothVal& data);

private:
    const uint8_t NullSet = 0;
    const uint8_t UnitSet = 1;

    uint8_t val = ZerothVal::NullSet;
};

std::ostream& operator<<(std::ostream& os, const ZerothVal& data) {
    return os << (data.isPresent() ? 'T' : 'F');
}

Operator support

Since you'd like to implement some kind of bool value, I'd highly recommend implementing the Safe Bool Idiom. For you that would mean that you have to implement operator bool() for your class (see this SO post for an explanation).


Testing the same code with different input data I would like to get an advice regarding how to test the same code with different input data.

I would like to test that method operationSucceeded will be invoked for all successful status codes. Here is how I can test (with Kiwi) single status code (i.e. 200):

it(@"should call operationSucceeded", ^{
    MKNetworkOperation *op =[[MKNetworkOperation alloc] initWithURLString:@"http://example.com/api" params:nil httpMethod:@"GET"];
    [op stub:@selector(notifyCache)];
    NSHTTPURLResponse *response = [[NSHTTPURLResponse alloc] initWithURL:nil
                                                              statusCode:200
                                                             HTTPVersion:nil
                                                            headerFields:nil];
    [op stub:@selector(response) andReturn:response];
    [[op should] receive:@selector(operationSucceeded)];
    [op connectionDidFinishLoading:[NSURLConnection nullMock]];
});

For now I do next:

it(@"should call operationSucceeded", ^{
    NSArray *successCodes = @[@200, @201, @202, @203, @204, @205, @206];
    for (NSNumber *statusCode in successCodes) {
        MKNetworkOperation *op =[[MKNetworkOperation alloc] initWithURLString:@"http://example.com/api" params:nil httpMethod:@"GET"];
        [op stub:@selector(notifyCache)];
        NSHTTPURLResponse *response = [[NSHTTPURLResponse alloc] initWithURL:nil
                                                                  statusCode:[statusCode intValue]
                                                                 HTTPVersion:nil
                                                                headerFields:nil];
        [op stub:@selector(response) andReturn:response];
        [[op should] receive:@selector(operationSucceeded)];
        [op connectionDidFinishLoading:[NSURLConnection nullMock]];
    }
});

But the main problem here is that if my test fail for one of status codes it will say something like: "'Operation, should call operationSucceeded' [FAILED], expected subject to receive -operationSucceeded exactly 1 time, but received it 0 times" which is not very useful.

You can find full source code here.

Any advices are very welcome! =)

I've got one possible solution from Max Lunin:

NSArray *successCodes = @[@200, @201, @202, @203, @204, @205, @206];
for (NSNumber *statusCode in successCodes) {
    it([NSString stringWithFormat:@"should call operationSucceeded for status code %@", statusCode], ^{
        MKNetworkOperation *op =[[MKNetworkOperation alloc] initWithURLString:@"http://example.com/api" params:nil httpMethod:@"GET"];
        [op stub:@selector(notifyCache)];
        NSHTTPURLResponse *response = [[NSHTTPURLResponse alloc] initWithURL:nil
                                                                  statusCode:[statusCode intValue]
                                                                 HTTPVersion:nil
                                                                headerFields:nil];
        [op stub:@selector(response) andReturn:response];
        [[op should] receive:@selector(operationSucceeded)];
        [op connectionDidFinishLoading:[NSURLConnection nullMock]];
    });
}

It solves the problem with error explanation. It will tell something like: 'Operation, should call operationSucceeded for status code 202' [FAILED], expected subject to receive -operationSucceeded exactly 1 time, but received it 0 times

Any other more clear solutions?


SQL Query - records within the last year Bit of a complicated one for me.

I have a database full of hundreds of thousands of records, many of which are duplicated.

I need to get all records within the last year but making sure every instance of that record is within the last year, e.g. if a record is duplicated and one is older than a year this shouldn't be included.

So far I have the below...

Step 1 - find out earliest date for each record

SELECT MIN(CreateDate) AS Date, Email FROM Results R
WHERE (R.Email IS NOT NULL AND R.Email <> '')
GROUP BY R.Email

I created this as a view and called it [EarliestInteraction]

Step 2 - grab all within the last year

Note: I need records within the last year but they also need to be in a log table also. So all records within the last year that are also present in some log tables.

So far I have done this...

SELECT * FROM EarliestInteraction ECI
WHERE ( CAST(ECI.Date AS DATE) >= CAST(GETDATE() - 365 AS DATE) )
AND (

    EXISTS (
        SELECT Id FROM LOG1 R
        WHERE Source = 'LOGGED'
        AND R.Email = ECI.Email
    )

    OR

    EXISTS (
        SELECT Id FROM LOG2 R WHERE (R.Email IS NOT NULL AND R.Email <> '')
        AND R.Email = ECI.Email
            AND R.EventType IN (
            'LOGGED'
    ))
)

My question is, is this a good way of doing this and accurate?

Or am I missing something that would bring back earlier duplicates...

Any thoughts on if this is accurate or achieves the brief would be great.

The following are a few suggestions on how I'd write your view.
Source Control
If you don't already have a database project, create one in Visual Studio. Then check it in to source control. Microsoft Azure DevOps Services is free & private for teams of 5 or less (this is per project, so 5 developers per project). Then you'll be able to track changes you make to your stored procedures, views, tables, etc.
Formatting
I would download the following tool for SSMS and Visual Studio, Poor Man's T-Sql Formatter and on GitHub. I use it when I have to edit other developer's code. It's a great way to standardize your SQL. I find it does most of the formatting for me, but I'll still make a few changes after.
Here are the settings I used:

Commas
I would put the commas in front to clearly define new columns. Versus code wrapped in multiple lines. It also makes trouble-shooting code easier.
Where Clause
If you put 1=1 at the top of a WHERE condition, it enables you to freely change the rest of the conditions when debugging a query. The SQL query engine will end up ignoring the 1=1 so it should have no performance impact. Reference
Common Table Expressions (CTE)
CTE's in your SQL help with documentation. The expression name can then let other developers know why you used that expression e.g. current_suppliers or active_projects.
Schema Names
Always reference the schema when selecting an object e.g. [dbo].[SalesTable].
Estimated Execution Plan
It's a good idea to check the Estimated Execution Plan. The shortcut in Microsoft SQL Server Management Studio (SSMS) is Ctrl + L. You can even run 2 queries in the same tab to compare the plans.
Keywords
Avoid using keywords as object names. Microsoft Reference

Also check out the book Clean Code. It will change the way you think about naming conventions.

Revised SQL
Without table definitions and sample records I was unable to test this, but it should give you a good start.
WITH
earliest_interaction
AS
(
    SELECT
          [min_createdate] = CAST(MIN(r.[CreateDate]) AS DATE)
        , r.[Email]
    FROM
        [dbo].[Results] AS r
    WHERE
        1=1
        AND (r.[Email] IS NOT NULL AND r.[Email] != '')
    GROUP BY
        r.[Email]
)
,
log_files
AS
(
    SELECT [Email] FROM [dbo].[LOG1] WHERE [Source] = 'LOGGED'
    UNION
    SELECT [Email] FROM [dbo].[LOG2] WHERE [Source] = 'LOGGED'
)
SELECT
      ei.[min_createdate]
    , ei.[Email]
FROM
    earliest_interaction AS ei
    INNER JOIN log_files AS lf ON lf.[Email] = ei.[Email]
WHERE
    1=1
    AND (ei.[min_createdate] >= DATEADD(YEAR, -1, GETDATE()))


Wrapper for commonly used database classes The main idea behind this class is to create a wrapper for commonly used Database classes. I'm new to SOLID principles and I'm trying to implement them in this library. Does this code adhere to SOLID principles?

//This is the opens the SQL Connection and is placed in DbConnectionSetup.cs

 public interface IConnectionProvider
{
    SqlConnection OConnection(string ConnectionString);
}

public abstract class DatabaseAttributes
{
    private string _connectionString = null;

    private string _command = null;

    public virtual string ValidateConnectionString
    {
        get
        {
            return this._connectionString;
        }
        set
        {
            if (!String.IsNullOrEmpty(value))
            {
                _connectionString = value;
            }
            else
            {
                throw new Exception("No Connection String property detected.");
            }
        }
    }

    public virtual string ValidateSqlCommand
    {
        get
        {
            return this._command;
        }
        set
        {
            if (!String.IsNullOrEmpty(value))
            {
                _command = value;
            }
            else
            {
                throw new Exception("No Command String detected.");
            }
        }
    }
}

[Description("This opens the Sql Connection")]
public class SqlOpenConnection : DatabaseAttributes, IConnectionProvider
{
    public SqlConnection OConnection(string ConnectionString)
    {
        ValidateConnectionString = ConnectionString;

        var _dbConConnection = new SqlConnection(ValidateConnectionString);

        try
        {
            if (_dbConConnection.State != System.Data.ConnectionState.Open)
            {
                _dbConConnection.Open();
            }
        }
        catch (SqlException ex)
        {
            throw new Exception("SqlException thrown", ex);
        }

        return _dbConConnection;
    }
}

/* I have a different class called SqlOps.cs which has the following code:*/

public interface ICommandWithParams
{
    int ExecuteCommandWithParams(string ConnectionString, string Command, Dictionary<string, SqlParameter> Params);
}

//have one function to Insert,Update and delete

public class ExecuteComplexCud : DatabaseAttributes, ICommandWithParams
{
    public int ExecuteCommandWithParams(string ConnectionString, string Command, Dictionary<string, SqlParameter> Params)
    {
        int rc;
        IConnectionProvider _dbCon = new SqlOpenConnection();//Get OConnection from DbConnectionSetup.cs

        ValidateSqlCommand = Command;//Check for Empty Command
        try
        {
            using (SqlCommand _sqlCmd = new SqlCommand(ValidateSqlCommand, _dbCon.OConnection(ConnectionString)))
            {
                _sqlCmd.CommandType = CommandType.Text;
                foreach (var keyValuePair in Params)
                {
                    _sqlCmd.Parameters.Add(keyValuePair.Value);
                }
                rc = _sqlCmd.ExecuteNonQuery();
            }
        }
        catch (SqlException)
        {
            throw;
        }
        catch (Exception)
        {
            throw;
        }
        finally
        {
            //Close the connection and dispose the object
            IConnectionProvider _dbCloseCon = new SqlCloseConnection();
            _dbCloseCon.OConnection(ConnectionString);
        }

        return rc;
    }
}

First of all, it's unclear to me what exactly you want to achieve with this code. Database access, yes, but what benefits should your code provide? Why don't you want to use SqlConnection (or IDbConnection) and its associated classes directly?

Without knowing what sort of abstraction level you want to provide (and why), I think adherence to SOLID principles is a moot point. But here's what I think:

Overall design

It looks like you're taking the 'S' in SOLID a bit too far. Writing a separate class for each possible operation can be useful in certain situations, but here it only seems to complicate things. It also looks like your code doesn't actually work correctly: the SqlConnection returned by SqlOpenConnection.OConnection is never passed to that SqlCloseConnection instance, so how can it close that connection? And why would you need to create a new object to close an existing connection anyway? That's not intuitive.

Note that using a SqlConnection directly allows you to use a using statement, which would result in cleaner (and correct) code. I don't see any added value in those SqlOpenConnection and SqlCloseConnection classes. A single Database class that offers various methods for storing and retrieving data would be less complicated, and still serve a single purpose (database access).

I don't see any code that's using your interfaces (except inside ExecuteComplexCud.ExecuteCommandWithParams, but that seems to be an implementation detail of your library), so it's hard to tell how useful those interfaces really are - and thus how well you're taking the 'I' in SOLID into account.

Exception handling

You don't need to catch an exception if you're just going to throw it.

The type of an exception is a useful piece of information. SqlException indicates that the problem is related to database access. By wrapping that in a generic Exception, calling code is no longer able to distinguish database related exceptions from other exceptions. Wrapping can be useful, for example if you want to hide implementation details. In that case, your library may want to provide its own exception type, such as DatabaseException. Also think about what information the caller wants or needs in order to handle such exceptions.

Other notes

Use methods instead of properties to validate something. Properties are used to store or retrieve information. Using them otherwise will make your code harder to understand and maintain.

Another reason to remove the ValidateConnectionString and ValidateSqlCommand properties is that there doesn't seem to be any reason to store that information - and certainly not in multiple locations (SqlOpenConnection, SqlCloseConnection, ExecuteComplexCud).

Note that validation implies that the syntax or structure of the connection or command string will be checked. A simple null-or-empty check does not live up to that expectation. Either perform a full validation, or just remove those validation methods and use string.IsNullOrEmpty instead. And since the connection and command strings are passed as arguments, it's better to throw an ArgumentException or ArgumentNullException if they're invalid.

OConnection is not a descriptive name. I assume it's short for OpenConnection? If so, SqlCloseConnection.OpenConnection doesn't make any sense.

You may also want to add documentation. Describe the purpose of each interface and method, what input they consider invalid (and how they will react to that), what edge cases need to be taken into account and which exceptions they can throw.

Regarding '//have one function to Insert,Update and delete', do those methods have their own class or interface, or are they part of one of the interfaces you have shown here?

Conclusion

Without knowing the reasoning behind your design decisions, I'd say this code is overly complicated. It's making writing a correct program harder instead of easier. As for SOLID principles, I think there's too little context shown here to determine how well this code is adhering to them.

I hope you found this helpful. Feel free to add more context or explanation to your post if you want to receive more accurate feedback.


Recursively list files within a directory I have the following code to recursively list files within a directory:

from fnmatch import filter
from os      import path, walk

def files_within(directory_path):
  files_within = []
  for root, directory_path, files_path in walk(directory_path):
    for file_path in filter(files_path, "*"):
      files_within.append(path.join(root, file_path))
  return files_within

I want change it to avoid side affects. Is it possible?

I'm not using the glob because I'm using Python 2.5 because of a legacy code.

Your function could be a generator (which were introduced in Python 2.2). It should also expose the pattern for the filter as a parameter:

import os
import fnmatch

def files_within(directory_path, pattern="*"):
    for dirpath, dirnames, filenames in os.walk(directory_path):
        for file_name in fnmatch.filter(filenames, pattern):
            yield os.path.join(dirpath, file_name)

if __name__ == "__main__":
    # You can simply iterate over the generator:
    for file_path in files_within("."):
        print file_path

    # If you absolutely need a list:
    file_list = list(files_within("../"))

    # Use the pattern parameter:
    text_files = list(files_within(".", "*.txt"))

I like to leave the names from os.walk the same as in the documentation, but this is a personal choice.

What is not quite a personal choice is the number of tabs per indentation level. Python's official style-guide, PEP8, recommends using always 4 spaces.

I also changed the way of the imports. Here it already becomes obvious why importing them like this is better (as far as readability goes). It is obvious that fnmatch.filter is from the module fnmatch and not the built-in filter.


Parse a JSON file in Java I am using Java to parse a JSON file into a hash map so that i can search by typing a date and then get the cost, tax and profit for that date.

So that if i write:

2001-09-18 13:11:01

I get:

7.14, 1.81 and 31.10

Here is part of the JSON file:

{
  "stat":[
    {
      "date":" 2001-09-18 13:11:01 ",
      "cost":" 7.14 ",
      "tax":" 1.81 ",
      "profit":" 31.10 "
    },
    {
      "date":" 2001-09-18 14:15:02 ",
      "cost":" 7.80 ",
      "tax":" 0.99 ",
      "profit":" 30.20 "
    }
  ]
}

And my code using Jackson:

public static void main(String[] args) throws JsonGenerationException {
        try {
            ObjectMapper mapper = new ObjectMapper();
            JSONParser parser = new JSONParser();
            JSONObject jsonobj = (JSONObject) parser.parse(new FileReader("test.json"));
            JSONArray jsons = (JSONArray) jsonobj.get("stat");
            Map<Integer, Map<String, String>> jsonmaps = new HashMap<Integer, Map<String, String>>();
            int i=0;
            for (Object j : jsons) {
                Map<String, String> map = new HashMap<String, String>();
                JSONObject o = (JSONObject) j;
                map = mapper.readValue(j.toString(), new TypeReference<Map<String, String>>() {
                });
                jsonmaps.put(i, map);
                i++;
            }
            System.out.println(jsonmaps.get(0).get("date"));

        } catch (IOException e) {
            e.printStackTrace();
        } catch (ParseException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }

The initial value of a map variable is not used, you can avoid this assignment and inline the variable. o is not used too.

If indexes in a map is consequent then it is better to use a List instead of Map. So the part of the code can be rewritten following way:

List<Map<String, String>> jsonmaps = new ArrayList<>();
for (Object j : jsons) {
    jsonmaps.add(mapper.readValue(j.toString(),
        new TypeReference<Map<String, String>>() {}));
}


Python script to query REST API and write to database I wrote the following sample script for learning purposes:

import requests
import json
import pyodbc
import os
import sys

source_url = 'https://jsonplaceholder.typicode.com/posts/'

query_posts_response = requests.get(source_url)

if (query_posts_response.status_code == 200):
    posts = json.loads(query_posts_response.content.decode('utf-8'))

db_connection = pyodbc.connect("Driver={SQL Server Native Client 11.0};"
    "Server=(local);"
    "Database=sandbox;"
    "Trusted_Connection=yes;")

sys_devnull = open(os.devnull, 'w')
sys_stdout_original = sys.stdout
sys.stdout = sys_devnull

db_cursor = db_connection.cursor()

for post in posts:
    db_cursor.execute("INSERT INTO dbo.Posts(id, title) VALUES (?, ?)", post['id'], post['title'])

db_connection.commit()
db_connection.close()

sys.stdout = sys_stdout_original

I'm grabbing content from a REST API, and I'm inserting it into a relational table.

I write scripts in other languages, but this is my first Python script. I don't know what I don't know. I'm looking for a critique. I realize I was lazy on the database call and didn't create an exception handler. That said, is this the generally accepted way to interface with a REST endpoint and interact with a database?

Concerning output of db_cursor.execute(), see the following:

Some suggestions:

A linter (or two) such as pycodestyle or flake8 will help you write more idiomatic Python. For example, brackets are not required around an if statement.
You can use query_posts_response.json() to easily get JSON response content.
I can't see a reason why this script should do anything with the input and output streams.
db_connection.close() should be in a finally block. The try block should start with creating the connection.
If you ever intend to reuse the code it should be wrapped inside at least a main function (which orchestrates argument parsing, handling exceptions which should lead to non-zero return codes, and the like) and a separate function to do the work.


Display A Random Tic-Tac-Toe Board Using Java Swing This is the problem:

(Game: display a tic-tac-toe board) Display a frame that contains nine labels. A label may display an image icon for X or and image icon for O. What to display is randomly decided. Use the Math.random() method to generate an integer 0 or 1, which corresponds to displaying an X or O image icon. These images are in the files x.gif and o.gif)

My code seems to do what the problem asks, and the output is practically the same as the example (minus the borders). However, without the solutions for odd-numbered exercises, I can't imagine what an "ideal" implementation of this problem would look like. For example, is it good practice to use arrays in the way that I did?

Here is my solution:

import javax.swing.*;
import javax.swing.border.*;
import java.awt.*;

public class DisplayTicTacToe extends JFrame {
public DisplayTicTacToe() {
    setLayout(new GridLayout(0, 3));

    ImageIcon[] display = new ImageIcon[3];
    display[0] = new ImageIcon("image/o.gif");
    display[1] = new ImageIcon("image/x.gif");
    display[2] = new ImageIcon("Empty Icon");
    for (int i = 0; i < 9; i++) {
        JLabel jlb = new JLabel(display[(int)(Math.random() * 3)]);
        jlb.setBorder(new LineBorder(Color.BLACK, 1));
        add(jlb);
    }
}

public static void main(String[] args) {
    DisplayTicTacToe frame = new DisplayTicTacToe();
    frame.pack();
    frame.setTitle("Display TicTacToe");
    frame.setLocationRelativeTo(null);
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    frame.setVisible(true);
}
}

Math.random is not very intuitive to use. Random makes it much easier. Create an instance of Random somewhere before the main loop, and generate the random indexes with random.nextInt(3) for values 0 1 2.

It's fine to use an array like you did to pick from the values randomly. When there are only few choices, like in this example, another option is to use a switch.

"display" is not a great name for an array of icons. icons would seem more natural.

A simple way to initialize an array:

ImageIcon[] icons = {
    new ImageIcon("images/x.gif"),
    new ImageIcon("images/o.gif"),
};


Regex matching keys in a Dictionary I have extended this class http://wiki.unity3d.com/index.php/CSharpMessenger_Extended to accept strings containing wildcards as eventTypes, therefore I am able to write things like

Messenger.AddListener("*", () => Console.WriteLine("woohoo, generic handler"));

But I am highly concerned about the performance of this implementation.

I've extended the Dictionary class like this:

public class EventDictionary : Dictionary<string, Delegate>
    {
        public Delegate[] GetHandlers(string Event)
        {
            List<Delegate> matches = new List<Delegate>();
            foreach (string pattern in Keys)
                if (Regex.IsMatch(Event, Regex.Escape(pattern).Replace( @"\*", ".*" ).Replace( @"\?", "." )))
                    matches.Add(this[pattern]);
            UnityEngine.Debug.Log(matches.Count);
            return matches.ToArray();
        }
    }

As you see, for every key in the dictionary, I first convert it to a regular expression, then check if it is a match for the event name, if it's so, add it to a collection and return the array.

But, this code runs in a game where the messenger script is used frequently, therefore I am really concerned about the performance of this approach.

I think storing regex's directly in the dictionary would be better but my main concern is about the regex matching.

Is my solution acceptable? If not what would be a better approach for solving my problem?

Here are some problems I see with your code:

The code removes the Dictionary's constant-time lookup.
The code reparses the pattern and recompiles each regex every time you're
in the loop.
The code uses regex's even for simple lookups.
The code will die if you try to assign more than one handler to an event.
This might just be an artifact of you copying/pasting, but the indenting on your method is a little unusual.
The code does not favor composition over inheritance.
Your parameter name does not follow the convention of being camelCase.
Your regex pattern doesn't require the beginning and ends, so e.g. as?f matches the same things as *as?f*.

I would use the built-in Dictionary lookup functionality for simple (no-wildcard) lookups and store compiled regex's for the ones with wildcards. Also, you can look at the reference source in your link to get an idea of how to solve the problem of multiple handlers for an event.

This should be a good starting point, with code to add a listener and get handlers. It only uses regex's when needed, keeping them precompiled in a second Dictionary. If the "*" case is pretty common, you could further improve this by handling that as a special case: you don't really need to compile and store a Regex for it, you just always run it. This would probably be a third field, maybe private Delegate universalHandlers;

public class EventDictionary
{
    private readonly Dictionary<string, Delegate> eventTable = new Dictionary<string, Delegate>();
    private readonly Dictionary<string, Regex> regexTable = new Dictionary<string, Regex>();

    private bool HasWildcards(string pattern)
    {
        return pattern.Contains("*") || pattern.Contains("?");
    }

    public void AddListener(string eventType, Delegate eventDelegate)
    {
        Delegate currentDelegate;
        if (eventTable.TryGetValue(eventType, out currentDelegate))
        {
            var d = eventTable[eventType];
            if (d != null && d.GetType() != eventDelegate.GetType())
                throw new ArgumentException(string.Format("Attempting to add listener with inconsistent signature for event type {0}. Current listeners have type {1} and listener being added has type {2}", eventType, d.GetType().Name, eventType.GetType().Name));
            eventTable[eventType] = Delegate.Combine(currentDelegate, eventDelegate);
        }
        else
        {
            eventTable.Add(eventType, eventDelegate);
        }

        if (HasWildcards(eventType) && !regexTable.ContainsKey(eventType))
            regexTable.Add(eventType, new Regex("^" + Regex.Escape(eventType).Replace(@"\*", ".*").Replace(@"\?", ".") + "$"));
    }

    // TODO implement remove method

    public Delegate GetHandlers(string eventType)
    {
        Delegate handler;
        eventTable.TryGetValue(eventType, out handler);
        foreach (var pair in regexTable)
            if (pair.Value.IsMatch(eventType))
                handler = Delegate.Combine(handler, eventTable[pair.Key]);
        UnityEngine.Debug.Log(handler.GetInvocationList().Length);
        return handler;
    }
}


Hello World, Phrancis I just yesterday started learning some Java. I wrote this piece and got some help from @Simon André Forsberg to refine it some. I added a few things since. It's super-simple but I just want to make sure I have a few concepts down before I move on to more complex things. Any constructive criticism is appreciated!

import java.lang.*;

public class HelloWorld {
    private final String output;

    public HelloWorld(String name) {
        output = "Hello World, " + name + "!";
    }

    public String getMessage() {
        return output;
    }

    public static void main (String args[]) {
        HelloWorld helloObj;
        helloObj = new HelloWorld("Phrancis");
        System.out.println(helloObj.getMessage());
    }
}

Console output:

Hello World, Phrancis!

First things first:

import java.lang.*;

This import is superfluous. Java by default imports the java.lang package, you don't have to specify any further things.

You seem to be using consistent two newlines between last } and the next opening brace. I personally find this to take a lot of space and I'd reserve "large" vertical distance to signify sectioning or similar, it's consistent ;)

public static void main (String args[]) {

While this is a perfectly valid declaration for main, I'd prefer to have the array declaration behind the type and not behind the argument name for consistency's sake:

public static void main (String[] args) {


Insertion and selection sort in PHP I've implemented InsertionSort and SelectionSort in PHP and tested it with an array of 20.000 unique integers.

It took many seconds to complete the insertion selection sorts.  For comparison, I saw a Java implementation that only took 3 seconds for both combined. I wanted to ask if my implementations are just bad or if this is a hardware/language based problem.

Here are my methods:

$rand = randomArray(20000);
insertionSort($rand);
selectionSort($rand);

function insertionSort($sort_array){
    $time_start = microtime(true);
    for ($i = 0; $i < count($sort_array)-1; $i++){
      for ($j = $i + 1; $j > 0; $j--)
        {
          if($sort_array[$j-1] > $sort_array[$j]){
            $temp = $sort_array[$j-1];
            $sort_array[$j-1] = $sort_array[$j];
            $sort_array[$j] = $temp;
          }
        }
    }
    $time_end = microtime(true);
    $time = $time_end - $time_start;
    echo $time;
    return $sort_array;
}

function selectionSort($sort_array)
{
    $time_start = microtime(true);
    for ($i = 0; $i < count($sort_array) - 1; $i++)
    {
        $min = $i;
        for ($j = $i + 1; $j < count($sort_array); $j++)
        {
            if ($sort_array[$j] < $sort_array[$min]){
              $min = $j;
        }
      $temp = $sort_array[$i];
      $sort_array[$i] = $sort_array[$min];
      $sort_array[$min] = $temp;
    }
    $time_end = microtime(true);
    $time = $time_end - $time_start;
    echo $time;
    return $sort_array;
    }
}

function randomArray($max){
    $random = range(0, $max-1);
    shuffle($random );
    return  $random;
}

One possible inefficiency is that you are copying the array when you call the function.  If you pass parameters by reference, then you could avoid allocating and copying the array.  See the difference in these two functions that just perform one swap:

function f($a) {
    $swap = $a[0];
    $a[0] = $a[1];
    $a[1] = $swap;
    return $a;
}

function g(&$a) {
    $swap = $a[0];
    $a[0] = $a[1];
    $a[1] = $swap;
}

$xyz = array('x', 'y', 'z');
print_r($xyz);                  # array { [0] => x  [1] => y  [2] => z }
$yxz = f($xyz);
print_r($xyz);                  # array { [0] => x  [1] => y  [2] => z }
print_r($yxz);                  # array { [0] => y  [1] => x  [2] => z }
g($xyz);
print_r($xyz);                  # array { [0] => y  [1] => x  [2] => z }

That aside, there is not that much you can do to optimize your two sorting algorithms.  You could try obtaining the length count($sort_array) just once, before the loop, to see if makes any difference.

The important thing to note, though, is that both insertion sort and selection sort are O(n2) algorithms.  That means that if you double the size of the array, it will take approximately quadruple the amount of time to complete the sort.  If you care about performance when sorting large lists, you don't use insertion sort or selection sort.  You pick a more efficient algorithm that tends to work in O(n log n) time, such as quicksort.

Comparing performance across languages is not a very useful way to judge the quality of your code.  To make a comparison that is at least somewhat fair, try racing against PHP's own array_sort() function.


Write a function to find the first non-repeated character in a string Here is a practice question I am solving:

  Write a function to find the first non-repeated character in a string.
  For instance, the first non-repeated character in 'total' is 'o' and
  the first non-repeated character in 'teeter' is 'r'.

How can I improve the efficiency of this algorithm?

function repeater(string){
    var charCount = {};
    for(var i = 0; i < string.length; i++){
        if(charCount[string[i]]){
            charCount[string[i]] = 'More Than One';
        } else {
            charCount[string[i]] = 'One Time';
        }
    }
    for(var j = 0; j < string.length; j++){
        if(charCount[string[j]] === 'One Time'){
          return string.charAt(j);
        }
    }

    return 'Everything is repeated';
}

http://repl.it/QUf/1

I also solved this using a nested loop:

var nonRepeater = function(str) {
  var index = [];
  var count;
  str.split('').forEach(function(letter, i) {
    count = 0;
    str.split('').forEach(function(latter) {
      if (letter === latter) {
        count += 1;
      }
    });
    index.push(count);
  });
//   console.log(index.indexOf(1));
  return str[index.indexOf(1)];
};

http://repl.it/QVI/2

I am trying to find a way to increase the efficiency of this algorithm.  I am toying with ways to use a RegEx.

Does anyone know how to write this more efficiently in JavaScript?  I have found a few guides in C but I do not know the language well.

After playing a lot with jsperf and having to admit that the regex solution is actually faster, which annoys me.

Your first approach is far superior than your second approach (O(2n) -> O(n^2)) as per Venu
you should cache string.length, looking up the value of a property slows things down

for(var i = 0, length = string.length; i < length; i++){

You can assign More than one and One time with a ternary, also you should cache string[i] and not look it up 3 times:

charCount[c] = charCount[c] ? 'More Than One' : 'One Time';

You do not need var j, just use i again
You used string[i] everywhere else, your return statement should be return string[i];
repeater is a terrible name if you are planning to return a non-repeater ;)
from a design perspective, I would return '' instead of 'Everything is repeated', because really, '' aka nothing is repeating

On the whole I think your code is fine, I am not sure ( besides the magical regex ) how it could be done much faster. You need something to track the character count and I am not sure how you can avoid the second loop.

Update: books.google.com/books?isbn=1118169387 <- This pretty much agrees that your first approach is as fast as it gets ( except for the mind boggling regex ;)


Is this a good use of Async in F#? I've hacked together some code to read the content from a sequence of files:

let fileContents =
    [ "filename1"; "filename2"; "etc" ]
    |> Seq.map(fun file -> async {
        use fs = new FileStream(file, FileMode.Open, FileAccess.Read, FileShare.Read)
        let data = Array.create(int fs.Length) 0uy
        let! _ = fs.AsyncRead(data, 0, data.Length)
        return data
    })
    |> Async.Parallel
    |> Async.RunSynchronously
    |> Seq.map System.Text.Encoding.ASCII.GetString

It works, but I'm not really sure about it. For one thing, is async actually giving me any benefit here? Also inside the async function I need to wait for AsyncRead to finish, but I don't need the returned value from it. Is there a better way of doing this than let! _ = ...?

You can take advantage of async to do parallel IO which Async.Parallel makes very easy. Also, when you don't need the return value of an async call you can use Async.ignore in combination with do!: do! fs.AsyncRead(data, 0, data.Length) |> Async.ignore

However this specific snipper can be made much simpler since there is no reason to await the result of the computations when using Async.Parallel:

open System.IO

let fileContents =
    [ "filename1"; "filename2"; "etc" ]
    |> List.map(fun file -> async { use fs = File.OpenRead(file) in return! fs.AsyncRead(fs.Length |> int)})
    |> Async.Parallel
    |> Async.RunSynchronously
    |> Seq.map System.Text.Encoding.ASCII.GetString

One caveat - if the size of your file is more than 2GB it won't work due to long->int conversion - but that would be a crazy big file to just read the content of like that :-D


Java API code with Spring Model:

public class UserBean {
    @JsonProperty
    private String userId;
    @JsonProperty
    private int limit;
    @JsonProperty
    private int page;

    public String getUserId() {
        return userId;
    }
    public void setUserId(String userId) {
        this.userId = userId;
    }
    public int getLimit() {
        return limit;
    }
    public void setLimit(int limit) {
        this.limit = limit;
    }
    public int getPage() {
        return page;
    }
    public void setPage(int page) {
        this.page = page;
    }
}

public class ResponseJSON {
    private Map<String, Object> response = new LinkedHashMap<String, Object>();
    public void setStatus(String status) {
        response.put("status", status);
    }
    public void setMessage(String message) {
        response.put("message", message);
    }
    public void add(String key, Object value) {
        response.put(key, value);
    }
    public Map<String, Object> get() {
        return this.response;
    }
}

Controller:

@ResponseBody
@RequestMapping(value = "/user/products", method = RequestMethod.POST)
public Object getProductListByUser(@RequestBody UserBean userBean) {
    ResponseJSON responseJSON = new ResponseJSON();
    try {
        List<Object> result = userRepository.getProductListByUser(userBean);
        int resultCount = result.size();

        List<Object> productList = new ArrayList<Object>();
        for (int i = 0; i < resultCount; i++) {
            ProductInfo product = (ProductInfo) result.get(i);

            Map<String, Object> productInfo = new LinkedHashMap<String, Object>();
            productInfo.put("id", product.getId());
            productInfo.put("title", product.getTitle());
            productInfo.put("description", product.getDescription());
            productInfo.put("iconImage", product.getIconImage());
            productInfo.put("updateDatetime", product.getUpdateDatetime());
            productInfo.put("createDatetime", product.getCreateDatetime());
            productList.add(productInfo);
        }

        responseJSON.setStatus(Constants.SUCCESS);
        responseJSON.add("items", productList);
    } catch (Exception e) {
        responseJSON.setStatus(Constants.FAIL);
        responseJSON.setMessage(e.getMessage());
        logger.error(e.getMessage());
    }

    return responseJSON.get();
}

Repository:

@SuppressWarnings("unchecked")
@Transactional
public List<Object> getProductListByUser(UserBean userBean) {
    Session session = sessionFactory.getCurrentSession();

    Criteria criteria = session.createCriteria(ProductInfo.class, "product")
        .createAlias("product.productOptions", "options", CriteriaSpecification.LEFT_JOIN)
        .add(Restrictions.eq("product.userId", userBean.getUserId()))
        .addOrder(Order.asc("productOptions.size"))
        .setFirstResult((userBean.getPage() - 1) * userBean.getLimit());
        .setMaxResults(userBean.getLimit());

    return criteria.list();
}

I'm developing some back-end APIs.
This API returns product lists of some user.
Purpose of this API is returning result of JSON data.
So, I get datas from repository and assign to ResponseJSON object in the controller.
Is this approach efficient or not? Which way is better?

It would be easier to follow the getProductListByUser method if you use two ResponseJSON instance. One for normal execution and one for the exceptional cases:

try {
    ...

    final ResponseJSON responseJSON = new ResponseJSON();
    ...

    responseJSON.setStatus(Constants.SUCCESS);
    responseJSON.add("items", productList);
    return responseJSON.get();
} catch (final Exception e) {
    final ResponseJSON responseJSON = new ResponseJSON();
    responseJSON.setStatus(Constants.FAIL);
    responseJSON.setMessage(e.getMessage());
    logger.error(e.getMessage());
    return responseJSON.get();
}

It also ensures that nobody modifies accidentally the exception case result in the try block and there isn't any incomplete ProductInfo data in the error response.


A simple decorator written as a class, which counts how many times a function has been called Can I improve its typing? Is there any other improvement or pythonic change that you would do?

F = TypeVar('F', bound=Callable[..., Any])

# This is mostly so that I practice using a class as a decorator.
class CountCalls:
    """Logs to DEBUG how many times a function gets called, saves the result in a newly created attribute `num_calls`."""
    def __init__(self, func: F) -> None:
        functools.update_wrapper(self, func)
        self.func = func
        self.num_calls: int = 0
        self._logger = logging.getLogger(__name__ + '.' + self.func.__name__)
        self.last_return_value = None

    def __call__(self, *args: Any, **kwargs: Any) -> Any:
        self.num_calls += 1
        self._logger.debug(' called %s times', self.num_calls)
        self.last_return_value = self.func(*args, **kwargs)
        return self.last_return_value

Here's the decorator in action:

>>> @CountCalls
... def asdf(var: str):
...     print(var)
...     return len(var)
...
>>> asdf('Laur')
Laur
4
DEBUG:__main__.asdf: called 1 times
>>> asdf('python 3')
DEBUG:__main__.asdf: called 2 times
python 3
8
>>> asdf(3)
DEBUG:__main__.asdf: called 3 times
3
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "C:/Projects/Python/he/src/he/decorators.py", line 156, in __call__
    self.last_return_value = self.func(*args, **kwargs)
  File "<input>", line 4, in asdf
TypeError: object of type 'int' has no len()
>>> asdf.num_calls
3

One thing you could try to improve the typing would be to type the method itself (although I'm not sure how well tools support it). Also, leading/trailing whitespace should be up to the logger, not the code using it.

F = TypeVar('F', bound=Callable[..., Any])

# This is mostly so that I practice using a class as a decorator.
class CountCalls:
    """Logs to DEBUG how many times a function gets called, saves the result in a newly created attribute `num_calls`."""
    def __init__(self, func: F) -> None:
        functools.update_wrapper(self, func)
        self.func = func
        self.num_calls: int = 0
        self._logger = logging.getLogger(__name__ + '.' + self.func.__name__)
        self.last_return_value = None

    __call__: F

    def __call__(self, *args: Any, **kwargs: Any) -> Any:
        self.num_calls += 1
        self._logger.debug(f'called %s times', self.num_calls)
        self.last_return_value = self.func(*args, **kwargs)
        return self.last_return_value

As for the code itself, you could make a callback-based API.

F = TypeVar('F', bound=Callable[..., Any])

# This is mostly so that I practice using a class as a decorator.
class CountCalls:
    """Logs to DEBUG how many times a function gets called, saves the result in a newly created attribute `num_calls`."""
    def __init__(self, func: F, callback: Optional[Callable[[int, Tuple[Any], Dict[str, Any]], Any]] = None) -> None:
        if callback is None:
            logger = logging.getLogger(__name__ + '.' + self.func.__name__)

            def callback(num_calls: int, args: Tuple[Any], kwargs: Dict[str, Any]):
                self._logger.debug(f'called %s times', self.num_calls)

        functools.update_wrapper(self, func)
        self.func = func
        self.callback = callback
        self.num_calls: int = 0
        self.last_return_value = None

    __call__: F

    def __call__(self, *args: Any, **kwargs: Any) -> Any:
        self.num_calls += 1
        self.callback(self.num_calls, args, kwargs)
        self.last_return_value = self.func(*args, **kwargs)
        return self.last_return_value

Or with the number of calls tracked in the callback (for increased flexibility):

F = TypeVar('F', bound=Callable[..., Any])

# This is mostly so that I practice using a class as a decorator.
class CountCalls:
    """Logs to DEBUG how many times a function gets called, saves the result in a newly created attribute `num_calls`."""
    def __init__(self, func: F, callback: Optional[Callable[[int, Tuple[Any], Dict[str, Any]], Any]] = None) -> None:
        if callback is None:
            logger = logging.getLogger(__name__ + '.' + self.func.__name__)
            num_calls: int = 0

            def callback(args: Tuple[Any], kwargs: Dict[str, Any]):
                nonlocal num_calls  # Not sure if this is necessary or not
                num_calls += 1
                self._logger.debug(f'called %s times', self.num_calls)

        functools.update_wrapper(self, func)
        self.func = func
        self.callback = callback
        self.last_return_value = None

    __call__: F

    def __call__(self, *args: Any, **kwargs: Any) -> Any:
        self.callback(self.num_calls, args, kwargs)
        self.last_return_value = self.func(*args, **kwargs)
        return self.last_return_value

Earlier, to pass a keyword argument while using it, @functools.partial(CountCalls, callback=callback) was needed. Now, @CountCalls(callback=callback) can be used instead.

F = TypeVar('F', bound=Callable[..., Any])

# This is mostly so that I practice using a class as a decorator.
class CountCalls:
    """Logs to DEBUG how many times a function gets called, saves the result in a newly created attribute `num_calls`."""
    def __init__(self, func: F = None, callback: Optional[Callable[[int, Tuple[Any], Dict[str, Any]], Any]] = None) -> None:
        if callback is None:
            logger = logging.getLogger(__name__ + '.' + self.func.__name__)
            num_calls: int = 0

            def callback(args: Tuple[Any], kwargs: Dict[str, Any]):
                nonlocal num_calls  # Not sure if this is necessary or not
                num_calls += 1
                self._logger.debug(f'called %s times', self.num_calls)

        if func is None:
            return functools.partial(CountCalls, callback=callback)

        functools.update_wrapper(self, func)
        self.func = func
        self.callback = callback
        self.last_return_value = None

    __call__: F

    def __call__(self, *args: Any, **kwargs: Any) -> Any:
        self.callback(self.num_calls, args, kwargs)
        self.last_return_value = self.func(*args, **kwargs)
        return self.last_return_value

(Note: none of this code has been tested.)


Count characters in a string This code is supposed to count each character in a given string and account for error handling and type casting as well, and it should do it in the most efficient manner possible, with O(n) being an upper limit for both time and memory. I have gone through my process for problem solving which involves explaining the problem in my own words, looking at the inputs, the outputs, if the answer is even possible given the I/O, and then refactoring my code. Please, if anyone sees a better way to accomplish this then let me know.

function characterCounter(str) {
    // Error Handling
    // No input
    if (!str) {
        return "String input required";
    }

    // attempt to type cast non strings
    if (typeof str !== "string") {
        str = str.toString();
        // input could not be successfully type casted
        if (typeof str !== "string") {
            return "Invalid input";
        }
    }

    const stringLen = str.length;
    const result = {};
    let i = 0;

    // loop through each character for O(n) time complexity and O(n) space complexity
    // since the object grows based on input size
    for (; i < stringLen; i++) {
        // If string not in object as a key then add it and set value to 1
        if (result[str[i]] === undefined && str[i] !== " ") {
            result[str[i].toLowerCase()] = 1;
        } else {
            if (str[i] !== " ") {
                // add one to value at current key
                result[str[i]]++;
            }
        }
    }
    return result;
}

// Returns an object which allows O(1) for insert, deletion, access, and O(n) for searching...
console.log(characterCounter("Hello, World!"));

The algorithm is not working quite as you may expect:

   if (result[str[i]] === undefined && str[i] !== " ") {
        result[str[i].toLowerCase()] = 1;
    } else {
        if (str[i] !== " ") {
            // add one to value at current key
            result[str[i]]++;
        }
    }

If you enter "Hello, World! - Hi!" the two upper case 'H' are only counted as one, because they both evaluate to true in the first if-statement - because you test for an upper case 'H' but save a lower case 'h'.

In the same place you filter spaces out both in true and false, so you could do that as the first thing to do in the loop.

All in all that could be like:

  for (; i < stringLen; i++) {
    let ch = str[i].toLowerCase();
    if (ch !== " ") {
      if (result[ch] === undefined) {
        result[ch] = 1;
      } else {
        result[ch]++;
      }
    }
  }

Use comments to explain why you do what you do (if not obvious), and not what you do - the code shows that.

The whole ting could also be done like this:

function characterCounter(str) {
  if (!str) {
    return "String input required";
  }

  if (typeof str !== "string") {
    str = str.toString();
    if (typeof str !== "string")
      return "Invalid input";
  }

  const result = {};

  for (var i in str) {
    var ch = str[i].toLowerCase();
    if (ch !== " ") {
      if (result[ch] === undefined) {
        result[ch] = 1;
      } else {
        result[ch]++;
      }
    }
  }
  return result;
}