Autofac DI container in console app Just used DI containers in MVC projects, via constructor injection, and I'd need to now inject a service into a console app. I'm using Autofac so I followed what I found here.

The code would be something like this:

The service the console app depends on:

public interface IBatchService
{
    void WriteInformation(string input);
}
public class BatchService : IBatchService
{
    public void WriteInformation(string input)
    {
        Console.WriteLine(input);
        Console.ReadKey();
    }
}

The console app main method

public class Program
{
    private static IContainer Container { get; set; }
    static void Main(string[] args)
    {
        var builder = new ContainerBuilder();

        builder.RegisterType<BatchService.BatchService>().As<IBatchService>();
        Container = builder.Build();

        using (var scope = Container.BeginLifetimeScope())
        {
            var batchService = scope.Resolve<IBatchService>();

            batchService.WriteInformation("Injected!");
        }
    }

It's using the Service Locator if I'm not wrong (I'm trying to get my head around all of this), which I would be likely to avoid in case I could as it's thoroughly discussed in the web. But can I actually avoid it in a console app? Can any see any big flaw with this implementation?

The main method is almost as simple as this one, only one call to one service (although the service doesn't write a console). And that's the only code in the project in fact.

Your Main method has too many responsibilities: it's your application's composition root, and it's executing the program's logic... if printing to Console can be considered logic.

There's not enough meat in this project to even justify using DI/IoC: you have a service that nothing depends on.

If you had, say, an Application class:

public interface IApplication
{
    void Run();
}

public class Application : IApplication
{
    private readonly IBatchService _service;
    public Application(IBatchService service)
    {
        _service = service;
    }

    public void Run()
    {
        _service.WriteInformation("Injected!");
    }
}

...then composition would refer to resolving all dependencies of an Application instance, regardless of how complex that might be.

The Main method would only ask for an Application instance (let's call it app), and your IoC container would do its job - the Main method would only call app.Run();, and the rest of the application doesn't need to know there's an IoC container involved.

You don't need the static IContainer field; I've never used Autofac (you should look into Ninject, seems much simpler), but your IoC container should only live (and die) in the Main method - if you're using it anywhere else, you're doing something wrong (Service Locator).

I'd make it something like this:

static void Main(string[] args)
{
    var builder = new ContainerBuilder();

    builder.RegisterType<BatchService.BatchService>().As<IBatchService>();
    var container = builder.Build();

    using (var scope = container.BeginLifetimeScope())
    {
        var app = scope.Resolve<IApplication>();
        app.Run();
    }
}

Then your Main method is done with. If you're going to expand the application, you just inject new dependencies, and consume them.

Actually, this:

builder.RegisterType<BatchService.BatchService>().As<IBatchService>();

I'd move elsewhere; as your application grows, you'll be registering tons of stuff here, registration is a concern of its own, that deserves its own class IMO. That class can be static for all you care, it's infrastructure stuff, its only goal is to spit out a configured IContainer.

The configuration class could look like this:

public static class ContainerConfig
{
    public static IContainer Configure()
    {
        var builder = new ContainerBuilder();

        builder.RegisterType<BatchService.BatchService>().As<IBatchService>();
        // add other registrations here...

        return builder.Build();
    }
}

Leaving your Main method like this:

static void Main(string[] args)
{
    var container = ContainerConfig.Configure();
    using (var scope = container.BeginLifetimeScope())
    {
        var app = scope.Resolve<IApplication>();
        app.Run();
    }
}


Countdown script in JavaScript I've written this little countdown script to count until the end of our school days.

Now, that was written quickly, badly remembering modulo from a few years ago, and I don't think that's very optimized.

I thought about updating only the seconds and check if it needs to update the minute too, etc. But it wouldn't be accurate as setTimeout is not (depending if the browser lags, etc).

(function() {
    //the variable block here seems kinda weird, but whatever toots your horn
    var el = document.getElementById('countdown'),
        endDate = new Date('March 30, 2012 18:10:00'),
        curDate,
        diff,
        days,
        hours,
        minutes,
        seconds,
        tmp,
        countdown,
        //added Math.floor. this is already a shortcut, might as well make it a double one
        pad = function(number) { return (number < 10 ? '0' : '') + Math.floor(number) },

        //calculate these constants once, instead of over and over
        minute = 60 * 1000,
        hour = minute * 60,
        day = hour * 24

    ;(function tick() {
        curDate = new Date()
        //you want the absolute value of this, not of individual calculations using this
        diff = Math.abs(new Date(curDate.getTime() - endDate.getTime()))
        days = diff / day

        tmp = diff % day
        hours = tmp / hour

        tmp = tmp % hour
        minutes = tmp / minute

        tmp = tmp % minute
        seconds = tmp / 1000

        //parseInt was redundant
        countdown = pad(days) + ':' + pad(hours) + ':' + pad(minutes) + ':' + pad(seconds)

        if ( 'textContent' in el ) {
            el.textContent = countdown
        }
        else {
            el.innerText = countdown
        }

        //dont't use arguments.calle, it's deprecated
        //simply use a named function expression
        setTimeout(tick, 1000)
    }())
}())

About the semicolons missing: it is a deliberate choice as I find this more readable (the ; right before the (function() is because it is the only edge case where the semicolon insertion doesn't work correctly).

Your whole code can be reduced to this.

/* secsToDHMS()
 * Author Gary Green
 * http://stackoverflow.com/users/63523/gary-green
 */
function secsToDHMS(s)
{
  var x = [86400, 3600, 60, 1],z,i=-1;
  while (z=x[++i])
        x[i] = ("0" + parseInt(s / z,10)).slice(-2),
        s %= z;

    return x.join(':');
};

(function() {

    var currentDate      = new Date();
    var endDate          = new Date('March 30, 2012 18:10:00');
    var secsToGo         = (new Date(endDate - currentDate)).getTime() / 1000;
    var countdownElement = document.getElementById('countdown');

    var timer = setInterval(function() {

        if (--secsToGo < 0) {
            clearInterval(timer);
            return;
        }

        countdownElement.innerHTML = secsToDHMS(secsToGo);

    }, 1000);

})();

Fiddle: http://jsfiddle.net/xg63V/4

Compressed version 311 bytes

(function(){var d=(new Date(new Date("March 30, 2012 18:10:00")-new Date)).getTime()/1E3,
f=document.getElementById("countdown"),g=setInterval(function(){if(0>--d)clearInterval(g);
else{for(var a=d,b=[86400,3600,60,1],c,e=-1;c=b[++e];)b[e]=("0"+parseInt(a/c,10)).slice(-2),
a%=c;f.innerHTML=b.join(":")}},1E3)})();


What is the most optimal way to concat XML files as one large string/text file? Python3 - Recursive XML Files to Large Text File
I have a recursive function that looks through a directory (there are 6000+ files).
For each XML file found - the XML is converted to a string and appended to an array.
At the end of the recursion - All these arrays are joined to return a (very) large string.
The error I am getting is MemoryError - How can I optimise my code?
import os
import xml.etree.ElementTree as ET

# Cycle through a directory and concat files together - recursive dive.
# NOTE:
# Have a folder called 'PLC 2360' in the immediate directory.
# The output is a huge text file

path = 'PLC 2360'

big_text = []

def big_file(path):
    try:
        for i in next(os.walk(path))[2]:    # For each XML file read and convert to string
            xml = ET.parse(path + '/' + i).getroot()
            big_text.append(ET.tostring(xml, encoding='unicode'))    # Append string of XML
    except:
        print('Error on', path)

    if next(os.walk(path))[1]:    # If folders exist - cycle through
        for i in next(os.walk(path))[1]:
            big_file(path + '/' + i)    # Enter next recursion layer

    return ''.join(str(i) for i in big_text)    # Return one big string

lis = big_file(path)
print(len(lis))
print(lis[:500])
```

functions
Your script does a few things

iterate over the files
recursively iterate over the subdirectories
parse the files
concatenate the results

Better would be to separate those in different functions.
The extra advantages is that you can test these functions separately, you can document them with a docstring, and add typing information
comments
You comment what the code does. Python is luckily so expressive that almost anyone can understand what a particular line does. what is more difficult is the why you do certain steps, and why you do them in a certain order. This is what should be commented
global variables
Your big_file method changes the global state of the program. That makes it more difficult to reason about, and also makes it difficult if you want to use this methon on 2 separate directories. Here you append to big_text. If you want to keep it like this, I would pass it on as a function parameter, instead of a global variable
def big_file(path, big_text = None):
    if big_text is None:
        big_text = []

    ...
            big_file(path + '/' + i, big_text=big_text)

pathlib.Path
Most file operations are simpler when using the pathlib  module. It will be a lot more robust than manually concatenating paths like in path + '/' + i
error handling
You have a try-except block with a bare except. Better here would be to catch the errors you expect specifically and handle those, and let other, unexpected error bubble up. Fail hard, fail fast, instead of covering up bugs can help you write more stable and correct software
logging
Instead of using print, you can use the logging module. That way you can make a distinction between different levels of importance, and filter some out if needed
generators
To prevent a MemoryError you can use generators. these are special functions that do their work piece by piece, and can work without keeping the whole structure in memory
You can have 1 generator generate the files
def iterate_files(path: Path) -> typing.Iterator[Path]:
    """Recursively iterates over `path`, yielding all the correct files"""
    for file in path.glob("*"):
        if file.is_dir():
            yield from iterate_files(file)
        else:
            # or a check that the file has a certain suffix
            yield file

Then you feed this iterator to the parser generator
def parse_files(files: typing.Iterator[Path]) -> typing.Iterator[str]:
    """Parse the xml files."""
    for file in files:
        try:
            xml = ET.parse(path + '/' + i).getroot()
            yield ET.tostring(xml, encoding='unicode')
        except <stricter exception>:
            logging.warn(f"error in {file}")
            raise

In the last except, you can have different except blocks with different result
You can then feed this to another generator which writes it to a file:
def write_to_file(
    text_iterator: Typing.Iterable[str], output_filehandle: typing.TextIO
) -> Typing.Iterable[str]:
    for chunk in text_iterator:
        output_filehandle.write(chunk)
        yield chunk

putting it together
if __name__ == "__main__":
    path = Path("PLC 2360")
    files = iterate_files(path)
    parsed_filed = parse_files(files)

    with Path(<output_path>).open("w") as output_filehandle:
        parsed_filed_after_writing = write_to_file(
            text_iterator=parse_files, output_filehandle=output_filehandle
        )
    ...

In that last part, I've opened the file in the main part of the script, taking into account the principles of clean architecture


Naruto, where are you? During Winter Bash 2014, the Naruto hat was awarded to users who gave an answer that got accepted but hasn't received votes for 12 hours.
It was a weird thing, really:
why would someone ever accept a question but not upvote it?
I haven't yet experienced a situation where this would be reasonable.

Accepted answers without upvotes seem to be an anomaly:
it happens when the OP doesn't have enough reputation to upvote,
and the post gets overlooked by other users in the community.
The community might actually want to clean these up.

To find eligible posts I put together this simple SEDE query (latest version, NOT for review):

DECLARE @username as NVarchar(60) = RTRIM(LTRIM(##DisplayName:string? ##));
DECLARE @userId as int = ##UserId:int?-1##;

SELECT TOP 100
  u.id as [User Link],
  p.Id as [Post Link],
  p.CreationDate
FROM Posts p
  JOIN Users u ON p.OwnerUserId = u.Id
  JOIN Posts q ON p.ParentId = q.Id
WHERE
  (@username = '' OR u.DisplayName = @username)
  AND (@userId = -1 OR u.Id = @userId)
  AND p.Score = 0
  AND p.Id = q.AcceptedAnswerId
ORDER BY p.CreationDate DESC

The query accepts two parameters:

Optional display name, to filter by username
Optional user id, to filter by user id
With default values it will show all eligible posts of the given site
It won't make sense to set both display name and user id, but such misuse would be  the user's fault

Something I don't like about this is the username parameter.
I would have wanted NULL or empty string as default value but couldn't figure out how to do that.
Using space does work, but it's not exactly pretty,
I'm wondering if there's a better way I'm missing.

I'm interested in any other suggestions to improve this query.

There is not a whole lot to say, but I found a few small things that could be improved.

Consistency

DECLARE @username as NVarchar(60) = RTRIM(LTRIM(##DisplayName:string? ##));
DECLARE @userId as int = ##UserId:int?-1##;
DECLARE @limit as int = ##Limit:int?100##;

Would be better as:
DECLARE @userName AS NVARCHAR(60) = RTRIM(LTRIM(##DisplayName:string? ##));
DECLARE @userId AS INT = ##UserId:int?-1##;
DECLARE @limit AS INT = ##Limit:int?100##;

Aliases
I think your aliases mostly obfuscate the query. It's also recommended to explicitly state the type of join, e.g.:
FROM Posts AS posts
  INNER JOIN Users AS users ON p.OwnerUserId = u.Id
  INNER JOIN Posts AS questions ON p.ParentId = q.Id

Trim
Your left and right trim operations don't really achieve anything. Do you expect a user name to have a bunch of white space before or after it? I'm not sure SE would even allow that. I removed them and got identical results.

Everything combined:
DECLARE @username AS NVARCHAR(60) = ##DisplayName:string? ##;
DECLARE @userId AS INT = ##UserId:int?-1##;
DECLARE @limit AS INT = ##Limit:int?100##;

SELECT TOP 100
  users.Id AS [User Link],
  posts.Id AS [Post Link],
  posts.CreationDate
FROM Posts AS posts
  INNER JOIN Users AS users ON posts.OwnerUserId = users.Id
  INNER JOIN Posts AS questions ON posts.ParentId = questions.Id
WHERE
  (@username = '' OR users.DisplayName = @username)
  AND (@userId = -1 OR users.Id = @userId)
  AND posts.Score = 0
  AND posts.Id = questions.AcceptedAnswerId
ORDER BY posts.CreationDate DESC

Eliminating the empty string check
There is a way to eliminate the @username = '' check, however it does have a performance impact. I ran it and it runs in ~400 ms, which is not bad but a bit slower. It makes the index scan on the [Users].[UIX_Users_Id] table take longer due to partial string match.
WHERE
  u.DisplayName LIKE CONCAT('%', @username, '%')
  AND (@userId = -1 OR u.Id = @userId)
  AND p.Score = 0
  AND p.Id = q.AcceptedAnswerId


Closing SQL connection for async operations The following code executes asynchron on the SQL database. Is the closing of the connection implemented in the right way? Are there any missing features regarding connection handling which need to be added?

public void deleteData(Node node, User user)
        {
            SqlConnection connection = getConnection();
            connection.Open();
            string query = "DELETE FROM NodeUserData WHERE node = @nodeId and userId = @userId;";
            SqlCommand cmd = new SqlCommand(query, connection);
            cmd.CommandType = CommandType.Text;

            try
            {
                cmd.Parameters.AddWithValue("@nodeId ", node.id);
                cmd.Parameters.AddWithValue("@userId", user.id);

                List<Task> asynctasks = new List<Task>();
                asynctasks.Add(cmd.ExecuteNonQueryAsync());
                Task.Factory.StartNew(() => closeConnectionOnFinish(asynctasks, connection));

            }
            catch (Exception ex)
            {
                writeExceptionToLogFile(ex);
            }
        }

And the closeConnectionOnFinish:

public async  void closeConnectionOnFinish(List<Task> _tasks, SqlConnection connection)
        {
            try
            {
                await Task.WhenAll(_tasks);
                connection.Close();
            }
            catch (Exception ex)
            {
                writeExceptionToLogFile(ex);
            }
        }

No that isn't right. If your query throws an error you won't be closing the connection. Swallowing exceptions like that (even though you're logging it) is generally bad as well. How does the user know anything has gone wrong?

If you can, just use async and await with a using:

public async Task DeleteDataAsync(...)
{
     using (var connection = GetConnection())
     using (var cmd = /* ... */)
     {
         // ... trimmed
         await cmd.ExecuteNonQueryAsync();
     }
}

If you can't do that (I don't know why you couldn't) then you could do this:

cmd.ExecuteNonQueryAsync().ContinueWith(_ => connection.Close());

But you should be waiting for that task to finish.

Your naming is non standard: methods should be PascalCase.

async void is generally bad - there's no way for the caller to wait until the work is finished. You shouldn't use async void methods (except for event handlers).

Async methods should have the suffix Async and return a Task or Task<T>.

Don't use Task.Factory.StartNew unless you want to specify advanced options; use Task.Run. However, don't use Task.Run for fake asynchrony - you'll just slow things down.


Recursive binary search implementation in Python I have this (recursive) binary search implementation in Python, and I want you to criticize every aspect of it. It is a naive implementation, (drilling the basics and stuff), but, I want you to comment on the perceived clarity, maintainability of the code, defensive programming practices, test cases that could be useful but are missing, implicit assumptions that may not hold, pythonic-ness of the code, anything that could help me improve the code.

import unittest

def binary_search(collection, key):
    """ Search for a given key in the list of elements
    passed as a parameter.

    :param collection: The iterable to be searched.
    :param key: The search key.

    Returns the index where the key was found, else the symbolic string 'Not found'.
    """
    # First, make sure that the collection we have been given is truly
    # iterable
    try:
        iterator = iter(collection)
    except TypeError:
        raise TypeError("`collection` is not an iterable collection.")

    # if the collection is empty, it means that we have gone
    # past the boundaries and haven't found a value, so we can
    # safely assume there doesn't exist one in the original collection
    if not collection:
        return "Not found"

    collection_length = len(collection)
    mid = collection_length // 2

    # if we found it great, return the index
    if collection[mid] == key:
        return mid
    # now, if we haven't found it,
    elif collection[mid] < key:
        # if there was only 1 element in the collection before
        # the last call to binary search, this means that the
        # floor division above will have returned a mid == 0,
        # causing the recursive function below to bug out and
        # blow the stack. This is a sentinel to make sure this doesn't
        # happen
        if mid == 0:
            return "Not found"
        res = binary_search(collection[mid:], key)
        return mid + res if res != "Not found" else "Not found"
    elif collection[mid] > key:
        return binary_search(collection[:mid], key)

class BinarySearchTests(unittest.TestCase):
    def test_value_found(self):
        collection = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]
        res = binary_search(collection, 3)
        self.assertEqual(res, 2, 'Binary search failed to return expected index of search key.')
        res = binary_search(collection, 5)
        self.assertEqual(res, 4, 'Binary search failed to return expected index of search key.')
        res = binary_search(collection, 11)
        self.assertEqual(res, 10, 'Binary search failed to return expected index of search key.')

    def test_value_not_found(self):
        collection = [1, 2, 3, 4, 5]
        res = binary_search(collection, -1)
        self.assertEqual(res, 'Not found', 'Binary search failed to return expected "Not found" value.')
        res = binary_search(collection, 6)
        self.assertEqual(res, 'Not found', 'Binary search failed to return expected "Not found" value.')

    def test_elements_is_not_iterable(self):
        import socket
        elements = socket.socket()
        key = 1
        self.assertRaises(TypeError, binary_search, (key, elements))
        elements.close()  # so that the interpreter doesn't give us a resource warning.

if __name__ == '__main__':
    unittest.main()

Use an inner function

As you make recursive calls, some conditions are unnecessarily reevaluated, for example checking if the collection is iterable. This is kind of ugly, along with the "sentinel" you put in place for mid == 0.

In a more conventional implementation, you don't slice the input collection, but narrow down the search range, by adjusting start and end boundaries. You could create an inner function with signature:

def _binary_search(start, end):
    # ...

And call it with parameters 0 and len(collection). The function will call itself recursively, replacing the start our end parameters with the recalculated mid. There is no list slicing, no unnecessary conditions, and no odd check on mid == 0. The inner function is not visible outside, it's a hidden implementation detail.

Unit testing

It's generally best to have one assert per test case.
It's a bit more work, but it's clearer, and its worth it.
The practice also makes you think twice before adding redundant tests that are just noise, and think through more carefully what you're testing and why is it important.

Some interesting cases that would be nice to see cleanly separated:

should return correct result when match is the first element
should return correct result when match is the last element
should return correct result when match is the only element
should return correct result when match is exactly at mid point
should return correct result when list is empty

Magic string

The string "Not found" appears multiple times in the implementation. It would be better to put it in a constant and reuse.

Alternative return value for missing

A common practice for the return value of a failed binary search is to return -1 -insertAt where insertAt is the position where the element should be inserted to keep the list sorted.


Implementing N-ary trees in C Looking at my code, I feel like it is not instantly clear for someone who did not write it. I feel like readability will be important for me in the future, if I'm ever to write code on an enterprise, or open source project. Do you have any advice as to how I can make this and future C more readable?

#include "datatypes.h"
#include <stddef.h>

//Changes the node's child (if it doesn't exist) to newChild
//or appends newChild to the end of the node's child's sibling list
void addChild ( treeNode *root, treeNode *newChild ) {
  treeNode *temp;
  if ( root->child == NULL ) {
    root->child = newChild;
  } else {
    temp = root->child;
    while ( temp->sibling != NULL ) {
      temp = temp->sibling;
    }
    temp->sibling = newChild;
  }
}

//Returns a new treeNode with default data values or {0,NULL,NULL}
treeNode newNode () {
  treeNode node = {0, NULL, NULL};
  return node;
}

datatypes.h

typedef struct treeNode {
  int n;
  struct treeNode *child;
  struct treeNode *sibling;
} treeNode;

treeNode newNode (); //Returns a treenode with default {0,NULL,NULL}
void addChild ( treeNode *t, treeNode *c ); //Adds child to the given  treeNode

Comment, Comment, Comment. If you want any other person on this blue planet to know whats going on in your code in the easiest way possible, comment your code.

// This is what the code does in the next line.
/* I'm going to write a block of text to tell you the purpose of what this function
is trying to accomplish */

Your code doesn't have to be pretty or legible for that matter, but a few comments go along way.

Also, whitespace for your if and while statements are appealing to the eye and allow the reader to break apart your conditional statements. Trying to cram as much as you can on one line causes headaches when trying to interpret them.


Immuitable recursive linear search of an array of string I am new to F#. Is the following code, which represents about three hours of work, a canonical way to achieve its goals. If not, how can it be improved?

For instance, can we provide a default value of zero for the index parameter? Also, how do we handle a search for a term that does not exist? Could we use something similar to tail::head instead of matching against array.[index]? It looks more verbose that it needs to be.

open System

let rec linearSearch (array: string[]) (target: string) (index: int) =
    match array.Length with
    | l when index >= l -> -1
    | _ ->
        match array.[index] with
        | x when x = target -> index
        | y -> linearSearch array target (index + 1)

[<EntryPoint>]
let main argv =
    let names = [|
        "erdnase"
        "vernon"
        "le paul"
        "scarne"
        "ireland"
    |]
    linearSearch names "le paul" 0 |> printfn "The target is at index: %A"
    linearSearch names "luttin" 0 |> printfn "The target is at index: %A"
    0

The goals for writing the code were:

Use an array instead of a list, because I would like to understand F# arrays.
Implement a linear search, because it is a simple algorithm.
Write in canonical F#, because canonical code is more maintainable.

The additional argument index is error prone. For example, a user might provide -1 and you end up with array.[-1]. So let us get rid of that first:

let linearSearch (array: string[]) (target: string) =
    let rec search index =
            match array.Length with
                | l when index >= l -> -1
                | _ ->
                    match array.[index] with
                    | x when x = target -> index
                    | y -> search (index + 1)
    search 0

Now the recursive call is hidden in search, and also the index cannot get negative or otherwise mangled by a user. Next, we can make your linearSearch more general:

let linearSearch (array: 'a[]) (target: 'a) =
    let rec search index =
            match array.Length with
                | l when index >= l -> -1
                | _ ->
                    match array.[index] with
                    | x when x = target -> index
                    | y -> search (index + 1)
    search 0

After all it should work for anything we can compare with =. Talking about comparing something, your pattern matches aren't really matching anything. You're just checking whether a condition has been met. Use usual structures instead:

let linearSearch (array: 'a[]) (target: 'a) =
    let rec search index =
            if index >= array.Length then
              -1
            elif target = array.[index] then
              index
            else
              search (index + 1)
    search 0

And last but not least, I would use the same promises as the usual C#/F# functions: either throw an exception when the value has not been found, or return an Option. That way, if we got an int, it's valid, so we don't have to check for an invalid position. I've choosed option:

let linearSearch (array: 'a[]) (target: 'a) =
    let rec search index =
            if index >= array.Length then
              None
            elif target = array.[index] then
              Some index
            else
              search (index + 1)
    search 0

But that's up to you.

Exercise

Write a function linearFind that uses a function to find the correct index:

linearFind (array : 'a[]) (predicate: a' -> bool) = …

If linearFind returns Some index, then predicate array.[index] should be true.
Express linearSearch in terms of linearFind.


Python web-scraper to download table of transistor counts from Wikipedia I have been looking for answers for how to easily scrape data from Wikipedia into a CSV file with Beautiful Soup. This is the code so far. Is there an easier way to do it?

import urllib.request
from bs4 import BeautifulSoup
import csv

#the websites
urls = ['https://en.wikipedia.org/wiki/Transistor_count']
data =[]

#getting the websites and the data
for url in urls:
##    my_url = requests.get(url)
    my_url = urllib.request.urlopen(url)

    html = my_url.read()
    soup = BeautifulSoup(html,'html.parser')

    My_table = soup.find('table',{'class':'wikitable sortable'})
    My_second_table = My_table.find_next_sibling('table')

    with open('data.csv', 'w',encoding='UTF-8', newline='') as f:
        fields = ['Title', 'Year']
        writer = csv.writer(f, delimiter=',')
        writer.writerow(fields)

    with open('data.csv', "a", encoding='UTF-8') as csv_file:
        writer = csv.writer(csv_file, delimiter=',')
        for tr in My_table.find_all('tr')[2:]: # [2:] is to skip empty and header
            tds = tr.find_all('td')
            try:
                title = tds[0].text.replace('\n','')
            except:
                title = ""
            try:
                year = tds[2].text.replace('\n','')
            except:
                year = ""

            writer.writerow([title, year])

    with open('data.csv', "a", encoding='UTF-8') as csv_file:
        writer = csv.writer(csv_file, delimiter=',')
        for tr in My_second_table.find_all('tr')[2:]: # [2:] is to skip empty and header
            tds = tr.find_all('td')
            row = "{}, {}".format(tds[0].text.replace('\n',''), tds[2].text.replace('\n',''))
            writer.writerow(row.split(','))

Is there a way to simplify this code?

Yes. Don't scrape Wikipedia. Your first thought before "should I need to scrape this thing?" should be "Is there an API that can give me the data I want?" In this case, there super is.

There are many informative links such as this StackOverflow question, but in the end reading the API documentation really is the right thing to do. This should get you started:

from pprint import pprint
import requests, wikitextparser

r = requests.get(
    'https://en.wikipedia.org/w/api.php',
    params={
        'action': 'query',
        'titles': 'Transistor_count',
        'prop': 'revisions',
        'rvprop': 'content',
        'format': 'json',
    }
)
r.raise_for_status()
pages = r.json()['query']['pages']
body = next(iter(pages.values()))['revisions'][0]['*']
doc = wikitextparser.parse(body)
print(f'{len(doc.tables)} tables retrieved')

pprint(doc.tables[0].data())

This may seem more roundabout than scraping the page, but API access gets you structured data, which bypasses an HTML rendering step that you shouldn't have to deal with. This structured data is the actual source of the article and is more reliable.


Matlab tic and toc commands implemented in C++ I need to time a few functions that I'm implementing, so I decided to implement a way of timing even multiple or sub calls based on the stack data structure and on the tic and toc commands of Matlab.

tictoc.h

#include <assert.h>
#include <cmath>
#include <ctime>
#include <iostream>
#include <stack>

static std::stack<std::clock_t> starting_times;

void start_timer()
{
    starting_times.push(std::clock());
}

int stop_timer(bool print, const char* to_execute = "")
{

    assert(!starting_times.empty());

    double milliseconds = (std::clock() - starting_times.top()) / (double)(CLOCKS_PER_SEC / 1000);
    starting_times.pop();

    if (print) {

        // minutes
        if (milliseconds >= 60 * 1000) {

            double m = milliseconds / 1000 / 60;
            double s = std::fmod(m, 60);
            double ms = std::fmod(s, 1000);

            std::cout << "Time elapsed '"
                      << std::lrint(m) << "m, "
                      << std::lrint(s) << "s and "
                      << std::lrint(ms) << "ms' "
                      << to_execute << "\n";

        } else if (milliseconds >= 1000) {
            std::cout << "Time elapsed '"
                      << std::lrint(milliseconds / 1000) << "s and "
                      << std::lrint(std::fmod(milliseconds, 1000)) << "ms' "
                      << to_execute << "\n";
        } else {
            std::cout << "Time elapsed '" << milliseconds << "ms' " << to_execute << "\n";
        }
    }

    return milliseconds;
}

to_execute is just a string that is eventually passed to describe which functions has just stopped being executed.

Of course, as you can observe from this code, I'm not expecting the functions to last more than 60 minutes, but if you think it's a good idea to add it, feel free to suggest a way to do it.

Of course, I've already tried the code and it seems to work.

Here's a simple runnable example that could use these functions:

main.cpp

#include "tictoc.h"
#include <iostream>

int main(const int argc, const char** argv)
{

    start_timer();

    int sum = 0;
    for (int i = 0; i < 10000; ++i) {
        sum += i;
    }

    std::cout << "sum = " << sum << "\n";

    stop_timer(true, "to execute the sum from 0 to 999");

    return 0;
}

Now put this and tictoc.h above in the same folder and compile with C++11, assuming you have it:

g++ -std=c++11 main.cpp -o main

Run executable main as:

./main

So as you are using c++ you should use the stl for that

#include <chrono>
#include <iostream>
int main() {
    std::chrono::high_resolution_clock::time_point start, end;

    start = std::chrono::high_resolution_clock::now();
    /* do someting */
    end = std::chrono::high_resolution_clock::now();
    double time_elapsed = std::chrono::duration_cast<std::chrono::milliseconds>( end - start ).count();
    std::cout << "Code took " << time_elapsed << " miliseconds to execute\n";
};

EDIT

So here an more extended version

#include <chrono>
#include <iostream>

std::chrono::high_resolution_clock::time_point start_timer() {
    return std::chrono::high_resolution_clock::now();
}

void stop_timer (clock::time_point &start, std::string &message) {
    using cast = std::chrono::duration_cast;
    auto duration = cast<std::chrono::milliseconds>(std::chrono::high_resolution_clock::now() - start);
    auto hours    = cast<std::chrono::hours>(duration).count();
    duration     -= cast<std::chrono::milliseconds>(hours);
    auto minutes  = cast<std::chrono::minutes>(duration).count();
    duration     -= cast<std::chrono::milliseconds>(minutes);
    auto seconds  = cast<std::chrono::minutes>(duration).count();
    duration     -= cast<std::chrono::milliseconds>(seconds);

    std::cout << "The code took ";
    if (hours >= 1) {
        std::cout << std::lrint(hours) << "h";
    }
    if (minutes >= 1) {
        std::cout << std::lrint(minutes) << "m";
    }
    if (seconds >= 1) {
        std::cout << std::lrint(seconds) << "s";
    }
    if (milliseconds >= 1) {
        std::cout << std::lrint(duration) << "ms";
    }
    std::cout << " to execute\n";
}

int main() {
    std::chrono::high_resolution_clock::time_point start = start_timer();
    /* do someting */
    stop_timer(start);
}


Header-only variadic console output and runtime input validation in C++ using templates Template IO Module

This is a single-file, header-only input validation with two variadic output functions. One output function simply inserts a new line char after every output and the other one inserts a space; this is the only difference.

The end goal of this project is a convenient, cross-platform IO module, and I thought that the static type-checking and compile-time generation of templates would be the perfect combination of type safety without run-time overhead, with the added bonus of the simplicity of adding a header-only module to an existing project.

Source

#ifndef _INPUT_VALIDATION_HPP
#define _INPUT_VALIDATION_HPP

/** Required STL Header Files for this module:
*  #include <iostream>
*  #include <limits>
*  #include <typeinfo>
*
*/

namespace IO {

    template <typename T>
    void print(const T& input)
    {
        std::cout << input << ' ';
    }

    template <typename T, typename... Types>
    void print(const T& firstArg, const Types&... arguments)
    {
        std::cout << firstArg << ' ';
        print(arguments...);
    }

    template <typename T>
    void println(const T& input)
    {
        std::cout << input << std::endl;
    }

    template <typename T, typename... Types>
    void println(const T& firstArg, const Types&... arguments)
    {
        std::cout << firstArg << std::endl;
        println(arguments...);
    }

    template <typename T>
    T get()
    {
        auto IO_status_flag = false;

        T response;
        std::cin >> response;

        do {
            if (std::cin.fail()) {

                IO::print("[Error] Please enter a valid response:");
                std::cin.clear();
                std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');

                std::cin >> response;
            } else {
                IO_status_flag = true;
            }
        } while (IO_status_flag != true);

        return response;
    }

}

#endif /* _INPUT_VALIDATION_HPP */

Test Code

#include <iostream>
#include <limits>
#include <typeinfo>

#include "input-validation.hpp"

int main()
{
    IO::print("Please enter username:");
    auto username = IO::get<std::string>();

    IO::print("Please enter age:");
    auto age = IO::get<int>();

    IO::print("\nName:", username);
    IO::print("\nAge:", age);

    auto array = { "one", "two", "three", "four", "five", "six" };

    IO::println("\n");

    for(auto str : array)
    {
        IO::println(str);
    }

}

Output

Please enter username: Jose
Please enter age: 23

Name: Jose
Age: 23

one
two
three
four
five
six

Comment

First question is why I would use this instead of the standard operator<< that we are all very familiar with?

Code Review

Reserved Identifiers

Identifiers with a prefix underscore and an initial capital letter are reserved in all contexts for the implementations. i.e. You are not allowed to use them.

So _INPUT_VALIDATION_HPP is an illegal identifier (for you).

The rules on leading underscore are complex. Even if you know the rules, most people don't, so avoid at all costs.

If you must, then make it a trailing underscore.

Manipulator support.

You have not provided support for stream manipulators (std::hex etc..).

std::endl or '\n'

Prefer '\n' over std::endl;. The difference is that std::endl also forces a flush. Manually forcing a flush is 99.99% of the time incorrect as the stream will automatically flush at the optimal times; doing it manually like this will severely degrade performance.

Recursive over iterative templates.

You use a recursive template design. Personally I prefer the iterative approach as it is more compact (and in my opinion readable).

// You recursive approach.
template <typename T>
void print(const T& input)
{
    std::cout << input << ' ';
}

template <typename T, typename... Types>
void print(const T& firstArg, const Types&... arguments)
{
    std::cout << firstArg << ' ';
    print(arguments...);
}

The alternative:

// The iterative approach.
template <typename T, typename... Types>
void print(Types const&... arguments)
{
    auto unused = {0, (std::cout << arguments << ' ', 0)...};
}

Get only half-checks.

In the get() function if it fails you drop the rest of the line and try again. This implies that the input is line based. BUT on success you don't check that the input is a whole line; you just check the line prefix.

// SO my code is.
int val = IO::get<int>();

// I type
10Ton

Then the above function accepts the input as good. But in my opinion that input should be considered to be incorrect as Ton is being ignored.


Triangle Type Program Is there any way to simplify it further?

import java.util.*;

class Triangle {

    public static void main(String[] args) {

        Scanner sc = new Scanner(System.in);
        int a = sc.nextInt();
        int b = sc.nextInt();
        int c = sc.nextInt();

        if(a==b && b==c)
            System.out.println("Equilateral");

        else if(a >= (b+c) || c >= (b+a) || b >= (a+c) )
            System.out.println("Not a triangle");

        else if ((a==b && b!=c ) || (a!=b && c==a) || (c==b && c!=a))
            System.out.println("Isosceles");

        else if(a!=b && b!=c && c!=a)
            System.out.println("Scalene");
    }
}

Is there any way to simplify it further?

Sure, but what's more important: Create some useful methods. Your class is single shot only: read data, classify, output. This is a bad start, a method should do one thing only. So you need three methods.

You classify into 4 categories and this is a clear case for an enum:

enum TriangleType {
    EQUILATERAL, INVALID, ISOSCELES, SCALENE
}

This is the only interesting method:

static TriangleType classify(int a, int b, int c) {
    if (a <= 0 || b <= 0 || c <= 0) return INVALID; // added test
    if (a == b && b == c) return EQUILATERAL;
    if (a >= b+c || c >= b+a || b >= a+c) return INVALID;
    if (b==c || a==b || c==a)) return ISOSCELES;
    return SCALENE;
}

Note that I'm violating the "braces everywhere" rule. I can't help myself.

In case of ISOSCELES, there's no need for checks like a==b && b!=c as we already know that not all sites are equal.

In case of SCALENE, there's no need for any test, as we know that there are no two equals sides.

You're using a bit too many parentheses, but feel free to keep them if you feel you need them.

Be consequent with your spacing, there are rules for it, which help to make it more readable:

single space between "if" and "("
(but no space between a method name and "(")
no space after "("
no space before ")"

The addition is subject to overflow as Ryan noted, but let's ignore it for now. You should know about it and do a corresponding test if needed, but in general, overflow is impossible to check for everywhere.

Here it's trivial, just replace a >= b+c by a >= (long) b+c. The latter means a >= ((long) b) + c, which is equivalent to ((long) a) >= ((long) b) + ((long) c).


Printing a number based on a user-provided number Is there a better way to ask for input and give answers, but without so many lines? Each input will give a specific output according to a key that I use, but this many lines seems excessive. I'm a beginner to java and could use some ideas on how to optimize this coding.

    import java.util.Scanner;
    class Test {
    public static void main (String [] args) {

    System.out.print ("Enter the day of the month - Answer: ");
    Scanner inputFigure = new Scanner (System.in);
    int i = inputFigure.nextInt ();
    if ( i==1) {
        System.out.println ("12");
    }
    else if (i==2) {
        System.out.println ("13");
    }
    else if (i==3) {
        System.out.println ("14");
    }
    else if (i==4) {
        System.out.println ("11");
    }
    else if (i==5) {
        System.out.println ("12");
    }
    else if (i==6) {
        System.out.println ("13");
    }
    else if (i==7) {
        System.out.println ("14");
    }
    else if (i==8) {
        System.out.println ("11");
    }
    else if (i==9) {
        System.out.println ("12");
    }
    else if (i==10) {
        System.out.println ("21");
    }
    else if (i==11) {
        System.out.println ("22");
    }
    else if (i==12) {
        System.out.println ("23");
    }
    else if (i==13) {
        System.out.println ("24");
    }
    else if (i==14) {
        System.out.println ("21");
    }
    else if (i==15) {
        System.out.println ("22");
    }
    else if (i==16) {
        System.out.println ("23");
    }
    else if (i==17) {
        System.out.println ("24");
    }
    else if (i==18) {
        System.out.println ("21");
    }
    else if (i==19) {
        System.out.println ("22");
    }
    else if (i==20) {
        System.out.println ("31");
    }
    else if (i==21) {
        System.out.println ("32");
    }
    else if (i==22) {
        System.out.println ("33");
    }
    else if (i==23) {
        System.out.println ("34");
    }
    else if (i==24) {
        System.out.println ("31");
    }
    else if (i==25) {
        System.out.println ("32");
    }
    else if (i==26) {
        System.out.println ("33");
    }
    else if (i==27) {
        System.out.println ("34");
    }
    else if (i==28) {
        System.out.println ("31");
    }
    else if (i==29) {
        System.out.println ("32");
    }
    else if (i==30) {
        System.out.println ("41");
    }
    else if (i==31) {
        System.out.println ("42");
    }
    else {
        System.out.println ("Invalid entry for the day. Try again.");
    }
}
}

The best way to shorten the code would be to write a formula for it.  That formula should be packaged into a function:

public static String outputForDay(int day) {
    if (!(1 <= day && day <= 31)) {
        throw new IllegalArgumentException("Invalid entry for the day.");
    }
    int onesDigit = day % 10,
        tensDigit = day / 10;
    return Integer.toString(10 * (tensDigit + 1) + (onesDigit % 4) + 1);
}

Then, you would call the function like this:

public static void main(String[] args) {
    try (Scanner scanner = new Scanner(System.in)) {
        System.out.print("Enter the day of the month: ");
        System.out.println(outputForDay(scanner.nextInt()));
    } catch (IllegalArgumentException e) {
        System.out.println(e.getMessage());
    }
}


Checking user authorization def user_not_authorized(exception)
    message = t('flash.access_denied')
    if exception.policy.class.to_s.underscore == 'group_policy' && current_user.students.size != 0
        current_user.students.each do |student|
          student.memberships.each do |membership|
            if membership.group_id == exception.record.id
              message = t('flash.pending_acceptance')
              break # and I would like to break from the outer loop too
            end
          end
        end
    end
    flash[:alert] = message
    redirect_to request.referrer || root_path
  end

This looks ugly to me. Is there a way to somehow compress those two each loops, where one should break both of them conditionally?

There's no reason (as far as I can tell) to do string comparison to check a class. #is_a? would presumably work fine too. And it'd be stricter, which is desirable.
There's no reason to check students.size. If there are no students, the loop just won't do anything, so why bother checking first? Conversely, if there are any students you're actually doing 2 database queries: One to get the count/size of students, and another to fetch the actual records.
Presuming non-ancient Rails, you can simply say current_user.students.memberships to get all the memberships in 1 query (admittedly with a couple of JOINs), rather than loop through students individually, and then getting their memberships.
Continuing from #3, you can (presumably) collapse the entire things into 1 database query.

I'd probably start with something like

def user_not_authorized(exception)
  redirect_path = request.referrer || root_path

  if exception.policy.is_a?(GroupPolicy)
    if current_user.students.memberships.where("group_id = ?", exception.record.id).any?
      redirect_to redirect_path, alert: t('flash.pending_acceptance') and return
    end
  end

  redirect_to redirect_path, alert: t('flash.access_denied')
end

Note that it doesn't actually load any records at all; it just checks if they're there or not.

In practice, I'd consider breaking it into separate methods where possible. It's not that great to have a naked query referencing a specific column name in the middle of your method.

Edit: Updated the code to return if the first redirect occurs, since you'd otherwise do two redirects in one action, which is not allowed. Thanks to tokland for catching that. Now the and return fix is about as quick and dirty as is gets, but check the comments for better ideas.


Debugging based on a debug level without performance impact? In a web application I am writing, I have used the following:

def dbgmsg(lvl, fmt, ...):
    if dbglvl >= lvl:
        # ... format and write the message
        pass # just here for correctness

Which is then used inside the code as:

dbgmsg(3, "Something happened")

Since there is no thing such as a the #ifdef in Python to my knowledge, I need to include the code at runtime. Will the above code cause a performance impact due to the call being made even when the globally set dbglvl is 0? Which alternatives exist?

It's going to take some time for Python to run the test dbglvl >= lvl. You can estimate the amount of time using the timeit module. For example, on my computer,

>>> from timeit import timeit
>>> def f1():
...     pass
...
>>> def f2():
...     if x > 0:
...         pass
...
>>> x = 0
>>> timeit(f1)
0.14283514022827148
>>> timeit(f2)
0.18653011322021484

So the if x > 0: test costs about 40 ns. If this is going to be a problem for you, then Python is probably not a suitable language for your application.
You can avoid the test altogether by having a separate name for the logging function at each debug level and then assigning a no-op function to the levels that don't apply.

def debug_message_write(format, *args):
    debug_output.write(format.format(*args))

def debug_message_ignore(format, *args):
    pass

debug_message_1 = debug_message_write if debug_level >= 1 else debug_message_ignore
debug_message_2 = debug_message_write if debug_level >= 2 else debug_message_ignore
# etc.

(You could avoid the repetition here by writing to the globals() dictionary in a loop.)

And then in the code you would call

debug_message_3("Opening {}", filename)

or whatever. But this is more complicated than your original approach and I doubt that the minuscule saving in time is worth it.
Your code would be easier to read if you wrote debug_message instead of dbgmsg and debug_level instead of dbglvl. There is no need for you to save on vowels: there is no danger of running out.


Function for reading bits I've made this simple function for reading N bits from an unsigned char*  (with a possible offset) and I'm willing to share it here, get a review, suggestions for improving it, etc...

typedef struct bits_t {
    unsigned char *data;
    unsigned int len;
} bits_t;

bits_t *read_bits(unsigned char *src, int bits_offset, int nbits){

    unsigned int curr_bit, curr_byte, remaining_to_read, bit_position_in_byte;

    curr_bit = curr_byte = 0;
    remaining_to_read = nbits;

    bits_t *bits = malloc(sizeof(bits_t));

    bits->len = nbits;
    bits->data = calloc(1, bits->len);

    for(curr_bit = 1; curr_bit <= nbits; curr_bit++){
        if(curr_bit >= bits_offset && remaining_to_read){
            curr_byte = (curr_bit - 1) / 8;
            bit_position_in_byte = (curr_bit - 1) - (curr_byte * 8);
            bits->data[remaining_to_read - 1] = read_bit(src[curr_byte], bit_position_in_byte);
            remaining_to_read--;
        }
    }

    return bits;
}

Main:

unsigned char *x = "oo";
bits_t *bits = read_bits(x, 0, 16);
for(i = 0; i < bits->len; i++){
    printf("%d", bits->data[i]);
}

Result: 0110111101101111

Consider checking src[currentByte] for '\0' and abort when it's true (apparently the passed in string wasn't long enough). Depends on your usage though.
Alternatively to the previous point you could add a source_length parameter to let the caller supply the information of how long the sequence really is.
I would consider changing the interface slightly. Have some methods which manage the object for you:

bits_t *new_bits_t(int nbits)
{
     bits_t *res = malloc(sizeof(bits_t));
     res->data = calloc(nbits);
     res->len = nbits;
     return res;
}

// frees the memory allocated for the structure and sets the reference to NULL
void delete_bits_t(bits_t **bits)
{
     if (bits == NULL || *bits == NULL) return;
     free((*bits)->data);
     free(*bits);
     *bits = NULL;
}

Then your read_bits function could fill in the  structure passed in and also return error code in case the reading failed (i.e. source is too short):

int read_bits(unsigned char *src, bits_t *bits, int bits_offset)
{
    ...
}

bit_position_in_byte = (curr_bit - 1) - (curr_byte * 8); should be equivalent to bit_position_in_byte = (curr_bit - 1) % 8;
Your loop is one based for some reason but inside the loop you have to subtract 1 from current_bit everywhere. You should just make your loop 0 based which would de-clutter the loop a bit.
I would find bit_in_byte just as descriptive a name as bit_position_in_byte.
You could get rid of the remaining_to_read by calculating the position:

for (curr_bit = 0; curr_bit < nbits; ++curr_bit) {
    input_bit = curr_bit + bits_offset - 1;
    input_byte = input_bit / 8;
    bit_in_byte = input_bit % 8;
    bits->data[nbits - curr_bit - 1] = read_bit(src[input_byte], bit_in_byte);
}

The loop apparently fills data from the end which seems little bit unexpected to me (kind of string reversal).


Not sure if this will help, but I'm using this to find nearest in a sorted column: (time series stuff)

result_index = df['col_to_search'].sub(search_value).abs().idxmin()

.sub(search_value) subtracts search_value from the df[col_to_search] to make the nearest value almost-zero,
.abs() makes the almost-zero the minimum of the column,
.idxmin() yields the df.index of the minimum value, or the closest match to search_value.

I got this approach from a similar search, but didn't note the original author or the site I found it.


Simple function to Http GET some data This is within a base class of a library, where several other classes inherit this one.

This seems like the simplest way of just getting some data, for now without timeout/errorhandling. This is supposed to get some data that will most ofthe time be converted to a small JSONObject.

Do you think I should put everything inside the try and use val? Any other suggestions? I'm basically just learning networking with android, only did it in C#.NET before.

protected fun sendGetRequest(url: URL): String {
    var urlConnection: HttpsURLConnection? = null
    var reader: InputStreamReader? = null
    var responseData = ""
    try {
        urlConnection = url.openConnection() as HttpsURLConnection

        reader = InputStreamReader(urlConnection.inputStream)
        responseData = reader.readText()
    }catch (e: Exception){
        this.messageHandler.handleMessage(e.message!!)
    }finally {
        reader?.close()
        urlConnection?.disconnect()
    }
    return responseData
}

The core of your code can be replaced by the following code:

url.openConnection()    //you can cast to fail if it isn't https
   .getInputStream()   //same as you
   .reader()          // same as wrapping it in inputStreamReader
   .use { it ->      // start lambda that handles the closing of the reader for you
      it.readText() // note, it's one parameter, so 'it ->' can be omitted
    }

The code above returns the text or throws the exception it got during execution, but always closes the reader, which in turn closes the connection.

The code above is almost the same as the code below btw:

url.readText()

If you want to remove all the variables, you should know that try catch can return a value:

val a = try{ 1 } catch(e: Exception) { 2 }


Login system, PDO and me - Part 2 This is a follow up question to: Login system, PDO and me

Is this considered secure for a login script?

if (filter_input(INPUT_POST, "login", FILTER_UNSAFE_RAW)) {
    $usercookie = 'Admin-'.uniqid(mt_rand(), true);

    $lastlogin = date('d / m / y - H:ia');
    $session = uniqid(mt_rand(), true);
    $active = 1;

    $username = filter_input(INPUT_POST, "username", FILTER_SANITIZE_STRING, FILTER_FLAG_STRIP_LOW);
    $password = filter_input(INPUT_POST, "password", FILTER_UNSAFE_RAW);

    $query = 'SELECT password FROM wcx_admin WHERE username = :username';

    $stmt = $backend->queryIt($query);
    $stmt = $backend->bind(':username', $username);
    $stmt = $backend->execute();
    $dbpass = $backend->getColumn();

    if(password_verify($password, $dbpass)) {
        $query = 'UPDATE wcx_admin SET active = :active, lastlogin = :lastlogin, session = :session WHERE username = :username';
        $stmt = $backend->queryIt($query);
        $stmt = $backend->bind(':active', $active);
        $stmt = $backend->bind(':lastlogin', $lastlogin);
        $stmt = $backend->bind(':session', $session);
        $stmt = $backend->bind(':username', $username);
        $login = $backend->execute();
    }

    if($login) {
        $_SESSION['loggedin'] = $session;
        $_SESSION['auth'] = $session;
        setcookie( 'wcxadmin', $usercookie, time()+3600);
        header('Location: index.php');
    }
    else {
        $_SESSION['errors'] = 'Incorrect Username or Password';
        header('Location: index.php');
    }
}

The bind function:

public function bind($param, $value, $type = null){
if (is_null($type)) {
    switch (true) {
        case is_int($value):
            $type = PDO::PARAM_INT;
            break;
        case is_bool($value):
            $type = PDO::PARAM_BOOL;
            break;
        case is_null($value):
            $type = PDO::PARAM_NULL;
            break;
        default:
            $type = PDO::PARAM_STR;
    }
}
$this->stmt->bindValue($param, $value, $type);
}

Error output:

if(isset($_SESSION['errors'])) {
    echo '
'.$_SESSION['errors'].'
';
    $_SESSION['errors'] = '';
}

You're probably sick of having me answer your questions, but I couldn't help myself on this one!

Basically, yes this is safe. However, you never specified what you wanted to protect against, so I'm assuming you mean from just a normal person who might try a couple tricks on you!

But nothing is perfect, and I'll see what I can do to point that out...

First off, I don't understand your first line.

filter_input(INPUT_POST, "login", FILTER_UNSAFE_RAW)

Are you checking to see if a "login" was sent? Your filter (FILTER_UNSAFE_RAW) doesn't do anything to the string if you don't specify other filters:

  Do nothing, optionally strip or encode special characters.

So I'm assuming it's just a check to see if the correct POST is there? If that's the case, what's wrong with using $_POST['login']?
I think your $usercookie is considered to be a security aspect, no? Well according to the docs:

  This function does not create random nor unpredictable strings. This function must not be used for security purposes.

And from the mt_rand() page...

  This function does not generate cryptographically secure values, and should not be used for cryptographic purposes.

And then a little bit further...

  If you need a cryptographically secure value, consider using openssl_random_pseudo_bytes() instead.

I think we all know what needs to be done here. (Get rid of of two functions you're using!)
Why are you not handling timestamps in $lastlogin instead of the database? The database has functionality meant for this. I believe you're using MySQL.
$session has the same security issue $usercookie had.
I would put all of these variables inside the first if.
For $username, I hope when someone registers, you alert them of the invalid characters that might be in their name. If you automatically strip them, they'll be very confused when the go to log in!
FILTER_UNSAFE_RAW does nothing to the $password. What's it point again?
I'm going to make an adjustment here:

    $login = $backend->execute();
}

if($login) {
    $_SESSION['loggedin'] = $session;
    $_SESSION['auth'] = $session;
    setcookie( 'wcxadmin', $usercookie, time()+3600);
    header('Location: index.php');
}
else {
    $_SESSION['errors'] = 'Incorrect Username or Password';
    header('Location: index.php');
}

And change all this to...

    if($backend->execute()) {
        // Sessions, cookie, and headers
    }
}
$_SESSION['errors'] = 'Incorrect Username or Password';
header('Location: index.php');

I think that makes the code more concise.
Will $_SESSION['loggedin'] ever be different than $_SESSION['auth']? You set them equal to each other, which I think could be avoided.
Your bind function looks fine, clean too.
Error checking seems fine too!

Good improvement though :)


PHP application configuration I've started looking at Heroku for application hosting and deployment and it appears that my configuration constants need to be stored in environment variables. I can do this in Heroku, but my application might also be deployed to shared servers and it may not be secure according to this question.

Basically I want the information to be read from a configuration file if it is present, otherwise try to read from the environment variables (in the case of Heroku).

Is this the best way of doing it?
Are there security implications?

settings.php

<?php

define('DB_URL', 'mysql://root:password@localhost:3306/database');
define('APP_URL', '[::1]');
define('APP_URL_PATH', '/path/to/application');
define('FOO', 'more things');

config.php

<?php

if (!is_file('settings.php')) {
    require 'settings.php';
    return;
}

define('DB_URL', getenv('DB_URL'));
define('APP_URL', getenv('APP_URL'));
define('APP_URL_PATH', getenv('APP_URL_PATH'));
define('FOO', getenv('FOO'));

index.php (and other application files)

<?php

require 'config.php';

$app = new \Slim\Slim\App();

// code

$app->run();

I'm wondering if settings.php is in the same directory tree where you have other scripts like index.php served by the web server.
I don't know if this is possible on Heroku,
but I recommend to move files that should never be served by the web server outside of the document root.

One time, I had a web server malfunction: the well-tuned configuration file got replaced with something primitive, and php disabled, where visiting settings.php in a browser would simply display the contents of the file without running through the PHP interpreter. Sure, this seems unlikely to happen, but by moving sensitive files outside the document root, you can effectively safeguard against such or similar misconfiguration issues. I also find it good organization to separate public files from internal details. So my PHP projects all have a public/ directory inside, containing practically views only, the model is elsewhere, cleanly separated.

About this:

define('DB_URL', 'mysql://root:password@localhost:3306/database');

Even if it's a dedicated and isolated database for your project only,
I don't see why would you ever use the root account.
Create a dedicated user with carefully selected grants instead.

Needless to say, settings.php should have restrictive permissions in the filesystem.


Adjust bootstrap dropdown menu based on page width I'm using Twitter Bootstrap's navbar. While testing different screen resolutions, I noticed that on lower resolutions, sometimes the submenu would get cut off by the right side of the page.

In the code below, when a dropdown is opened, I'm getting the width of the dropdown menu and dropdown submenu, adding those together and checking them against the width of the window. If the width is greater than the window width, I add the class pull-right so the dropdown menu and/or dropdown submenu will extend to the left and not get cut off by the right side of the page.

In this jsFiddle, you can see what I'm talking about by increasing/decreasing the width of the Results section. When you increase the width of the Results section far enough, the submenu for 'Dropdown 2' will open to the right. When you decrease the size of the Results section enough that the submenu would get cut off, the 'pull-right' class is applied and it extends to the left.

For the most part, this code is doing what it should, but does anyone have any suggestions on how to improve it?

JavaScript

$('.dropdown-toggle').click(function() {
    var dropdownList = $('.dropdown-menu');
    var dropdownOffset = $(this).offset();
    var offsetLeft = dropdownOffset.left;
    var dropdownWidth = dropdownList.width();
    var docWidth = $(window).width();

    var subDropdown = $('.dropdown-menu').eq(1);
    var subDropdownWidth = subDropdown.width();

    var isDropdownVisible = (offsetLeft + dropdownWidth <= docWidth);
    var isSubDropdownVisible = (offsetLeft + dropdownWidth + subDropdownWidth <= docWidth);

    if (!isDropdownVisible || !isSubDropdownVisible) {
        $('.dropdown-menu').addClass('pull-right');
    } else {
        $('.dropdown-menu').removeClass('pull-right');
    }
});

Please let me know if I need to better my explanation or if any other details are needed.

I came across this post searching for a fix to a slightly different problem. I have long entries in a drop-down menu that get cut off on small screen sizes.
I found that if I allow the content in the dropdown to wrap it automatically gets adjusted to the page width:

.dropdown-menu > li > a {
    white-space: normal;
}

If you don't want it to wrap if there is enough space, add no-wrap back in with a media query. Because it is so simple I figured I leave this here for others, even though it does not directly apply to the cascading dropdown in your example.


Simple Timed Cache by Wrapping HashMap Just wanted to get a bit of feedback on this simple cache. My use case is to store this data on an Android client to avoid making a high volume of network calls for lookups.

I feel like maybe I should just extend HashMap with the small amount I've added, or change up my thinking and do some type of static implementation?

public class DataCache<K, V> {
    private final long mDefaultTimeout = 15;
    private long mTimeout = 0;
    private HashMap<K, DataValue<V>> dataMap;

    DataCache() {
        dataMap = new HashMap<K, DataValue<V>>();
        mTimeout = mDefaultTimeout;
    }

    DataCache(long timeoutMinutes) {
        dataMap = new HashMap<K, DataValue<V>>();
        mTimeout = timeoutMinutes;
    }

    public void put(K key, V value) {
        dataMap.put(key, new DataValue<V>(value));
    }

    public V get(K key) {
        DataValue<V> data = dataMap.get(key);
        V result = null;
        if (data != null) {
            long diff = TimeUnit.MILLISECONDS.toMinutes(System.currentTimeMillis() - data.insertTime);
            if (diff >= mTimeout) {
                dataMap.remove(key);
                data.value = null;
            }
            result = data.value;
        }
        return result;
    }

    public void setTimeout(long minutes) {
        mTimeout = minutes;
    }

    public long getTimeout() {
        return mDefaultTimeout;
    }

    private final class DataValue<T> {
        public T value;
        public long insertTime;

        DataValue(T value) {
            this.value = value;
            insertTime = System.currentTimeMillis();
            }
        }
    }
}

Small code duplication

The two constructors of DataCache have duplicated code:

DataCache() {
    dataMap = new HashMap<K, DataValue<V>>();
    mTimeout = mDefaultTimeout;
}

DataCache(long timeoutMinutes) {
    dataMap = new HashMap<K, DataValue<V>>();
    mTimeout = timeoutMinutes;
}

The difference between the default constructor and the second one is the use of a default timeout value. It would be better to invoke the second constructor from the default one to avoid duplication:

DataCache() {
    this(mDefaultTimeout);
}

DataCache(long timeoutMinutes) {
    dataMap = new HashMap<K, DataValue<V>>();
    mTimeout = timeoutMinutes;
}

Logic of code

You have a DataValue class that holds the time at which it was inserted in the map. For the map itself, this should be transparent: it doesn't know what the value is doing to find out if it's outdated or not. The field long insertTime goes in that direction.

But then, you are using

long diff = TimeUnit.MILLISECONDS.toMinutes(System.currentTimeMillis() - data.insertTime);

inside the map itself. This means that, somehow the map has figured it out: it knows that the values are following the current system time to know if they're outdated or not. So this couples the map with the logic of the expiration of the value.

I would suggest creating a method isExpired(timeOut) inside the class DataValue, that will tell if a given value is expired or not based on the current timeout.

private final class DataValue<T> {
    public T value;
    private long insertTime;

    DataValue(T value) {
        this.value = value;
        insertTime = System.currentTimeMillis();
    }

    public boolean isExpired(long timeOut) {
        return TimeUnit.MILLISECONDS.toMinutes(System.currentTimeMillis() - insertTime) >= timeOut;
    }

}

Note that, in the mean time, I also made the insertTime variable private because, now, the map doesn't need to know that information. The advantage of that approach is that, someday, if you decide not to rely on System.currentTimeMillis() but on something else entirely (reading info from a file, always return true, etc.), you just need to change that single method.

In the same way, I would move the

data.value = null;

logic, that reinitializes the value to a method inside the DataValue class, and make value private.

Constants

In Java, a constant, like your mDefaultTimeout should be both static and final. Also, it should be written in uppercase. So instead, I would have

private static final long DEFAULT_TIMEOUT = 15;

Note that you could add a comment to specify the unit for which 15 refers to (Is it seconds? minutes?).

Namings

Why are you prefixing all your variables by m? This does not really follow the Java naming conventions. It would be preferable to have

private long timeout = 0;


Scheme/Racket: idiomatic infix math evaluator Inspired by xkcd and a couple of praising blog posts, I decided to try out Lisp. It seemed that the best-supported dialect was Racket, itself a variant of Scheme, so I went with that and wrote an infix mathematical expression parser to get familiar with the basic concepts.

That was a mind-bending experience considering I never did functional programming. Not having mutable variables (or not using them, anyways) and having to use recursion instead of loops was a little hard for my imperative mind.

Anyways, I came up with this. My goal was to parse infix mathematical expressions with correct precedence and parentheses. It supports additions, subtractions, multiplications and divisions. It works, but it's probably full of hints that I'm new at this, and I would like to know which parts could have been made more idiomatic.

#lang racket

;; usage: (reduce (tokenize))

;; tokenizing function
(define (tokenize)
  (define (-tokenize-operator first)
    (cond
      ([equal? first #\+] (list + (read-char)))
      ([equal? first #\-] (list - (read-char)))
      ([equal? first #\*] (list * (read-char)))
      ([equal? first #\/] (list / (read-char)))))

  (define (-tokenize-number first)
    (define (--char->number char)
      (let ([ascii-zero (char->integer #\0)])
        (- (char->integer char) ascii-zero)))

    (define (--read-number initial)
      (let ([char (read-char)])
        (if (char-numeric? char)
            (--read-number (+ (--char->number char) (* initial 10)))
            (list initial char))))

    (if (char-numeric? first)
        (--read-number (--char->number first))
        '()))

  (define (-tokenize-openparen first)
    (if (equal? first #\()
        (list #\( (read-char))
        '()))

  (define (-tokenize first endchar)
    (if (equal? first endchar)
        '()
        (let ([operator (-tokenize-operator first)]
              [number (-tokenize-number first)]
              [openparen (-tokenize-openparen first)])
          (cond
            ([pair? operator] (cons (car operator) (-tokenize (cadr operator) endchar)))
            ([pair? number] (cons (car number) (-tokenize (cadr number) endchar)))
            ([pair? openparen] (list (-tokenize (cadr openparen) #\))))
            (else (tokenize))))))

  (let ([first (read-char)])
    (-tokenize first #\newline)))

;; parsing and evaluation function
(define (reduce tokens)
  (define (-operator-priority op)
    (cond
      ([ormap (lambda (p) (equal? p op)) (list + -)] 1)
      ([ormap (lambda (p) (equal? p op)) (list * /)] 2)))

  (define (-rvalue list max-priority)
    (define (--lower-parentheses parenthesed-expression next-tokens)
          (let ([paren-result (car (-rvalue parenthesed-expression 0))])
            (-rvalue (cons paren-result next-tokens) max-priority)))

    (define (--reduce-rvalue lvalue next-tokens)
      (let* ([operator (car next-tokens)]
             [priority (-operator-priority operator)]
             [then (cdr next-tokens)])
        (if (> priority max-priority)
            (let* ([rvalue (-rvalue then priority)]
                   [value (operator lvalue (car rvalue))])
              (-rvalue (cons value (cdr rvalue)) max-priority))
            list)))

    (let ([lvalue (car list)]
          [next-tokens (cdr list)])
      (if (pair? lvalue)
          (--lower-parentheses lvalue next-tokens)
          (if (pair? next-tokens)
              (--reduce-rvalue lvalue next-tokens)
              list))))

  (car (-rvalue tokens 0)))

Thanks!

Here are my review comments on the tokeniser:

It's unusual to prefix symbols with -. The indentation should be enough to tell you the "nesting level" of the inner functions, without having to put -s in front.
Your inner -tokenize could be hoisted into a named let, so that your tokenize function looked like this (I will make other changes to the function later, but for now I try to keep as close to your original code as possible):

(define (tokenize)
  ;; other internal definitions

  (let recur ([first (read-char)]
              [endchar #\newline])
    (if (equal? first endchar)
        '()
        (let ([operator (-tokenize-operator first)]
              [number (-tokenize-number first)]
              [openparen (-tokenize-openparen first)])
          (cond
            ([pair? operator] (cons (car operator) (recur (cadr operator) endchar)))
            ([pair? number] (cons (car number) (recur (cadr number) endchar)))
            ([pair? openparen] (list (recur (cadr openparen) #\))))
            (else (recur (read-char) #\newline)))))))

Character comparisons can use eqv? instead of equal?. Not really a biggie, but usually I reserve equal? for deep comparisons.
Use #f for false/null values, not '(). In Scheme, #f is the only false value there is; '() is a true value (like everything else). Though, instead of all those different tokenising helpers, and calling them all and then only using one of the return values, it would be nicer to test directly in the cond.
Instead of all the repeated (read-char)s, you might consider using a port stream instead. Create one using sequence->stream, then you can call stream-first and stream-rest on it.

With all that in mind, I'd write the tokeniser like so:

(define (tokenize [port (current-input-port)])
  (let recur ([str (sequence->stream (in-input-port-chars port))])
    (cond [(stream-empty? str) '()]
          [(char-numeric? (stream-first str))
           (let loop [(str str)
                      (digits '())]
             (if (and (not (stream-empty? str))
                      (char-numeric? (stream-first str)))
                 (loop (stream-rest str) (cons (stream-first str) digits))
                 (cons (string->number (list->string (reverse digits)))
                       (recur str))))]
          [(char-whitespace? (stream-first str))
           (recur (stream-rest str))]
          [else (cons (stream-first str) (recur (stream-rest str)))])))

(Comments on the evaluator to come later.)


Conversion to typescript from javascript This is my first attempt in converting a pure javascript to typescript. Any comment?
const apiUrl = 'https://localhost:123'

export function webApiUrl() {
    return apiUrl;
}

export function downloadPdf(id : number, fileName: string) {
    const spinner: HTMLElement = document.getElementById('spinner') as HTMLImageElement

    spinner.style.display = 'block'

    fetch(`${apiUrl}/api/file/download?fileDocumentId=${id}`).then(resp => resp.arrayBuffer()).then(resp => {

        // set the blog type to final pdf
        const file = new Blob([resp], { type: 'application/pdf' });

        // process to auto download it
        const fileURL: string = URL.createObjectURL(file);
        const link: HTMLAnchorElement = document.createElement('a');
        link.href = fileURL;
        link.download = fileName;
        link.click();
        spinner.style.display = 'none'
    });
}

I think the only code I can't figure out how to convert over is the blob type.

Use explicit type annotation only when necessary In the vast majority of cases, TypeScript can infer the type of an expression automatically, without you having to note it manually. It's probably better and easier to let TS take care of it - it makes for less code to read and write. If you're ever not sure what sort of type an expression is, use an IDE which can tell you (like VSCode, which works really well with TypeScript - just hover over an expression and it'll tell you what type TypeScript has inferred for it).
For example:
const fileURL: string = URL.createObjectURL(file);
const link: HTMLAnchorElement = document.createElement('a');

can be
const fileURL = URL.createObjectURL(file);
const link = document.createElement('a');

IMO, TypeScript shines most when it looks almost identical to JavaScript, with the added bonus of warning you when you're doing something that's type-unsafe.

I think the only code I can't figure out how to convert over is the blob type.

Its type is a Blob, which you can see in VSCode (or any other good TS-aware IDE):

But, as said, better to let TS infer it automatically; no need for const file: Blob =.
Prefer generics over type assertion Generics are the most elegant way to denote the type of a particular expression, when the type isn't set in stone by the function signature. The as keyword, in contrast, is less type-safe: as tells TypeScript "I know exactly what I'm doing, ignore whatever type this value was determined to be previously and assume that it's of THIS type instead".
For example, the following code does not throw a TS error, despite the as clearly being nonsensical:
const foo = {
    bar: 'bar'
} as {
    fn?: () => Promise<Array<string>>
};

as isn't as bad as any, but they both should be avoided when possible.
In your case, getElementById is not generic, unfortunately, but querySelector is. You can change:
const spinner: HTMLElement = document.getElementById('spinner') as HTMLImageElement

to
const spinner = document.querySelector<HTMLElement>('#spinner')!;
// you could also use <HTMLImageElement> if you needed image element specific types

Generics are essentially type arguments that come in <> brackets, before the expression arguments inside the parentheses of the function call. Above, <HTMLImageElement> tells querySelector: "This selector, if it matches, will match an HTMLImageElement." This is preferable to as because only the types that extend Element can be passed to querySelector.
Function name Functions should describe actions, or what they do. webApiUrl might be more precisely named getApiUrl. (Or, just export the URL string alone: export const apiUrl = '...';, there doesn't seem to be any need for the function)
Error handling If the fetch fails, no indication is given to the user or to other parts of the program. Whenever you have a Promise, you should almost always make sure that possible errors inside it get handled. TSLint rule: no-floating-promises.
While you could add a .catch onto the fetch Promise chain:
    spinner.style.display = 'none'
})
.catch((error) => {
    // handle errors
});

It might be better to let the caller handle errors as needed. Change:
fetch(`${apiUrl}/api/...

to
return fetch(`${apiUrl}/api/...

so that each caller can do downloadPdf(id, fileName).catch(handleErrors) individually.
Semicolons Some of your lines are missing semicolons. This is stylistically inconsistent and can lead to odd bugs if you forget a semicolon where it's required. Add the missing semicolons, and consider using a linter which warns you when they're missing. (The rationale behind using a linter is similar to the rationale behind using TypeScript - turn annoying, hard-to-encounter, sometimes-hard-to-debug runtime errors into trivially fixable compile-time errors.)


Using generators on Observable class I've decided to implement a simple case of the Observer pattern. I've gone about this considering you can only register 1 single observer in the Observable, instead of allowing multiple ones.

I would really like feedback about what'd be the proper way to return the listener in the run method while still yielding values, if possible i'd like to allow some sort of method chaining

Overall I would like some criticism about this whole design and of course, anything else is on the table will be appreciated.

Code:

import time

class Observer:

    def __init__(self):
        self.lines = []
        self.execution_time = None

    def on_process(self, value):
        line = f"<< processed: {value} >>"
        self.lines.append(line)
        return line

    def on_done(self, value):
        self.execution_time = value

class Observable:

    def __init__(self):
        self.listener = None

    def register_listener(self, listener):
        self.listener = listener
        return self

    def run(self, iterable):
        s = time.time()
        for v in iterable:
            yield self.listener.on_process(v)

        self.listener.on_done(time.time() - s)

if __name__ == '__main__':
    obj = Observable().register_listener(Observer())

    for x in obj.run(range(10)):
        print(x)

    listener = obj.listener
    print("Number of lines:", len(listener.lines))
    print("Execution time:", listener.execution_time)

You haven't implemented the observer pattern as your subject, Observable, doesn't contain a list of observers.

Whilst this may seem like some small petty point it is the core problem that the pattern hopes to solve.
You have failed to make the subject relay to its observers. As you have moved the code, run, that should be in the observer into the subject.

This makes the observer interface neigh on useless, because if you want to change this functionality then you're screwed.

An example of the observer pattern is in Python's logging library.
In this you have events like a logger.info(message).
The logger, the subject, goes on to call the underlying handlers, observers, with the event.
This causes the handler to handle the event how it's been designed to.

It's common to have a StreamHandler and sometimes a FileHandler.
The former that just prints the message, and the latter that writes the message to a file.


Find the two largest integers in a list I wanted to know if there was any way to improve on this solution. I believe it uses the least amount of variables. Also wanted to know how are my variable names? Are they sufficient? Do I need anymore indenting? I'm looking for perfection in a way. At least a clear, correct and readable program.

/*
 * File: FindTwoLargest.java
 * ----------------------
 * This program finds the two largest integers in a list.
 *
 */

/* Library packages */
import acm.program.*;

public class FindTwoLargest extends ConsoleProgram {
    public void run() {
        println("This program finds the two largest integers in a list.");
        println("Enter values, one per line, using a 0 to signal the end of list.");
        int n = readInt("? ");
        int largest = n;
        int secLargest = n;
        while (true) {
            n = readInt("? ");
            if (n == SENTINEL) break;
            secLargest = n;
            while (true) {
                n = readInt("? ");
                if (n == SENTINEL) break;
                if (n > largest) {
                    secLargest = largest;
                    largest = n;
                } else if (n < largest && n > secLargest) {
                    secLargest = n;
                }
            }
            break;
        }
        println("The largest value is " + largest);
        println("The second largest value is " + secLargest);
    }

    /* Private constant */
    private static final int SENTINEL = 0;
}

Please bare in mind that this is my first review on here :)

I see that you wanted to do the whole sorting yourself and personally I think that's good to understand some basics. However in aspect of perforfmance and readability it's not quite the best.
Why re-invent the wheel when you can use already existing solutions?

 /*
 * File: FindTwoLargest.java
 * ----------------------
 * This program finds the two largest integers in a list.
 *
 */

/* Library packages */
import acm.program.*;

public class FindTwoLargest extends ConsoleProgram {

    /* Private constant */
    private static final int SENTINEL = 0;

    public void run() {
        println("This program finds the two largest integers in a list.");
        println("Enter values, one per line, using a 0 to signal the end of list.");
        List<Integer> userInput = new ArrayList<>();
        while (true) {
            int n = readInt("? ");
            if (n == SENTINEL)
            {
              break;
            }
            userInput.add(n);
        }
        if(!userInput.isEmpty()) {
          java.util.Collections.sort(userInput);
          java.util.Collections.reverse(userInput);
          println("The largest value is " + userInput.get(0));
          println("The second largest value is " + userInput.get(1));
       }
    }
}

So that's a solution I would use. It only uses one while loop which is enough to work and uses less If-Else logic. You can just collect every input given to the list and then use the Collections API from Java to easily sort the list. The call to reverse will change the sorting from ascending to descending meaning the hightest and second highest values are at the beginning.

However if that is not what you wanted or needed I would ask why you used two while loops? It's not necessary for the logic.
You can check everything in the first while and remove duplicate code and unneeded checks, which reduce the readability and maintenance of the code.

Another hint would be that with else if you indicate that the previous condition was checked and didn't comply, meaning no need to check again what you did in the first if. You would only need to check that again if you just used a second if.

Which also improves some maintenance and readability, is moving constants and global variables before the constructor of a class. In case you didn't have contact with that yet, a Constructor is the first method in a class ( used for defining the initialization and so on but that doesn't belong here ) and comes before all custom methods. Meaning it's at the top of a class. And to easily get a grip of what is global or where the constants are you should always move those at the top of the class.

import acm.program.*;

public class FindTwoLargest extends ConsoleProgram {

    /* Private constant */
    private static final int SENTINEL = 0;

    public void run() {
        println("This program finds the two largest integers in a list.");
        println("Enter values, one per line, using a 0 to signal the end of list.");
        int n = readInt("? ");
        if (n == SENTINEL) {
            break;
        }
        int largest = n;
        int secLargest = n;
        while (true) {
            n = readInt("? ");
            if (n == SENTINEL) { // I just personally don't like inline if's
                break;
            }
            if ( n >= largest ) {
                secLargest = largest;
                largest = n;
            } else if ( n > secLargest ) {
                secLargest = n;
            }
        }
        println("The largest value is " + largest);
        println("The second largest value is " + secLargest);
    }
}

Note: I tested a slightly different variant of this in the ideone online compiler here.

So with the answer of Roland in mind here's what you can also do to fix those little bugs:

  1 5 5 0 (only one of the 5s is accepted, the other is ignored)

Just change the first if too if( n >= largest ) if you don't want to ignore the second entry.

  5 0 (there is no second number at all, nevertheless your program says
  that the second-largest number is 5)

Again this depends on what you want to do. If you don't want to display a secondlargest in this case you could do something like this:

int secondLargest = 0;

...
if(secondLargest != 0){
  println("The second largest value is " + secLargest);
}

  0 (basically works, but the output should indicate that there wasn't a
  number at all)

I just did the sentinel check for the first input already. That allows the user to quit the program directly. If that's not what you want to do you can think about something else.


Programatically creating context menu I have the need to programatically create a context menu using names for the menu items which aren't known until the program loads user data.

Here's how I have implemented it.  I'm not happy with the cast to get the menu item text to find out which item they have clicked on. Is there a better way that I'm missing?

public partial class Form1 : Form
{
    private readonly string[] cards = new string[] {"HBOS", "EGG", "RBOS"};

    ContextMenu mnuContextMenu = new ContextMenu();

    private void Form1_Load(object sender, EventArgs e)
    {
        ContextMenu = mnuContextMenu;

        SetupRightClickmenu();
    }

    private void SetupRightClickmenu()
    {
        MenuItem bank1MenuItem = new MenuItem(BankNames.Bank1);
        MenuItem bank2MenuItem = new MenuItem(BankNames.Bank2);
        MenuItem bank3MenuItem = new MenuItem(BankNames.Bank3);
        MenuItem cashMenuItem = new MenuItem("Cash");
        MenuItem creditCardMenuItem = new MenuItem("Credit Card");

        bank1MenuItem.Click += bank1MenuItem_Click;
        bank2MenuItem.Click += bank2MenuItem_Click;
        bank3MenuItem.Click += bank3MenuItem_Click;
        cashMenuItem.Click += cashMenuItem_Click;

        mnuContextMenu.MenuItems.Add(bank1MenuItem);
        mnuContextMenu.MenuItems.Add(bank2MenuItem);
        mnuContextMenu.MenuItems.Add(bank3MenuItem);
        mnuContextMenu.MenuItems.Add(cashMenuItem);
        mnuContextMenu.MenuItems.Add(creditCardMenuItem);

        MenuItem creditCardSubMenuItem;

        foreach(string card in cards)
        {
            creditCardSubMenuItem = new MenuItem();
            creditCardSubMenuItem.Text = card;
            creditCardMenuItem.MenuItems.Add(creditCardSubMenuItem);
            creditCardSubMenuItem.Click += creditCardSubMenuItem_Click;
        }
    }

    void creditCardSubMenuItem_Click(object sender, EventArgs e)
    {
        MenuItem menuItem = (MenuItem) sender;

        MessageBox.Show("You clicked on " + menuItem.Text);
    }

    void cashMenuItem_Click(object sender, EventArgs e)
    {
        MessageBox.Show("You clicked on cash");
    }

    void bank3MenuItem_Click(object sender, EventArgs e)
    {
        MessageBox.Show("You clicked on bank 3");
    }

    void bank2MenuItem_Click(object sender, EventArgs e)
    {
        MessageBox.Show("You clicked on bank 2");
    }

    void bank1MenuItem_Click(object sender, EventArgs e)
    {
        MessageBox.Show("You clicked on bank 1");
    }
}

In the actual version the cards array is received from the users data.

You can use the Tag property to store the card.  Its normal to store the context object in the Tag.

that way you can make the text whatever you like eg..

creditCardSubMenuItem.Text = string.Format("The card {0}", card);
creditCardSubMenuItem.Tag= card;


2-sum problem on a range in Python Here is the problem description (from an algorithm course: https://www.coursera.org/learn/algorithm-design-analysis):

  The goal of this problem is to implement a variant of the 2-SUM
  algorithm (covered in the Week 6 lecture on hash table applications).

  The file contains 1 million integers, both positive and negative
  (there might be some repetitions!). This is your array of integers,
  with the \$i\$th row of the file specifying the \$i\$th entry of the array.

  Your task is to compute the number of target values \$t\$ in the interval
  [-10000,10000] (inclusive) such that there are distinct numbers \$x,y\$ in
  the input file that satisfy \$x+y=t\$.

I wrote the following Python program. It runs already 25 minutes and just calculated 6200 of 20000 numbers.
This is too slow.

import sys

def has_two_sum(n,num_set,nums):
  res = any(((n-x) in num_set) and 2*x !=n for x in nums)
  return res

with open(sys.argv[1], 'r') as fp:
  nums = [int(line) for line in fp]
  num_set = set(nums)

two_sum = sum(1  for n in range(-10000,10001) if has_two_sum(n,num_set,nums))
print(two_sum)

Your code runs instantly on my system with 1 million randomly generated numbers. are you sure you are searching for n - x in num_set and not in nums?

Anyway, a couple of Python style pointers:

You can iterate over the set directly, no need to pass both the set and the list:

def has_two_sum(n, num_set):
    res = any(((n-x) in num_set) and 2*x != n for x in num_set)
    return res

Not much point in storing a value to immediuately return it:

def has_two_sum(n, num_set):
    return any(((n-x) in num_set) and 2*x != n for x in num_set)

Boolean values are converted to 0 or 1 when used in an integer setting, so your adding of values is typically written as:

two_sum = sum(has_two_sum(n, num_set) for n in range(-10000, 10001))

If you are going to have a function that contains a single liner, you better give it a great, descriptive name. I don't think that has_two_sum really fits the bill. Perhaps has_two_items_that_sum_to_n or has_two_that_sum_to_n? Alternatively, I think my preferred option here would be to get rid of the function altogether and let code speak by itself, writing the whole thing as two nested comprehensions:

two_sum = sum(any(n - x in num_set and 2*x != n for x in num_set)
              for n in range(-10000, 10001))

There is a school of thought that considers nested comprehensions to be confusing (e.g. the Google Python style guide doesn't allow them), but I think this case is simple enough, YMMV.


Saving images using Opencv in python Is there a better way to rewrite this part in python. It doesnot have to be shorter as long as its much better. thanks in advance

count = 1
while count < 10:
    _, pho = webcam.read()
    gr = cv2.cvtColor(im, cv2.COLOR_BGR2GRAY)
    images = face_cascade.detectMultiScale(gr, 1.3, 4)
    for (x,y,w,h) in faces:
            cv2.rectangle(pho,(x,y),(x+w,y+h),(0,255,0),2)
            fa = gray[y:y + h, x:x + w]
            e_size = cv2.resize(fa, (100, 100))
            cv2.imwrite('%s/%s.JPEG' % (folder,count), e_size)
    count += 1
    cv2.imshow('OpenCV', pho)
    key = cv2.waitKey(20)

A very simple change that replaces the while loop with a for loop (I don't know much about OpenCV, so I can't help too much there):

for count in range(1, 10):
    _, pho = webcam.read()
    gr = cv2.cvtColor(im, cv2.COLOR_BGR2GRAY)
    images = face_cascade.detectMultiScale(gr, 1.3, 4)
    for (x,y,w,h) in faces:
            cv2.rectangle(pho,(x,y),(x+w,y+h),(0,255,0),2)
            fa = gray[y:y + h, x:x + w]
            e_size = cv2.resize(fa, (100, 100))
            cv2.imwrite('%s/%s.JPEG' % (folder,count), e_size)
    cv2.imshow('OpenCV', pho)
    key = cv2.waitKey(20)

Additionally, in Python 3 especially, the % string formatting syntax is out-of-favor/deprecated -- use the {} and .format syntax instead:

cv2.imwrite('%s/%s.JPEG' % (folder,count), e_size)

becomes

cv2.imwrite('{!s}/{!s}.JPEG'.format(folder, count), e_size)


Writing a byte array image to disk with an automatically generated filename I have the following code that that writes large images to disk. My application server is running out of memory and I'm wondering if I could somehow optimize the following:

public void writeImgToDisk(byte[] base64AttachmentInBytes, String dmxi){

        String destinationAndFileName  = "";
        String fileNameMinusExtension = getCurrentTimeStampForFileNaming();
        String extension="";
        try {

               TikaConfig config = TikaConfig.getDefaultConfig();
               InputStream stream = new ByteArrayInputStream(base64AttachmentInBytes);

               MediaType mediaType = config.getMimeRepository().detect(stream, new Metadata());
               MimeType mimeType;

            try {
                mimeType = config.getMimeRepository().forName(mediaType.toString());
                extension = mimeType.getExtension();
                System.out.println("Extension is:"+ extension);

                File folder  = new File("Z:\\images\\"+ sref);
                if (!folder.exists()){
                    folder.mkdir();
                }
                destinationAndFileName  = "Z:\\images\\"+ dmxi + "\\" + fileNameMinusExtension+ extension;

                System.out.println("destinationAndFileName is:"+ destinationAndFileName);
            } catch (MimeTypeException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }

            InputStream input = new ByteArrayInputStream(base64AttachmentInBytes);
            OutputStream output = new FileOutputStream(destinationAndFileName);
            IOUtils.copy(input, output);

            System.out.println("It was written..");
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }

    }

This works but its the cause of a bottleneck during high volume calls.

You're not closing the output stream! If that doesn't fix your issue, you'll have to post more contextual code, because after you free the output stream, the rest of this method should just free its resources after it is done.