Next lexicographic permutation of a list I began learning Haskell last week.  I wrote the following to get the next lexicographic permutation of a list.  It works perfectly but I feel like I'm just writing a c# method.  My long list of where commands just feels like an imperative approach.  I would appreciate any feedback if this solution is indeed the way most people would write something like this in haskell:

-- Finds the end of a list that is monotonically decreasing (swap)
-- Pivot is the index just before it. (pivot)
-- exchanges the smallest value > pivot in swap with pivot
-- reverses the swap with the pivot in it, and concatenates the new permutation
nextPerm :: (Ord a,Num a) => [a] -> [a]
nextPerm [] = []
nextPerm a = if pivotIndex == (-1) then a
             else prePivot ++ [swapVal] ++ swapWithPivot
             where
                 swap = foldl (\acc x -> if x<=(last acc) then acc ++ [x] else [x]) [(-1)] a
                 pivotIndex = (length a) - (length swap) - 1
                 prePivot = take pivotIndex a
                 pivotVal = a !! pivotIndex
                 swapVal = foldl1 (\acc x -> if x > pivotVal then x else acc) swap
                 swapWithPivot = insert pivotVal $ delete swapVal $ reverse swap

I'm a beginner at Haskell as well, so take my opinion with a big rock of salt.

nextPerm :: (Ord a, Num a) => [a] -> [a]
nextPerm [] = []
nextPerm a

I wouldn't use a as a type variable and a variable at the same time.
I like to name lists xs, ys, cs, etc. I feel this is more descriptive, and it works well with pattern-matching, e.g. (x:xs), where x is the head of the list, xs is the tail.

swap = foldl (\acc x -> if x <= (last acc) then acc ++ [x] else [x]) [(-1)] a

Here you are going through the list from the front. When you use last, the program has to go through the whole list, because lists are kind of like linked lists, think [1,2,3] = 1 : 2 : 3 : [], so accessing an element takes \$O(n) \$ time.
For the same reason, (++) is expensive, while (:) is cheap. Try running swap on a list of \$ 10^5 \$ size; it's slow.

Other than the speed factor, I think your implementation is OK!

If you want to make it faster, first of all you should change swap. For example, first reverse the list, then work on it - as lists are made to be accessed from the front, not the back. Let me show you an improved version of swap along with a few other changes.

nextPerm :: Ord a => [a] -> [a]
nextPerm xs
  | null xs || pivotIndex == -1 = xs
  | otherwise = prePivot ++ swapVal : swapWithPivot
  where pivotIndex = length xs - length swap - 1
        prePivot = take pivotIndex xs
        pivotVal = xs !! pivotIndex
        swapVal = foldl1 (\acc x -> if x > pivotVal then x else acc) swap
        swapWithPivot = insert pivotVal . delete swapVal . reverse $ swap
        swap = reverse . fmap fst
                       . takeWhile (uncurry (>=))
                       . (<*>) zip (\ls -> head ls:ls)
                       . reverse $ xs

As you can see, this way, you don't even need to restrict yourself to the class Num.
To make it even better, figure out how to use span instead of take, and takeWhile. If you do that, you won't even need indexing anymore.   Use Hoogle, play around in ghci, look at types using :t <typename> e.g. :t span - type signatures (and names) will usually tell you what a function does.

Lists are weak at random access, but kind of good at being sliced up; therefore I'd definitely drop the whole indexing thing.

Anyway, I will include how I'd write it, hopefully it'll give you some new ideas.

nextPerm' :: Ord a => [a] -> [a]
nextPerm' = uncurry (++) . uncurry swap
                         . (fmap fst *** fmap fst)
                         . span (uncurry (>=))
                         . (<*>) zip (\ls -> head ls:ls)
                         . reverse
    where swap rpost [] = ([], rpost)
          swap rpost (pivot:rpre) = (reverse rpre, ins . span (<= pivot) $ rpost)
              where ins (le,[])   = le ++ [pivot]
                    ins (le,a:as) = a:le ++ pivot:as

If you want to test an implementation, issue

(sort . permutations $ [1,2,3,4,5]) == (take 120 $ iterate nextPerm [1,2,3,4,5])

If it works, the result will be True.


Importing a CSV file into Excel I need help making this code run faster.  I am using it to import a CSV file into Excel. It works, but it is very slow. The file is almost 20MB.

{Sub OpenTextFile()

Dim FilePath As String
Dim linitem As Variant
FilePath = "filepath.txt"
Open FilePath For Input As #1
row_number = 0
Do
Line Input #1, LineFromFile
LineItems = Split(LineFromFile, "|")

ActiveCell.Offset(row_number, 30).Value = LineItems(0)
ActiveCell.Offset(row_number, 29).Value = LineItems(1)
ActiveCell.Offset(row_number, 28).Value = LineItems(2)
ActiveCell.Offset(row_number, 27).Value = LineItems(3)
ActiveCell.Offset(row_number, 26).Value = LineItems(4)
ActiveCell.Offset(row_number, 25).Value = LineItems(5)
ActiveCell.Offset(row_number, 24).Value = LineItems(6)
ActiveCell.Offset(row_number, 23).Value = LineItems(7)
ActiveCell.Offset(row_number, 22).Value = LineItems(8)
ActiveCell.Offset(row_number, 21).Value = LineItems(9)
ActiveCell.Offset(row_number, 20).Value = LineItems(10)
ActiveCell.Offset(row_number, 19).Value = LineItems(11)
ActiveCell.Offset(row_number, 18).Value = LineItems(12)
ActiveCell.Offset(row_number, 17).Value = LineItems(13)
ActiveCell.Offset(row_number, 16).Value = LineItems(14)
ActiveCell.Offset(row_number, 15).Value = LineItems(15)
ActiveCell.Offset(row_number, 14).Value = LineItems(16)
ActiveCell.Offset(row_number, 13).Value = LineItems(17)
ActiveCell.Offset(row_number, 12).Value = LineItems(18)
ActiveCell.Offset(row_number, 11).Value = LineItems(19)
ActiveCell.Offset(row_number, 10).Value = LineItems(20)
ActiveCell.Offset(row_number, 9).Value = LineItems(21)
ActiveCell.Offset(row_number, 8).Value = LineItems(22)
ActiveCell.Offset(row_number, 7).Value = LineItems(23)
ActiveCell.Offset(row_number, 6).Value = LineItems(24)
ActiveCell.Offset(row_number, 5).Value = LineItems(25)
ActiveCell.Offset(row_number, 4).Value = LineItems(26)
ActiveCell.Offset(row_number, 3).Value = LineItems(27)
ActiveCell.Offset(row_number, 2).Value = LineItems(28)
ActiveCell.Offset(row_number, 1).Value = LineItems(29)
ActiveCell.Offset(row_number, 0).Value = LineItems(30)

row_number = row_number + 1
Loop Until EOF(1)

Close #1
End Sub

}

If I were to rewrite your code, I'd do it like this:

Option Explicit

Step one, always be explicit. Nobody likes seeing a variable being used that they never saw declared anywhere.

Public Sub ImportTextFile()

Step two, name the procedure after what it's doing, and make it do that.

    Dim filePath As String
    filePath = "filepath.txt"

    Dim fileNumber As Long
    fileNumber = FreeFile

FreeFile will give you a file number that's ok to use. Hard-coding a file number is never a good idea, even if you're only using one file.

    Dim inputData As String
    Dim lineCount As Long

    On Error Goto ErrHandler
    ToggleWaitMode True, "Importing text file..."

On Error Goto ErrHandler means if anything wrong happens (file not found, more data in the file than can fit on the worksheet, or whatever), we'll skip straight to an error-handling subroutine where we'll take good care of closing our file and toggle "wait mode" back off.

    Open filePath For Input As #fileNumber

        Do Until EOF(fileNumber)

            Line Input #fileNumber, inputData
            lineCount = lineCount + 1

            WriteLineContentToActiveSheet inputData, lineCount

        Loop

    Close #fileNumber

So the ImportTextFile procedure does nothing but reading the file's content. WriteLineContentToActiveSheet will do the worksheet-writing part.

ErrHandler:
    ToggleWaitMode

    If Err.Number <> 0 Then
        Close 'closes any file left open
        MsgBox Err.Description
        Err.Clear
    End If

End sub

The code under this label will execute even if no error occurred, this is why we're checking for Err.Number being other than 0. But regardless of whether there's an error or not, we always want to ToggleWaitMode.

Private Sub ToggleWaitMode(Optional ByVal wait As Boolean = False, Optional ByVal statusBarMessage As String = vbNullString)

    Application.ScreenUpdating = True 'ensure status message gets displayed
    Application.StatusBar = statusBarMessage
    Application.Cursor = IIf(wait, xlWait, xlDefault)
    Application.ScreenUpdating = Not wait
    Application.EnableEvents = Not Wait

End Sub

This small procedure takes two optional parameters. If none are specified, "wait mode" is toggled off - this means the status bar message is reset, the mouse cursor goes back to default, Excel resumes redrawing the screen and fires events whenever something happens. "Wait mode" is just all that reversed :)

Private Sub WriteLineContentToActiveSheet(ByVal inputData As String, ByVal targetRow As Long)
    Dim lineFields() As String
    lineFields = Split(inputData, "|")

    Dim fieldCount As Integer
    fieldCount = UBound(lineFields)

    Dim currentField As Integer
    Dim targetColumn As Integer
    Dim fieldValue As String

    With ActiveSheet
        For currentField = 0 To fieldCount

            fieldValue = lineFields(currentField)
            targetColumn = fieldCount - currentField + 1

            .Cells(targetRow, targetColumn).Value = fieldValue

        Next
    End With

End Sub

This small procedure takes a single line of data, splits it into as many fields as it has, and writes each field value into a cell of the active sheet. Using UBound we're no longer tied to a specific number of fields - if the file format changes, the procedure should still work. And if it doesn't, the procedure that called it will cleanly handle the error for us.

Local variables currentField, targetColumn and fieldValue are not necessary - they could all be inlined, but having them makes code easier to follow and allows you to place breakpoints to validate their values as you run the code line-by-line with F8 when you're debugging.

Couple observations:

You need to indent your code. Indentation makes it much easier to read, for yourself and anyone looking at it. Give that Tab button some lovin'!

Procedures define a scope, anything under it should be indented.
Within a scope, code blocks should also be indented - this includes If blocks, With blocks and any code that runs inside a loop.

OpenTextFile is a bad name for a procedure that actually imports a text file.
Be consistent with your naming: chose a convention, and stick to it.
You're reading a file, that's an I/O operation and by Murphy's Law this is going to fail one day or another. Make sure your code properly handles any error that could happen while processing the file, you don't want to leave a file handle opened by mistake.
Importing a 20mb text file will take a while, even with screen updating turned off. Tell your user you're working, give'em a hourglass cursor, and you can even update the statusbar message every couple thousand lines read/written (just call ToggleWaitMode True with a new status message) so they know it's not frozen.


Iterating through 128-byte records in a file I need to read records from a flat file, where each 128 bytes constitutes a logical record. The calling module of this below reader does just the following.

while(iterator.hasNext()){
    iterator.next();
    //do Something
 }

Means there will be a next() call after every hasNext() invocation.

Now here goes the reader.

public class FlatFileiteratorReader implements Iterable<String> {

    FileChannel fileChannel;

public FlatFileiteratorReader(FileInputStream fileInputStream) {
    fileChannel = fileInputStream.getChannel();
}

private class SampleFileIterator implements Iterator<String> {
    Charset charset = Charset.forName("ISO-8859-1");
    ByteBuffer byteBuffer = MappedByteBuffer.allocateDirect(128 * 100);
    LinkedList<String> recordCollection = new LinkedList<String>();
    String record = null;

    @Override
    public boolean hasNext() {
        if (!recordCollection.isEmpty()) {
            record = recordCollection.poll();
            return true;
        } else {
            try {
                int numberOfBytes = fileChannel.read(byteBuffer);
                if (numberOfBytes > 0) {
                    byteBuffer.rewind();
                    loadRecordsIntoCollection(charset.decode(byteBuffer)
                            .toString().substring(0, numberOfBytes),
                            numberOfBytes);
                    byteBuffer.flip();
                    record = recordCollection.poll();
                    return true;
                }
            } catch (IOException e) {
                // Report Exception. Real exception logging code in place
            }
        }
        try {
            fileChannel.close();
        } catch (IOException e) {
            // TODO Report Exception. Logging
        }
        return false;

    }

    @Override
    public String next() {
        return record;
    }

    @Override
    public void remove() {
        // NOT required

    }

    /**
     *
     * @param records
     * @param length
     */
    private void loadRecordsIntoCollection(String records, int length) {
        int numberOfRecords = length / 128;
        for (int i = 0; i < numberOfRecords; i++) {
            recordCollection.add(records.substring(i * 128, (i + 1) * 128));
        }
    }

}

    @Override
    public Iterator<String> iterator() {
        return new SampleFileIterator();
    }
}

The code reads 80 mb of data in 1.2 seconds on a machine with 7200 RPM HDD, with Sun JVM and running Windows XP OS. But I'm not that satisfied with the code I have written. Is there any other way to write this in a better way (especially the decoding to character set and taking only the bytes that has been read, I mean the charset.decode(byteBuffer) .toString().substring(0, numberOfBytes) part)?

There is no particular advantage to using a direct buffer here. You have to get the data across the JNI boundary into Java-land, so you may as well use a normal ByteBuffer. Direct buffers are for copying data when you don't want to look at it yourself really.
Use a ByteBuffer that is a multiple of 512, e.g. 8192, so you aren't driving the I/O system and disk controller mad with reads across sector boundaries. In this case I would think about using 128*512 to agree with your record length.
The .substring(0, numberOfBytes) is unnecessary. After the read and rewind, the ByteBuffer's position is zero and its limit equals numberOfBytes, so the charset.decode() operation is already delivering the correct amount of data.
You're assuming you didn't get a short read from FileChannel.read(). You can't assume that, there is nothing in the Javadoc to support that assumption. You need to read until the buffer is full or you get EOF.

Having said all that, I would also experiment with a BufferedReader around an InputStreamReader around the FileInputStream, and just read 128 chars at a time. You might get a surprise as to which is faster.


Authentication API script I have coded a script that I run when I need to verify the authentication for a user, I use it as a php script and host it on api.mydomain.com. It just simply checks to see if the user details are correct, the same as the parameters passed and to check if the account has a subscription.

The end result is checking if the response contains congratulations.

<?php
$connection = mysqli_connect(
    "localhost",
    "root",
    "",
    "mydb"
);

if (mysqli_connect_errno()) {
    exit('failed to connect to the database server.');
}

if (!isset($_GET['auth_username']) || !isset($_GET['auth_password'])) {
    exit('you failed to provide the correct parameters.');
}

$username = isset($_GET['auth_username']) ? $_GET['auth_username'] : '';
$password = isset($_GET['auth_password']) ? $_GET['auth_password'] : '';

$commandText = "SELECT `password`,`has_subscription` FROM `a__client_accounts`";
$whereClause = "WHERE `username` = '" . $username . "'";
$result = $connection->query($commandText . " " . $whereClause);

if (!$result) {
    exit('Invalid query: ' . $connection->error);
}
else if ($result->num_rows > 0) {
    $array =  $result->fetch_array();

    if (!password_verify($password, $array['password'])) {
        exit('we found your account, but you entered the wrong password.');
    }
    else if ($array['has_subscription'] == '0') {
        exit('you authenticated, but you don\'t have a subscription');
    }
    else {
        exit('congratulations');
    }
}
else {
    echo 'no account was found with that name.';
}

I would make my review mostly on the security, with some other improvements.

The main issue with this code, obviously, is SQL injection. It can easily allow anyone to browse the whole contents of your database. You should be really using prepared statements for the every SQL query you run, if at least one variable is going to be used in it.
Worse yet, by echoing the error out, you ease the task for the attacker significantly. By getting the feedback for their actions through the error message, an attacker could save themselves a lot of time. In case of error, only a generalized error message should be ever sent outside.
a somewhat similar issue: never give a potential hacker any certain feedback regarding the correctness of login and password - it's none of their business. Whereas a honest user could be just informed that their credentials didn't match. So it should be the only response in case of the wrong credentials.
Another issue is HTTP method used. It is considered a good practice to use POST method for the credentials, as it has a much lower footprint. For example, when using GET method, you will have all user passwords stored in the server's access log in the raw form. Why bother with hashing them in the database, if raw passwords can be easily accessed anyway?

I also removed redundant isset() calls. Given you already tested your input variables with isset, a condition $username = isset($_GET['auth_username']) will never be evaluated to false. So I just removed it altogether.

A note on the error reporting. Like it is said above, a system error message should be never echoed outside. At the same time, it should be logged carefully for the site admin's reference. Seemingly contradicting at first, there rules are  perfectly go together. I wrote an article on the PHP error reporting in which I summarized all my experience and which I highly recommend to read.

So the code would be

<?php

error_reporting(E_ALL);
ini_set('display_errors', 0);
ini_set('log_errors', 1);
mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);

try {
    $connection = mysqli_connect("localhost","root","","mydb");

    if (!isset($_POST['auth_username'],$_POST['auth_password'])) {
        exit('you failed to provide the correct parameters.');
    }

    $sql = "SELECT password, has_subscription FROM a__client_accounts WHERE username=?";
    $stmt = $connection->prepare($sql);
    $stmt->bind_param("s", $_POST['auth_username'])
    $stmt->execute();
    $stmt->store_result();
    $stmt->bind_result($db_password, $has_subscription);

    if ($stmt->fetch() && password_verify($_POST['auth_password'], $db_password)
    {
        if (!$has_subscription)
        {
            exit('you authenticated, but you don\'t have a subscription');
        } else {
            exit('congratulations');
        }
    } else {
        echo 'wrong login or password';
    }
} catch (\Exception $e) {
    error_log($e);
    exit("server error");
}


Simple C++ allocator for POD types Edit 1 after first review: I changed to use malloc / free since apparently it's also easier to optimize than new[] / delete[] : https://godbolt.org/g/CZy28o ; of course this adds an additional requirement on the alignment.

template <class T>
struct pod_allocator
{
    static_assert(std::is_pod_v<T>, "can only be used with POD types");
    static_assert(alignof(T) <= alignof(std::max_align_t), "type must not have specific alignment requirements");
    using value_type = T;
    auto allocate(std::size_t num)  { return (T*) malloc(sizeof(T) * num); }
    void deallocate(T* p, std::size_t)   { free(p); }
};

The incentive here is to allow some compilers (clang as of clang-6.0 for instance) to remove allocations in some cases, by observing that they are able to optimize out new/delete pairs but not ::operator new/::operator delete pairs : see the following in https://godbolt.org/g/jumtCU

#include <algorithm>
#include <numeric>
#include <cinttypes>
int foo(int* src)
{
   constexpr int n = 10;
   auto x = new int[n];
   for(int i = 0; i < n; i++)
   {
       if(i%2 == 0)
         x[i] = src[n - i - 1] * 2;
       else
         x[i] = src[i] * src[i];
   }
   int z = 0;
   if(x[1] < 1) z = 10;
   else z = x[2] + x[4] - x[1] - x[3];

   delete[] x;

   return z;
}

While the vector version... well... produces "suboptimal" assembly: https://godbolt.org/g/cZvSRb .

#include <algorithm>
#include <numeric>
#include <cinttypes>
#include <vector>
int foo(int* src)
{
   constexpr int n = 10;
   std::vector<int> x(n);
   for(int i = 0; i < n; i++)
   {
       if(i%2 == 0)
         x[i] = src[n - i - 1] * 2;
       else
         x[i] = src[i] * src[i];
   }
   int z = 0;
   if(x[1] < 1) z = 10;
   else z = x[2] + x[4] - x[1] - x[3];

   return z;
}

Hence, here is the code of the allocator, which allows us to claim back much better codegen (https://godbolt.org/g/hvPY14) :

template <class T>
class pod_allocator
{
  public:
    static_assert(std::is_pod_v<T>, "can only be used with POD types");
    using value_type = T;
    using pointer = T*;
    using const_pointer = const T*;
    using reference = T&;
    using const_reference = const T&;
    using size_type = std::size_t;
    using difference_type = std::ptrdiff_t;
    using is_always_equal = std::true_type;

    constexpr T* address(T& value) const noexcept { return &value; }
    constexpr const T* address(const T& value) const noexcept { return &value; }

    constexpr auto max_size() const noexcept {
        return std::numeric_limits<std::size_t>::max() / sizeof(T);
    }

    auto allocate(std::size_t num) { return new T[num]; }
    void deallocate(T* p, std::size_t) { delete[] p; }

    constexpr void construct(T*, const T&) noexcept { }
    constexpr void destroy(T*) noexcept { }
};

template<class T1, class T2>
constexpr bool operator==(const pod_allocator<T1>&, const pod_allocator<T2>&) noexcept { return true; }

template<class T1, class T2>
constexpr bool operator!=(const pod_allocator<T1>&, const pod_allocator<T2>&) noexcept { return false; }

template<typename T>
using pod_vector = std::vector<T, pod_allocator<T>>;

Are there some subtle bugs that I could be introducing this way ? (not counting of course the lack of value-initialization, which is, in this case, a feature and not a bug).

If you're going to roll your own allocator in C++17, you can save yourself a lot of grief by just leaving everything to default. The only things you need to include are value_type, allocate(), deallocate(), and operators == and !=.

In fact, I believe by not using the defaults, you've introduced a nasty little bug. Try doing this: auto v = pod_vector<int>(5, 42);. This should create a vector with five ints, all set to 42. But without construct() properly defined, who knows what will happen?

My advice:

Your allocator should be 4 lines.

the static_assert
value_type
allocate()
deallocate()

(And you need the == and != comparisons too.)

At the very least, either remove construct() or give it a sensible implementation (i.e., placement new). With destroy() it's less of an issue because you're only dealing with PODs.

AFTER EDIT 1: I'd wager the reason you're seeing better code generation for malloc() than new is that new throws std::bad_alloc automatically when it can't allocate.

I suspect if you changed your allocate() function to:

auto allocate(std::size_t num)  {
    auto const p = (T*) malloc(sizeof(T) * num);
    if (!p)
        throw std::bad_alloc{};
    return p;
}

you'd see a very different code gen situation.

As it stands, if you get an allocation failure, you're just returning a null pointer... which will lead to undefined behaviour, because allocate() must return a pointer to allocated memory.


Could I dispose IDisposable objects that are part of a Thread? I have a question about using an IDisposable object in the ThreadStart function. I have a class that derives from ServiceBase:

public class ServiceTest: System.ServiceProcess.ServiceBase
{
    private System.ComponentModel.Container components = null;
    private System.Threading.Thread         m_cProcessThread;

    public ServiceTest()
    {
        this.ServiceName = "ServiceTest";
    }

    static void Main()
    {
        System.ServiceProcess.ServiceBase[] ServicesToRun  = new System.ServiceProcess.ServiceBase[] { new ServiceTest() };

        System.ServiceProcess.ServiceBase.Run(ServicesToRun);
    }
    protected override void Dispose( bool disposing )
    {
        if ( disposing )
        {
            if (components != null)
            {
                components.Dispose();
            }
        }
        base.Dispose( disposing );
    }

    protected override void OnStart(string[] args)
    {
        ProcessThread   cThread;

        cThread = new ProcessThread();
        m_cProcessThread = new Thread(new ThreadStart(cThread.InitProcess));
        m_cProcessThread.Start();
    }

    protected override void OnStop()
    {
        m_cProcessThread.Abort();
    }
}

and in the OnStart function I have the ProcessThread object which is an IDisposable object. I was wondering if it is safe to re-write the OnStart function to do this:

    protected override void OnStart(string[] args)
    {
        using (Process cThread = new ProcessThread())
        {
            m_cProcessThread = new Thread(new ThreadStart(cThread.InitProcess));
        }

        m_cProcessThread.Start();
    }

Or will there be any consequences in doing this? I have a bit of an OCD when I see IDisposable objects in C# code that makes me believe that they HAVE to be disposed as soon as you are done lol. Also I don't know if my question title really fits with what I'm asking so if you could come up with a better title then feel free to edit.

Code is targeted at .net 2.0, running on a Windows 2000 server.

Strictly speaking, it depends on what InitProcess does.  It may very well work OK as long as it never touches resources in the ProcessThread class which get disposed.

However, even if it works this time, it is a bad idea.  Passing a method group from a class which gets disposed before the thread even starts is asking for trouble.

A better option would be to create a method inside your ServiceTest class that the thread executes.  Inside that thread, you can instantiate a ProcessThread object, call InitProcess, and ensure that Dispose is only called once InitProcess has completed:

protected override void OnStart(string[] args)
{
    m_cProcessThread = new Thread(InitProcess);
    m_cProcessThread.Start();
}

private void InitProcess()
{
    using(var processThread = new ProcessThread())
    {
        processThread.InitProcess();
    }
}

You may also note that I removed the explicit construction of a ThreadStart delegate.  In many cases, you can pass a method group directly where delegates are called for.  I find it a little cleaner to do so.


Selecting an enum for a payment transaction I'm writing a small Android application that deals with payments and, in my function that handles transactions, I need to translate a number of installments (1-12) and whether or not it has interest (0-1) into the de facto objects for those of the SDK I'm using.

Right now I have a switch that looks like the following:

InstalmentTransactionEnum install = InstalmentTransactionEnum.ONE_INSTALMENT;
//TODO: Implement interest
switch (installments) {
    case "1":
        install = InstalmentTransactionEnum.ONE_INSTALMENT;
        break;
    case "2":
        install = InstalmentTransactionEnum.TWO_INSTALMENT_NO_INTEREST;
        break;
    case "3":
        install = InstalmentTransactionEnum.THREE_INSTALMENT_NO_INTEREST;
        break;
    case "4":
        install = InstalmentTransactionEnum.FOUR_INSTALMENT_NO_INTEREST;
        break;
    case "5":
        install = InstalmentTransactionEnum.FIVE_INSTALMENT_NO_INTEREST;
        break;
    case "6":
        install = InstalmentTransactionEnum.SIX_INSTALMENT_NO_INTEREST;
        break;
    case "7":
        install = InstalmentTransactionEnum.SEVEN_INSTALMENT_NO_INTEREST;
        break;
    case "8":
        install = InstalmentTransactionEnum.EIGHT_INSTALMENT_NO_INTEREST;
        break;
    case "9":
        install = InstalmentTransactionEnum.NINE_INSTALMENT_NO_INTEREST;
        break;
    case "10":
        install = InstalmentTransactionEnum.TEN_INSTALMENT_NO_INTEREST;
        break;
    case "11":
        install = InstalmentTransactionEnum.ELEVEN_INSTALMENT_NO_INTEREST;
        break;
    case "12":
        install = InstalmentTransactionEnum.TWELVE_INSTALMENT_NO_INTEREST;
        break;
}
stoneTransaction.setInstalmentTransactionEnum(install);

Now this is without me having implemented interest which in this method would require another massive switch statement where install would be equal to TWO_INSTALLMENT_WITH_INTEREST and so on.

My thinking is that there must be a smarter way of doing this that doesn't involve an if statement to check the interest variable (a boolean) and the massive switch for installments. What is the best way of achieving this?

You're using the wrong tool for the job.

If there's ever going to be a loan that I will pay an amount to every week for 2 years, you're gonna have to write a script to generate the code for you, because doing it by hand would suck. You'd have to write code for 204 different enum values, including the switch cases.

If you had to map months to strings, you could have given your enums values, e.g.

enum Month {
    JANUARY(1, "January", "JAN"),
    FEBRUARI(2, "February", "FEB"),
    ...

    private int number;
    private String name;
    private String shorthandName;
    Month(int month, String fullName, String shortName){
        number = month;
        name = fullName;
        shorthandName = shortName;
    }
    //add getters....
}

Like that.

But what you have here is more like an object. You have two values to keep track of, and they are not related. This doesn't lend itself to an enumeration as you'll need to create each combination by hand.

For that, I recommend using value objects. An example of a value object is a String. It's an immutable object of which you cannot change the value.

So consider making a class like so:

class InstallmentTransaction {
    private final int installments;
    private final boolean interest;

    private static final Map<Integer, Map<Boolean, InstallmentTransaction>> instances = new HashMap<>();
    private static final Object lockObject = new Object();

    private InstallmentTransaction(int installments, boolean interest)
    {
        this.installments = installments;
        this.interest = interest;
    }

    public static InstallmentTransaction getInstance(int installments, boolean interest){
        synchronized(lockObject){
            Map<Boolean, InstallmentTransaction> map = instances.get(installments);
            if(map == null){
                map = new HashMap<>();
                instances.put(installments, map);
            }
            InstallmentTransaction instance = map.get(interest);
            if(instance == null){
                instance = new InstallmentTransaction(installments, interest);
                map.put(interest, instance);
            }
            return instance;
        }
    }

    //getters go here
}

I didn't implement the full thing, that's something you'll have to do, but this will give you the benefit of object equality combined with the ability to reference instances (getInstance(12, false)).

I also opted for Integer here, because that's the format you store numbers in. Even if the API only exposes Strings, that doesn't mean you are stuck using Strings. ""+int will give you a String, and Integer.parseInt("") will give you an integer.

synchronized is used to prevent there being two threads both calling getInstance at the same time and creating two objects for the same values.

The upsides of this approach are that it automatically supports all values of installments and interest.

The downside is that if you have a lot of values, you risk filling up your memory with all of them. You don't have this, though, as right now it's going to be 24 values (12 months * 2). For Android development, it might not even matter - apps don't have the same active duration as server applications do.


Pig Latin Anslatortray I've created a program that translates any word into the form of igpay atinlay or pig Latin!

I got the project idea from here and you can read the rules here.

My program outputs the same translation as in the rules section in the link above, except for the word "always". It isn't really a big deal as the program translates almost any word correctly.

package pigLatin;

import java.util.Scanner;

public class PigLatin
{
 String str;
 Scanner scan = new Scanner(System.in);
 char firstVow;
 char secondVow;
 int i;

 public void init()

 {
  str = scan.next();
  i = 0; //beginning of a string

  if (str.length() <= 3 && (str.charAt(i) == 'a' || str.charAt(i) == 'e' || str.charAt(i) == 'i' || str.charAt(i) == 'o' || str.charAt(i) == 'u'))
  {
   System.out.println(str + "ay");
   scan.close();
  }

  else if (str.length() > 3 && (str.charAt(i) == 'a' || str.charAt(i) == 'e' || str.charAt(i) == 'i' || str.charAt(i) == 'o' || str.charAt(i) == 'u')) {
   firstVow = str.charAt(i);
   i = 1;

   while (str.charAt(i) != 'a' || str.charAt(i) != 'e' || str.charAt(i) != 'i' || str.charAt(i) != 'o' || str.charAt(i) != 'u')

   {
    i++;
    if (i == str.length() - 1 && (str.charAt(i) != 'a' || str.charAt(i) != 'e' || str.charAt(i) != 'i' || str.charAt(i) != 'o' || str.charAt(i) != 'u')) {
     System.out.println(str + "ay");
     break;

    } else if (str.charAt(i) == 'a' || str.charAt(i) == 'e' || str.charAt(i) == 'i' || str.charAt(i) == 'o' || str.charAt(i) == 'u')

    {
     secondVow = str.charAt(i);

     //issues arise if the first vowel is equal to the second one
     if (firstVow != secondVow) {

      System.out.print(str.substring(str.indexOf(secondVow), str.length()));
      System.out.print(str.substring(str.indexOf(firstVow), str.indexOf(secondVow)));
      System.out.println("ay");
      break;

     } else if (firstVow == secondVow) {
      System.out.print(str.substring(2));
      System.out.print(firstVow);
      System.out.print(str.charAt(1));
      System.out.print("ay");
      break;
     }
     scan.close();
    }
   }

   //checks for words that begin with consonants
  } else {

   i = 0;
   while (str.charAt(i) != 'a' || str.charAt(i) != 'e' || str.charAt(i) != 'i' || str.charAt(i) != 'o' || str.charAt(i) != 'u')

   {
    i++;
    if (str.charAt(i) == 'a' || str.charAt(i) == 'e' || str.charAt(i) == 'i' || str.charAt(i) == 'o' || str.charAt(i) == 'u')

    {
     firstVow = str.charAt(i);
     System.out.print(str.substring(str.indexOf(firstVow), str.length()) + str.substring(0, str.indexOf(firstVow)) + "ay");
     break;
    }
   }
  }
  scan.close();

 }

 public static void main(String[] args) {
  new PigLatin().init();
 }

}

Your leading whitespace is much smaller than is typical. Prefer 4 spaces for a level of indent.
In java we typically put the curly brace { on the same line. If you insist on doing it on a new line, do it consistently (main) and don't add a blank line of whitespace before the curly brace.
Variable names need to be clear and descriptive. str is meaningless - perhaps word? Likewise, firstVow is confusing, since nobody's getting married. Surely you mean firstVowel. Extra characters come at no charge, so if they make the variable name more descriptive, use them.
Closeable resources need to be closed always. Use either try-with-resources or a finally block to close open resources. Otherwise an exception in the middle of execution will prevent the resource from being closed.
Variables should be defined in the most constraining scope possible. All of your variables can and should be method variables, defined at the time of first need.
You repeat code to see if a character is a vowel. Using either a method so you're only writing the code once or a Set to hold all vowels would be better.
for loops would be easier to read than while loops, since you need all three statement anyway.
Your last block of work (word starts with a consonant) assumes there'll eventually be a vowel in the word.
It's generally preferable that methods return a value rather than performing direct IO themselves. If you don't follow that, the pattern you used for the first two blocks (multiple System.out.print calls) is much easier to read than the one big print with everything mushed together. If you do return a value, using a StringBuilder might be a better idea than using string concatenation.
In several places you store a character and use to that with an indexOf when the index is just i or 0. You should simplify that down.
In a couple of places you do if (condition) { } else if (!condition) { }. It's OK to just do if (condition) { } else { }


Handling potentially missing props of an object I'm building a wrapper for the Stackoverflow Careers RSS feeds for a NodeJS application and I've hit an interesting point.

The following code works fine and produces a valid result but I feel like I can handle it better.

Here's my Utils.js file:

let utils = {
    UrlPrefix: 'https://stackoverflow.com/jobs/feed?q=',
    Location: "&l=",
    Unit: "&u=",
    TechLiked: "&tl=",
    TechDisliked: "&td="
}

module.exports = utils;

And the function that I'm getting cold sweats from:

function UrlBuilder(props) {

    let builtUrl = utils.UrlPrefix;

    if(props.query) {
        builtUrl += props.query
    }

    if(props.location) {
        builtUrl += utils.Location + props.location
    }

    if(props.td) {
        builtUrl += utils.TechDisliked + props.td
    }

    if(props.tl) {
        builtUrl += utils.TechLiked + props.tl
    }

    if(props.unit) {
        builtUrl += utils.Unit + props.unit
    }

    return builtUrl;
}

Is there no way around this or can I handle this better? All of the properties are optional so any one of them could be absent.

It depends on what you want to make better. Personally, I find the code readable as you have it, however, depending on how many url options might still come, I guess you can make it yourself a bit easier.

A first start I would do, would be to split the query from the url itself, maybe you will not always add a query, so, I would change the utils class to:

let utils = {
    url: 'https://stackoverflow.com/jobs/feed',
    query: 'q=',
    location: "&l=",
    unit: "&u=",
    techLiked: "&tl=",
    techDisliked: "&td="
};

Furthermore, I also camelCased the properties of the utils object, I don't see no reason why they should be PascalCased. But I guess that could depend on some code guidelines you are following.

The same thing goes for the UrlBuilder. I would expect that PascalCased functions are classes, but since you return a string at the end of the function, it would not be the case. So you should also use camelCasing for simple functions. Together with that change, you could also introduce default values for your props parameter, so you wouldn't have to do checks for undefined/null values, like so:

/**
 * @method urlBuilder
 * @param {{query: string=, location: string=, td: string=, tl: string=, unit: string=}} props
 * @returns Full url needed to query rss feed
 */
function urlBuilder(props = {}) { ... }

If you expect a certain type of input parameters, it never hurts to document what does might be. It would help you further in the future when maintaining this code.

Now, the biggest difference I could imagine making is inside the urlBuilder function. One major change you could make is to choose for a more obvious name, urlBuilder itself sounds a bit like a wizard function to which you could add more options in a fluent language style. Maybe you could change it to getSpecificUrlFor(props = {}) which would indicate that you would receive a url based on the properties itself.

Now, inside the function, I added a small map that maps the expected properties of your argument to the properties in your utils object.

const map = {
  query: 'query',
  location: 'location',
  unit: 'unit',
  tl: 'techLiked',
  td: 'techDisliked'
};

The property of the map will indicate the expected property for your props object, and the value would link to the property on your utils object.

The biggest advantage you have here is that you can expand or reduce the parameters like you want to, if you would implement the check in the following way:

let result = [];
// iterate the full props object by it's key
for (const key of Object.keys( props )) {
  if (!map[key]) {
    // no matching prop found
    continue;
  }
  // found a key, so add it
  result.push(utils[map[key]]);
  result.push(props[key]);
}

Only if a property is defined on the map, it will add the utils value to your result set, together with the value of the props.

As the query is no longer part of the UrlPrefix property, you could now either return the url with potentially found parameters, or just return the default url, like so:

if (!result.length) {
  // no matching props
  return utils.url;
}
// return the url
return utils.url + '?' + result.join('');

There is no overly pragmatic reason for choosing the array.Join method, but rather a habit from using StringBuilder in other languages, so concatenating your strings as in your example is perfectly fine as well.

In the end, the full change looks like this, I added some example outputs as well so you could validate if it is now working as you expect it to work.

let utils = {
    url: 'https://stackoverflow.com/jobs/feed',
    query: 'q=',
    location: "&l=",
    unit: "&u=",
    techLiked: "&tl=",
    techDisliked: "&td="
};

/**
 * @method getSpecificUrlFor
 * @param {{query: string=, location: string=, td: string=, tl: string=, unit: string=}} props
 * @returns Full url needed to query rss feed
 */
function getSpecificUrlFor(props = {}) {
  const map = {
    query: 'query',
    location: 'location',
    unit: 'unit',
    tl: 'techLiked',
    td: 'techDisliked'
  };
  let result = [];
  // iterate the full props object by it's key
  for (const key of Object.keys( props )) {
    if (!map[key]) {
      // no matching prop found
      continue;
    }
    // found a key, so add it
    result.push(utils[map[key]]);
    result.push(props[key]);
  }
  if (!result.length) {
    // no matching props
    return utils.url;
  }
  // return the url
  return utils.url + '?' + result.join('');
}

// some valid inputs
console.log( getSpecificUrlFor({query: 'javascript'}) );
console.log( getSpecificUrlFor({query: 'javascript', tl: 'react'}) );
console.log( getSpecificUrlFor() ); // props would be at least an empty object, so expect the urlPrefix

// overwriting props that are not in the map doesn't work
console.log( getSpecificUrlFor({query: 'javascript', url: 'http://www.google.com'}) );

// invalid input -> gets ignored
console.log( getSpecificUrlFor(5) );
console.log( getSpecificUrlFor('a') );

One more thing you could still add, is to make sure that your parameters are properly encoded, you could do this with encodeURIComponent or something similar for nodejs


PHP/MySQL to delete data from 4 different tables at once I have four mysql tables setup like the following:

users (userid, firstname, lastname)  *one entry per user
schedules (userid, scheduleid, schedulename) *each user could have multiple schedules
classes (classid, scheduleid, classname) *each schedule is made up of multiple classes
schools (scheduleid, schoolid, schoolname) *one entry per user

I want the user to be able to delete their account, including ANY info from other tables tied to their account. So if they had 3 schedules with a bunch of classes in each and a school tied to each schedule, I'd want ALL of that data (including their user info) to be deleted in one fell swoop.

userid is all I need to delete from the users and schedules table easily.  But the classes and schools tables DON'T have a userid field, so I have to do some other way.

I wrote and tested the following code and it works fine...

//first get all schedules associated with this user account and put scheduleids into array
$sql = "SELECT scheduleid FROM schedules
        WHERE userid = '$userid'";
        $selectschedules = mysqli_query($connection, $sql);
        if (!$selectschedules) {
        die("Database query failed: " . mysqli_error());
        } else {
            while($data=mysqli_fetch_assoc($selectschedules)){
                 $schedules[] = $data;
            }
        }
//now loop through each schedule to delete all classes and any school associated with that particular schedule
foreach ($schedules as $item) {
    $sql = "DELETE FROM classes
            WHERE scheduleid = '".$item['scheduleid']."'";
            $deleteclasses = mysqli_query($connection, $sql);
            if (!$deleteclasses) {
            die("Database query failed: " . mysqli_error());
            } else {
            }
    $sql = "DELETE FROM schools
            WHERE scheduleid = '".$item['scheduleid']."'";
            $deleteschool = mysqli_query($connection, $sql);
            if (!$deleteschool) {
            die("Database query failed: " . mysqli_error());
            } else {
            }
}
//next delete all schedules associated with user account
$sql = "DELETE FROM schedules
        WHERE userid = '$userid'";
        $deleteschedules = mysqli_query($connection, $sql);
        if (!$deleteschedules) {
        die("Database query failed: " . mysqli_error());
        } else {
        }
//finally, delete the user's account
$sql = "DELETE FROM users
        WHERE userid = '$userid'
        LIMIT 1"; //should only be one, just being safe
        $deleteuser = mysqli_query($connection, $sql);
        if (!$deleteuser) {
        die("Database query failed: " . mysqli_error());
        } else {
        }

However, I believe the above code is inefficient (particularly looping through each schedule and doing delete queries on every loop). So I'm curious if there is a shorter/more efficient way to accomplish this same thing.  I am not good with JOINS and could be wrong, but would using JOINS potentially be the key to shortening this and NOT having to loop at all?

SIDE NOTE: Earlier I mentioned that classes and schools tables DON'T have a userid field.  If they did, it would be very easy to delete all of this stuff in one query. So it begs the question:  Should I add the userid field to the other tables?  I don't want to add it just to make deleting easier, but if you think it SHOULD have it regardless, then I would consider.

Any info/suggestions would be greatly appreciated.

Basically there are three vectors of improvement:

of course you can make your queries optimal, avoiding a loop
also, I would suggest to optimize the the process of executing the query, as it is evident that each query takes too much code to write
you should never put a single variable directly in the query, but always use a prepared statement instead.

To solve the first problem, you can always use a JOIN. Yes, DELETE queries support JOINs as well as SELECT ones. So it goes:

DELETE c.* FROM classes c, schedules s WHERE c.scheduleid=s.id AND s.userid = ?

To solve the second one would propose to use PDO (which can solve the third one as well) as it lets you to run a query in one line:

$pdo->prepare("DELETE FROM users WHERE userid = ?")->execute([$userid]);

So the overall code would be

$sql = "DELETE c.* FROM classes c, schedules s WHERE c.scheduleid=s.scheduleid AND s.userid = ?";
$pdo->prepare($sql)->execute([$userid]);

$sql = "DELETE c.* FROM schools sc, schedules s WHERE sc.scheduleid=s.scheduleid AND s.userid = ?";
$pdo->prepare($sql)->execute([$userid]);

$sql = "DELETE FROM schedules WHERE s.userid = ?";
$pdo->prepare($sql)->execute([$userid]);

$sql = "DELETE FROM users WHERE s.userid = ?";
$pdo->prepare($sql)->execute([$userid]);

As long as you are connecting to PDO properly, this code will report errors automatically, without a single extra line of code!

  Should I add the userid field to the other tables?

Quite contrary, you should NOT add such a field, keeping your database structure as normalized as possible


Crawler for scraping and approving friend requests in Facebook I've made a Facebook scraper using VBA in combination with selenium which is able to parse the name from friend request list.

It is necessary to do one thing manually, though. After running this script, when Facebook page opens up, there will be a notification popping up mentioning "Allow" or "Block". Wait a little while to get the buffering done and then Just click on "Allow". That's it.

There is a javascript function within this script to handle scrolling the webpage.

Here is the working code which I have tried to make accurately:

Sub Facebook_Crawler()

Dim driver As New WebDriver
Dim posts As Object, post As Object

With driver
    .Start "chrome", "https://www.facebook.com"
    .get "/login.php?login_attempt=1&lwv=111"

    .FindElementByXPath("//*[@id='email']").SendKeys ("someEmail") --Fill in the email
    .FindElementByXPath("//*[@id='pass']").SendKeys ("password") --Fill in the password
    .FindElementByXPath("//*[@id='loginbutton']").Click
    .Wait 800
    .FindElementByXPath("//a[@id='findFriendsNav']").Click
    .Wait 500
End With

For x = 0 To 50
    driver.ExecuteScript "window.scrollTo(0, document.body.scrollHeight);"
    driver.Wait 500
Next x

Set posts = driver.FindElementsByXPath("//div[contains(@class,'friendBrowserNameTitle')]/a")

For Each post In posts
    i = i + 1
    Cells(i, 1) = post.Text
Next post

End Sub

VBAs selenium bindings are not as rich in functionality as other language bindings, but I would improve the following:

use the "by id" locators whenever possible:

.FindElementById("email").SendKeys ("someEmail") --Fill in the email
.FindElementById("pass").SendKeys ("password") --Fill in the password
.FindElementById("loginbutton").Click

if tempting to use XPaths, give CSS selectors a chance:

Set posts = driver.FindElementsByCss(".friendBrowserNameTitle > a")

avoid "magic" numbers in the code, like that value 50 - the number of scroll actions - configure it as a constant
I also don't particularly like the .Wait calls - this is typically a red flag when it comes to browser automation with selenium, but I am not sure VB bindings provide an Explicit Wait with a pre-defined condition


TaskScheduler in python I have found the need to schedule a few simple functions in a first-come-first-served way so I developed a small task scheduler based on multiprocessing and I thought it would be a good candidate for my first question here.

I'm open to all kind of comments, regarding style, conventions, possible bugs and other things I'm not considering now. I added some comments for the most sensible mechanism but this is mostly a half-day code.

from multiprocessing import Process
from multiprocessing import JoinableQueue

def worker_process(in_queue, out_queue):
    run = True
    while run:
        # in the queue there should be a Job or None.
        # Latter case is the signal that we should stop.
        task = in_queue.get()
        if task:
            task.run()
            in_queue.task_done()
            # when done, we signal and we put the task id in the output queue
            out_queue.put(task.get_id())
        else:
            run = False

class Scheduler:
    def __init__(self, job_done_cb, cb_args=[], N=2):
        self.n = N
        self.processes = []
        self.job_queue = JoinableQueue()
        self.done_queue = JoinableQueue()
        self.job_done_cb = job_done_cb
        self.cb_args = cb_args
        for i in range(self.n):
            # these workers will get jobs from the queue and give us back the id when done
            p = Process(target=worker_process, args=(self.job_queue, self.done_queue))
            self.processes.append(p)
        # there process will get back the job of the id done and execute the callback
        self.cb_process = Process(target=self.on_job_done)

    def start(self):
        for p in self.processes:
            p.start()
        self.cb_process.start()

    def stop(self):
        # add the necessary None to the queue and wait for the processes to be done
        # this will make the queue execute the remaining task before finishing
        for i in range(self.n):
            self.job_queue.put(None)
        self.job_queue.close()
        for p in self.processes:
            p.join()
        self.done_queue.put(None)
        self.cb_process.join()

    def add_task(self, task_id, task, arg_list):
        job = Job(task, task_id, arg_list)
        self.job_queue.put(job)

    def wait_for_idle(self):
        self.job_queue.join()

    def on_job_done(self):
        run = True
        while run:
            # again, if the id is not None then we should go on waiting for stuff
            done = self.done_queue.get()
            if done:
                self.job_done_cb(self, done, *self.cb_args)
            else:
                run = False

class Job:
    def __init__(self, task, job_id, arg_list):
        self.task = task
        self.id = job_id
        self.arg_list = arg_list

    def run(self):
        self.task(*self.arg_list)

    def get_id(self):
        return self.id

Unused Loop Variables

for i in range(self.n):
    self.job_queue.put(None)

can be written like this

for _ in range(self.n):
    self.job_queue.put(None)

and any place where you don't use the loop variable. The _ lets you and other programmers know that that variable isn't used and should be ignored.

Naming

p should be process to be descriptive. While p in self.processes can be obvious, it's good to get in the practice of descriptive variable names.

Type Hints

This

def add_task(self, task_id, task, arg_list):

can instead be (assuming task is a str and arg_list is a list)

def add_task(self, task_id: int, task: str, arg_list: list) -> None:

Using type hints allows you to see/show what types of parameters are accepted, and what type(s) is/are returned from the function.


Checking a DB table before doing an insert using AWS Lambda Pretty new to AWS Lambda and Javascript, and particularly Promises.

Simple check db table before insert.

It all seems to be working - I'm just wondering if there was a neater way of doing any of this that I'm not aware of.

import AWS from 'aws-sdk';
const dynamo = new AWS.DynamoDB.DocumentClient({ region: 'eu-west-1'})

export function main(event, context, callback) {

  dynamo.get({ TableName: 'client', Key: { id: event.name } })
    .promise()
    .then(function(data) {

      if(data.Item){
        callback(null, `Client name '${event.name}' is already in use.`)
        return
      }

      const timestamp = new Date().getTime()

      dynamo.put({
        TableName: 'client',
        Item: {
          id: event.name,
          created: timestamp,
          updated: timestamp,
          deleted: null
        },
        ConditionExpression: 'attribute_not_exists(id)'
      })
      .promise()
      .then(function(data) {
        callback(null, event)
      }).catch(function(err) {
        callback(err)
      })

  }).catch(function(err) {
     callback(err)
  })

}

Your code is already pretty good by common standards, IMO.

If it is possible, you can try to adopt ES7's async/await. The code will become more linear, while still asynchronous.

Watch out for browser compatibility though.

export async function main(event) {

  const data = await dynamo
    .get({ TableName: 'client', Key: { id: event.name } })
    .promise();

  if (data.Item) {
    throw new Error(`Client name '${event.name}' is already in use.`);
  }

  const timestamp = new Date().getTime();

  return await dynamo
    .put({
      TableName: 'client',
      Item: {
        id: event.name,
        created: timestamp,
        updated: timestamp,
        deleted: null
      },
      ConditionExpression: 'attribute_not_exists(id)'
    })
    .promise();

}


Unit-testing friendly singleton In my project we have a few singletons, which tends to be problematic in unit tests. So I wanted to find solution for the problem. Here is what I came with so far:

smart_singleton.h

class smart_singleton
{
public:
    static std::shared_ptr<smart_singleton> get_instance();

private:
    smart_singleton();

    smart_singleton(const smart_singleton&) = delete;
    smart_singleton operator=(const smart_singleton&) = delete;
    smart_singleton(smart_singleton&&) = default;
    smart_singleton& operator=(smart_singleton&&) = default;

    static std::weak_ptr<smart_singleton> weak_instance;
};

smart_singleton.cpp

std::weak_ptr<smart_singleton> smart_singleton::weak_instance;

std::shared_ptr<smart_singleton> smart_singleton::get_instance()
{
    if (auto existing_instance = weak_instance.lock()) {
        return existing_instance;
    } else {
        std::shared_ptr<smart_singleton> tmp_shared(new smart_singleton());
        weak_instance = tmp_shared;
        return tmp_shared;
    }
}

smart_singleton::smart_singleton()
{

}

The difference is that you need to hold one shared_ptr from "get_instance()" anywhere in your code for the object to not be destroyed. In your prodution code that would be somewhere in the main function (or some object that is alive for the whole scope of main). In UT that would be for the duration of one test.

IMPORTANT:
Please don't focus on the singleton pattern itself being discouraged. My project has singletons and changing that would mean a lot of effort. My attempt is only to make unit testing easier. What I'm interested in, therefore, are the differences between having a classic implementation of singleton in C++ and my implementation and what possible problems could the latter one cause.

I like the idea.

First off, it appears that the get_instance() method isn't thread-safe:

std::shared_ptr<smart_singleton> smart_singleton::get_instance()
{
    if (auto existing_instance = weak_instance.lock()) {
        return existing_instance;
    } else {
        std::shared_ptr<smart_singleton> tmp_shared(new smart_singleton());
        weak_instance = tmp_shared;
        return tmp_shared;
    }
}

It's fine for it to not be thread-safe, but in that case, it is very important for the documentation to make it clear. If you are going that route, however, it might be worth separating the get_instance from creation like so:

std::shared_ptr<smart_singleton> smart_singleton::create_instance()
{
    if (weak_instance.lock()) {
        throw already_instantiated_error{}; // something better
    }
    std::shared_ptr<smart_singleton> tmp_shared(new smart_singleton());
    weak_instance = tmp_shared;
    return tmp_shared;
}

std::shared_ptr<smart_singleton> smart_singleton::get_instance()
{
    return weak_instance.lock();
}

Then you can note in the documentation that get_instance() returns a nullptr if there is no instance present; in a "global" scope (like main), they should call auto keep_alive = smart_singleton::create_instance();.

smart_singleton.h needs header-guards, and it should include <memory>:

#ifndef SMART_SINGLETON_H_
#define SMART_SINGLETON_H_

#include <memory>

...

#endif

To me, the main problem with this design is that it is so easy to use wrong. Any class caching a static copy of the singleton will make the unit tests fail. The unit tests have to go blindly in and assume that there are no offending constructs anywhere.

It might be a better idea to just have a smart_singleton::reset() which resets all the data stored in the singleton that the unit tests can call in the tear-down method.

If I was doing this, I'd actually make this a CRTP class so that it becomes very easy to create smart_singletons:

template<class T>
class smart_singleton
{
public:
    static std::shared_ptr<T> get_instance()
    {
        if (auto existing_instance = weak_instance.lock()) {
            return existing_instance;
        } else {
            std::shared_ptr<T> tmp_shared(new T());
            weak_instance = tmp_shared;
            return tmp_shared;
        }
    }
protected:
    smart_singleton() {}
private:
    smart_singleton(const smart_singleton<T>&) = delete;
    smart_singleton operator=(const smart_singleton<T>&) = delete;
    smart_singleton(smart_singleton<T>&&) = default;
    smart_singleton& operator=(smart_singleton<T>&&) = default;

    static std::weak_ptr<T> weak_instance;
};

template<class T>
std::weak_ptr<T> smart_singleton<T>::weak_instance;

Now to make a class a singleton, this is all I'd have to do:

class my_singleton : public smart_singleton<my_singleton>
{
    friend class smart_singleton<my_singleton>; // so that the smart-singleton can access the private constructor of this class
public:
    ...
private:
    my_singleton();
};


Simple color flooding game in Javascript/HTML5 canvas area I'm learining JS. I have prepared a simple color flooding game, like Pixelated.

The engine below works, though scoring is not implemented.  Please help me find out how this code could be made better, more idiomatic JavaScript, or more efficient.

// global variables
var canvas = document.getElementById("game");
var context = canvas.getContext("2d");
var colors = ["#FF7F34", "#FF4D79", "#FFEF42", "#78E853", "#64C4FF", "#954FFF"];
var squareSize = 20;
var canvasSize = 14;
var buttonSize = 30;
var spaceSize = 15;
var board = [];
var currentColor = 0;

// draw 14x14 board of randomly coloured squares and add each square to 2d array 'board'
function drawBoard(colors) {
    // draw main board
    for (var i = 0; i < canvasSize; i++) {
        var boardColumn = [];
        for (var j = 0; j < canvasSize; j++) {
            colorId = Math.floor((Math.random() * 6)) + 1;
            context.fillStyle = colors[colorId - 1];
            context.fillRect(i*squareSize, j*squareSize, squareSize, squareSize);
             if (i == 0 && j == 0) {
                currentColor = colorId;
                colorId = 0;
            }
            boardColumn.push(colorId);
        }
        board.push(boardColumn);
    }
    // draw controls
    colors.forEach(function (value, i) {
        context.fillStyle = value;
        context.fillRect(i * (buttonSize + spaceSize) + spaceSize, squareSize * canvasSize + spaceSize, buttonSize, buttonSize);
    });
    expandOcean(currentColor);
    redrawBoard();
}
// click handler
function clickToColor(event) {
  var color = 0;
  var x = event.x;
  var y = event.y;
  x -= canvas.offsetLeft;
  y -= canvas.offsetTop;
  var controlsOffsetTop = squareSize * canvasSize + spaceSize;
  colors.forEach(function(value, i) {
    if (x > (spaceSize + i * (spaceSize + buttonSize)) && x <= ((i + 1) * (spaceSize + buttonSize)) && y > controlsOffsetTop && y <= (controlsOffsetTop + buttonSize)) color = i + 1;
  });
    if (color != 0 && color != currentColor)
        {
            currentColor = color;
            expandOcean(currentColor);
            redrawBoard();
        }
}
// flooding with chosen color
function expandOcean(color) {
    var touched = false;
    board.forEach(function(column, i) {
        column.forEach(function(value, j) {
            if (j + 1 < canvasSize && ((column[j] == 0 && column[j+1] == color) || (column[j] == color && column[j+1] == 0))) {
                board[i][j+1] = 0;
                board[i][j] = 0;
                touched = true;
            }
        });
    });
    board.forEach(function(column, i) {
        column.forEach(function(value, j) {
            if (i + 1 < canvasSize && ((column[j] == 0 && board[i + 1][j] == color) || (column[j] == color && board[i+1][j] == 0))) {
                board[i + 1][j] = 0;
                board[i][j] = 0;
                touched = true;
            }
        });
    });
    if (touched == true) expandOcean(color);
}

function redrawBoard() {
    board.forEach(function(column, i) {
        column.forEach(function(value, j) {
            if (board[i][j] == 0) {
                context.fillStyle = colors[currentColor - 1];
            } else {
                context.fillStyle = colors[board[i][j] - 1];
            }
            context.fillRect(i*squareSize, j*squareSize, squareSize, squareSize);
        });
    });
}

drawBoard(colors);
canvas.addEventListener('click', clickToColor, false);
<!DOCTYPE html>
<html>
<body>
    <canvas id="game" width="280" height="400"></canvas>
    <script type="text/javascript" src="game.js"></script>
</body>
</html>

This would be so much easier with the use of a table or divs. You could then create an array of objects (one object per square). Ex:

[
    {
        "className": "orange",
        "x": 0,
        "y": 0
    }
];

By doing this, you can match the class of the surrounding objects, rather than changing the color directly. Hope this helps.


Calculating reverse dependencies of a Debian package It works pretty well, but I suspect there's too many variables, and I wonder what else.

I'm using this library: Python APT.

#!/usr/bin/env python3

import argparse
import apt

def getdeps(deptype, pkg, otherpkg):
    deps = list()
    name = otherpkg.shortname
    otherpkg = otherpkg.candidate
    for deplist in otherpkg.get_dependencies(deptype):
        for dep in deplist.or_dependencies:
            if dep.name == pkg.shortname:
                deps.append(name)
    return deps

def reverse_dependencies(pkg):
    """Which packages have some kind of dependency on the given package"""

    cache = apt.cache.Cache()
    try:
        pkg = cache[pkg]
    except KeyError as e:
        print(str(e).strip('"'))
        return 1

    dependents = dict()
    recommends = list()
    suggests = list()
    replaces = list()
    enhances = list()
    depends = list()

    for key in cache.keys():
        otherpkg = cache[key]
        depends.append(getdeps("Depends", pkg, otherpkg))
        recommends.append(getdeps("Recommends", pkg, otherpkg))
        suggests.append(getdeps("Suggests", pkg, otherpkg))
        replaces.append(getdeps("Replaces", pkg, otherpkg))
        enhances.append(getdeps("Enhances", pkg, otherpkg))

    dependents["Depends"] = depends
    dependents["Recommends"] = recommends
    dependents["Suggests"] = suggests
    dependents["Replaces"] = replaces
    dependents["Enhances"] = enhances

    for deptype, deps in dependents.items():
        deps_output = list()
        for match in deps:
            if match:
                for item in match:
                    deps_output.append(item)
        if deps_output:
            print(deptype.upper(), end=": ")
            print(" ".join(deps_output))

if __name__ == '__main__':
    parser = argparse.ArgumentParser()
    parser.add_argument("pkg", help="pkg to describe")
    args = parser.parse_args()
    reverse_dependencies(args.pkg)

Example terminal session:

$ ./rdeps.py python3-apt
DEPENDS: python3-apt-dbg wajig
ENHANCES: python-apt-common

#!/usr/bin/env python3

import argparse
import apt

def getdeps(deptype, pkg, otherpkg):

I recommend not using abbrivates like dep or pkg.

    deps = list()

We usually create lists with [] not list()

    name = otherpkg.shortname
    otherpkg = otherpkg.candidate

I wouldn't do this. You only use it once, so its actually best to just otherpkg.candidate.get_dependencies Generally, I recommend against replacing one variable with another because often it increases confusion.

    for deplist in otherpkg.get_dependencies(deptype):
        for dep in deplist.or_dependencies:
            if dep.name == pkg.shortname:
                deps.append(name)

    return deps

Why does this function return a list? It seems to me that you are checking whether otherpkg depends on pkg. If so, this function really ought to return True or False.

def reverse_dependencies(pkg):
    """Which packages have some kind of dependency on the given package"""

    cache = apt.cache.Cache()
    try:
        pkg = cache[pkg]
    except KeyError as e:
        print(str(e).strip('"'))
        return 1

You don't do anything with this return value. you should pass it to sys.exit()

    dependents = dict()

We usually create dicts with {}

    recommends = list()
    suggests = list()
    replaces = list()
    enhances = list()
    depends = list()

These are all basically the same thing which means they shouldn't be seperate variables.

    for key in cache.keys():
        otherpkg = cache[key]
        depends.append(getdeps("Depends", pkg, otherpkg))
        recommends.append(getdeps("Recommends", pkg, otherpkg))
        suggests.append(getdeps("Suggests", pkg, otherpkg))
        replaces.append(getdeps("Replaces", pkg, otherpkg))
        enhances.append(getdeps("Enhances", pkg, otherpkg))

This would be better as iteration over a list of the dependency types. Also you are putting lists into lists. It'd be cleaner if you just had lists.

    dependents["Depends"] = depends
    dependents["Recommends"] = recommends
    dependents["Suggests"] = suggests
    dependents["Replaces"] = replaces
    dependents["Enhances"] = enhances

    for deptype, deps in dependents.items():
        deps_output = list()
        for match in deps:
            if match:

There's no point in doing this, because the loop will be executed 0 times if match is empty

                for item in match:
                    deps_output.append(item)

Use dep_output.extend(match) it has the same effect as this loop

        if deps_output:
            print(deptype.upper(), end=": ")
            print(" ".join(deps_output))

if __name__ == '__main__':
    parser = argparse.ArgumentParser()
    parser.add_argument("pkg", help="pkg to describe")
    args = parser.parse_args()
    reverse_dependencies(args.pkg)

My reworking of your code:

#!/usr/bin/env python3

import argparse
import apt
import sys

DEPENDENCY_TYPES = [
    "Depends",
    "Recommends",
    "Suggests",
    "Replaces",
    "Enhances",
]

def extract_dependencies(package, dependency_type):
    """
    Generator that produce all the dependencies of a particular type
    """
    for dependency_list in package.candidate.get_dependencies(dependency_type):
        for dependency in dependency_list.or_dependencies:
            yield dependency.name

def reverse_dependencies(pkg):
    """Which packages have some kind of dependency on the given package"""

    cache = apt.cache.Cache()
    try:
        pkg = cache[pkg]
    except KeyError as error:
        print(error.args[0])
        sys.exit(1)

    dependents = { name : [] for name in DEPENDENCY_TYPES }

    for key in cache.keys():
        other_package = cache[key]
        for dependency_type, specific_dependents in dependents.items():
            if pkg.shortname in extract_dependencies(other_package, dependency_type):
                specific_dependents.append(other_package.shortname)

    for dependency_type, specific_dependents in dependents.items():
        if specific_dependents:
            print(dependency_type.upper(), ": ", " ".join(specific_dependents))

if __name__ == '__main__':
    parser = argparse.ArgumentParser()
    parser.add_argument("pkg", help="pkg to describe")
    args = parser.parse_args()
    reverse_dependencies(args.pkg)


Find and Replace on PowerPoint Charts with Excel Sheets This will Search through all slides and shapes until it finds a chart, if it's not a Pie chart then it will open up the ActiveWorkbook behind that Chart then it will check the sheet for words in fndList and replace with with words in rplcList.

I would really just like to make this code run faster and smoother, and ask here in case there is a better/faster/more reliable way to do a find and replace for charts in PowerPoint.

Some things I believe could be improved are:

Every Chart has its ActiveWorksheet opened up and activated on the screen, so making this all run in the background would be nice.
I believe Excel.Application DisplayAlerts = False is a great solution to keeping the error We couldn't find anything to replace. Click options for more ways to search. from popping up, but if I'm not mistaken, turning off DisplayAlerts just automatically "clicks" the default value. So the pop-up still happens, and the code is still "pressing ok." So these are two unnecessary things that are happening in the background and consuming memory.
Late binding could be an option here, but I've never done it before and am not very familiar with how to do it correctly so I stayed away. But right now I have to load a reference to excel every time I run the code.

These are only three things that I could think of that could be improved. There might be many more.

Any and all help is appreciated. Thanks in advance!

Option Explicit

Private Sub findAndReplaceChrt()

'Timer start
Dim StartTime As Double
Dim SecondsElapsed As Double
StartTime = Timer

Dim pptPres As Object
Dim sld As Slide
Dim shpe As Shape
Dim c As Chart

Dim sht As Object
Dim fndList As Variant
Dim rplcList As Variant
Dim listArray As Long

Excel.Application.DisplayAlerts = False
Application.DisplayAlerts = False

fndList = Array("blue", "green")
rplcList = Array("Blue", "Green")

'Make pptPres the ppt active
Set pptPres = PowerPoint.ActivePresentation

'Loop through each sld and check for chart title, grab avgScore values and create pptTable to paste into ppt chart
For Each sld In pptPres.Slides

    'searches through shapes in the slide
    For Each shpe In sld.Shapes

        'Checks if shape is a Charts and has a Chart Title
        If Not shpe.HasChart Then GoTo nxtShpe

        Set c = shpe.Chart

        If Not c.ChartType = xlPie Then

            ActiveWindow.ViewType = ppViewNormal
            c.ChartData.Activate

            'Loop through each item in Array lists
            For listArray = LBound(fndList) To UBound(fndList)

                    Worksheets(1).Cells.Replace What:=fndList(listArray), Replacement:=rplcList(listArray), _
                    LookAt:=xlPart, SearchOrder:=xlByRows, MatchCase:=False, _
                    SearchFormat:=False, ReplaceFormat:=False

            Next listArray

            c.ChartData.Workbook.Close

        End If

nxtShpe:
    Next shpe

Next sld

Excel.Application.DisplayAlerts = True
Application.DisplayAlerts = True

'End Timer
SecondsElapsed = Round(Timer - StartTime, 2)
MsgBox "This code ran successfully in " & SecondsElapsed & " seconds", vbInformation

End Sub

This is pretty well optimized. All I can say is that some variables could have better names

pptPres - targetPresentation
sld - targetSlide
shpe - targetShape
c - targetChartObject
sht - targetSheet
fndList - wordsToFind

Otherwise, like I said, there's nothing I can really say need to be optimized.

If you don't have to change the window viewtype and activate the chart data, I'd get rid of those.


Using Regex to parse a chat transcript I need to classify each line as "announce, whisper or chat" once I have that sorted out I need to extract certain values to be processed.

Right now my regex is as follow:

var regex = new Regex(@"^\[(\d{2}:\d{2}:\d{2})\]\s*(?:(\[System Message\])?\s*<([^>]*)>|((.+) Whisper You :))\s*(.*)$");

Group 0 is the entire message.
Group 1 is the hour time of when the message was sent.
Group 2 is wether it was an announce or chat.
Group 3 is who sent the announce.
Group 4 is if it was a whisper or not.
Group 5 is who sent the whisper.
Group 6 is the sent message by the user or system.

Classify each line:

if 4 matches
 means it is a whisper
   else if 2 matches
     means it is an announce
       else
         normal chat

Should I change anything to my regex to make it more precise/accurate on the matches ?

Sample data:

[02:33:03] John Whisper You :  Heya
[02:33:03] John Whisper You :  How is it going
[02:33:12] <John> [02:33:16] [System Message] bla bla
[02:33:39] <John> heya
[02:33:40] <John> hello :S
[02:33:57] <John> hi
[02:33:57] [System Message] <John> has left the room
[02:33:57] [System Message] <John> has entered the room

You can always break it down in multiple lines to make it more readable. You can also use named groups which take the "magic" out of the group numbers (4 == whisper, 3 == normal, etc).

        var regex = new Regex(@"^\[(?<TimeStamp>\d{2}:\d{2}:\d{2})\]\s*" +
            @"(?:" +
                @"(?<SysMessage>\[System Message\])?\s*" +
                @"<(?<NormalWho>[^>]*)>|" +
                @"(?<Whisper>(?<WhisperWho>.+) Whisper You :))\s*" +
            @"(?<Message>.*)$");

        string data = @"[02:33:03] John Whisper You :  Heya
[02:33:03] John Whisper You :  How is it going
[02:33:12] <John> [02:33:16] [System Message] bla bla
[02:33:39] <John> heya
[02:33:40] <John> hello :S
[02:33:57] <John> hi
[02:33:57] [System Message] <John> has left the room
[02:33:57] [System Message] <John> has entered the room";

        foreach (var msg in data.Split(new char[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries))
        {
            Match match = regex.Match(msg);
            if (match.Success)
            {
                if (match.Groups["Whisper"].Success)
                {
                    Console.WriteLine("[whis from {0}]: {1}", match.Groups["WhisperWho"].Value, msg);
                }
                else if (match.Groups["SysMessage"].Success)
                {
                    Console.WriteLine("[sys msg]: {0}", msg);
                }
                else
                {
                    Console.WriteLine("[normal from {0}]: {1}", match.Groups["NormalWho"].Value, msg);
                }
            }
        }


Perfect Number usage in higher order function I am new to Scala.

I have been trying to complete an exercise where the ask was as follows:

// Write a function isPerfectNumber which takes integer input and returns String output.

// It finds if a number is perfect, and returns true if perfect, else returns false

// Write a higher order function myHigherOrderFunction which takes isPerfectNumber and intList as input, and returns a List of Strings which contain the output if the number is perfect or not using map.

Perfect Number :

https://rosettacode.org/wiki/Perfect_numbers#Scala

My Code :

object ListMapHigherOrder{
def main(args:Array[String])
{

val intRes = args.toList
val intList: List[Int] = intRes.map(_.toInt).toList

 def isPerfectNumber(input: Int) :String  =
 {
 var check_sum = ( (2 to math.sqrt(input).toInt).collect  { case x if input % x == 0 => x + input / x}  ).sum
 if ( check_sum == input - 1 )
      return "true"
      else
      return "false"
      }

def myHigherOrderFunction(argFn: Int => String, argVal:List[Int]): List[String] = { argVal.map(argFn) }

println(myHigherOrderFunction(isPerfectNumber, intList))

}
}

Code execution : scala ScalaExcercise12.scala 1 6 13

Expected Output : List(false , true , false)

the code gives expected output, am not sure how the backend testing is being done.... it just dosent pass the test.

Is there any issue with the code? - i did like to fix it , but cant i see anything wrong/missing

any help or direction is greatly appreciated.

Here is my corrected solution.

object ListMapHigherOrder{
  def main(args:Array[String]) = {

    val intList: List[Int] = args.map(_.toInt).toList

    def isPerfectNumber(input: Int) :String  = {
      val check_sum = ( (2 until input).collect  { case x if input % x == 0 => x }  ).sum
      if (input <= 1) "false"
      else if ( check_sum == input - 1 ) "true"
      else "false"
    }

    def myHigherOrderFunction(argFn: Int => String, argVal:List[Int]): List[String] = { argVal.map(argFn) }

    println(myHigherOrderFunction(isPerfectNumber, intList))
  }
}

Since this is supposed to be code review, I will not mince words. Some problems I noticed with your code:

1) Your formatting is irregular. For anyone trying to understand your code, this is a needless obstruction.

2) You do not check if the number is negative or equal to 1. In all such cases, the number is not perfect. You need to check for these cases.

3) You are using a var for check_sum. This is an eyesore for anyone who knows the language. Prefer immutable values.

4) You copied and pasted your code for isPerfectNumber from someone else without understanding what it does. In this case, the code you copied and pasted is actually wrong, because it gives the wrong answer for 1. It is probably also a good idea to explicitly check if the number is negative. The original code was also relatively inefficient because of the call to square root. The change to remove square root is only a micro-optimization, but it should be faster.

5) You are trusting that calling toInt on a String won't throw an exception. This depends on the assignment specification. If the exercise assumes you will always get well-formed input, then I suppose this is fine. But if not, you need to use a try-catch block or a Try monad. Since I am not sure if you need this stuff, I won't add it to your solution.

6) The idiom for myHigherOrderFunction is strange. There is almost no point to passing a list and a lambda function to a function just so you can call map on it. If you need to call map, just call map. Creating a wrapper function is unnecessary and an eyesore. I understand that this is what the assignment asks you to do, probably to test your understanding of functions as values. However, it is ugly code, I cannot help but mention that this should be avoided normally.

I think this question is rather manageable, and that if you had spent more time on it, you would have been able to get it yourself. However, I will not lecture you. The biggest mistake you made was trusting someone else's code without checking it yourself. So you should not trust me either.


How would you rate this python code? (Django AJAX) This code comes straight out from the example code in a Django book. I find it quite bad, mainly because the use of flags (if ajax then again if ajax) and unnecessarily bigger variable scope (set title='', tags='' and then use these variables in different flows). The function body also seems way too long.
I guess I'll subtract the book title (for the record I find the book in general good).

How would you rate this code, say out of 10 (10=very good, 3 and below being unacceptable)? (I'd rate it a 3)

The reason I ask is, I had rejected similar code in the past during code review and now I'm wondering whether I've been too strict or didn't understand python culture (I'm new to python).

@login_required
def bookmark_save_page(request):
  ajax = 'ajax' in request.GET
  if request.method == 'POST':
    form = BookmarkSaveForm(request.POST)
    if form.is_valid():
      bookmark = _bookmark_save(request, form)
      if ajax:
        variables = RequestContext(request, {
          'bookmarks': [bookmark],
          'show_edit': True,
          'show_tags': True
        })
        return render_to_response(
          'bookmark_list.html', variables
        )
      else:
        return HttpResponseRedirect(
          '/user/%s/' % request.user.username
        )
    else:
      if ajax:
        return HttpResponse(u'failure')
  elif 'url' in request.GET:
    url = request.GET['url']
    title = ''
    tags = ''
    try:
      link = Link.objects.get(url=url)
      bookmark = Bookmark.objects.get(
        link=link,
        user=request.user
      )
      title = bookmark.title
      tags = ' '.join(
        tag.name for tag in bookmark.tag_set.all()
      )
    except (Link.DoesNotExist, Bookmark.DoesNotExist):
      pass
    form = BookmarkSaveForm({
      'url': url,
      'title': title,
      'tags': tags
    })
  else:
    form = BookmarkSaveForm()
  variables = RequestContext(request, {
    'form': form
  })
  if ajax:
    return render_to_response(
      'bookmark_save_form.html',
      variables
    )
  else:
    return render_to_response(
      'bookmark_save.html',
      variables
    )

In my opinion it's little bit hard to read. You have to split it up into POST/GET methods.
Then you have to clean up code in POST/GET methods.
Something like this, for example.

@login_required
def bookmark_save(request):
    form = BookmarkSaveForm()
    if request.method == 'POST':
        form = bookmark_POST(request):
        if isinstance(form, HttpResponse):
            return form
    elif 'url' in request.GET:
        form = bookmark_GET(request)

    variables = RequestContext(request, {'form': form})

    if 'ajax' in request.GET:
        template_name = 'bookmark_save_form.html'
    else:
        temaplte_name = 'bookmark_save.html'
    return render_to_response(temaplte_name, variables)

And functions for POST/GET actions

def bookmark_POST(request):
    form = BookmarkSaveForm(request.POST)
    if form.is_valid():
        bookmark = _bookmark_save(request, form)
        if 'ajax' in request.GET:
            variables = RequestContext(request, {
                'bookmarks': [bookmark],
                'show_edit': True,
                'show_tags': True
            })
            return render_to_response(
                'bookmark_list.html', variables
            )
        else:
            return HttpResponseRedirect(
                '/user/%s/' % request.user.username
            )
    else:
        if 'ajax' in request.GET:
            return HttpResponse(u'failure')
    return form

def bookmark_GET(request):
    url = request.GET['url']
    try:
        link = Link.objects.get(url=url)
        bookmark = Bookmark.objects.get(
            link=link,
            user=request.user
        )
        title = bookmark.title
        tags = ' '.join(
            bookmark.tag_set.all().\
            values_list('name', flat=True)
        )
    except (Link.DoesNotExist, Bookmark.DoesNotExist):
        title = ''
        tags = ''
    form = BookmarkSaveForm({
        'url': url,
        'title': title,
        'tags': tags
    })
    return form


Idiomatic catching of unchecked exceptions in ExecutorService.submit OK, here's the problem space:
Lwjgl, a graphical library, has the ability to throw an unchecked LWJGLException on certain methods - like Display.create(). This becomes a problem when you have to off-load all work done by Display onto a single thread using an ExecutorService - that's what I'm doing, unfortunately, due to how Lwjgl works. ExecutorService.submit(Runnable) does not allow you to throw unchecked exceptions. ExecutorService.submit(Callable<V>) does, however, I still have to handle that exception.

My solution? I used the strategy pattern, like so.

private final ExecutorService executor;
private final ExceptionHandler exceptionHandler;

@Inject
public LwjglWindow(@Lwjgl ExecutorService executor, @Lwjgl ExceptionHandler exceptionHandler) {
    this.executor = executor;
    this.exceptionHandler = exceptionHandler;
}

public void start() {
    executor.submit(() -> {
        try {
            Display.create();
        } catch (LWJGLException ex) {
            exceptionHandler.onException(ex);
        }
    });
}

Using dependency injection, I inject an instance of ExceptionHandler to deal specifically with exceptions originating from Lwjgl. Should an exception occur from Lwjgl, well, then, this is called (from LwjglExceptionHandler):

@Override
public void onException(Exception exception) {
    logger.trace("lwjgl encountered an error", exception);
    systemShutdownStrategy.shutdown();
}

systemShutdownStrategy is an interface that is nothing more than a strategy pattern. This is injected into the instance of LwjglExceptionHandler, so it could be a stub and do nothing or it could shut down your program (which is, as the name of the field suggests, the intent). There's always room to implement some user-friendly error notification, rather than closing quietly, but this is just here for demonstration purposes.

The reason behind my choices here is that if a LwjglException occurs, the program is pretty much guaranteed to be unrecoverable. However, I'm concerned that I might be creating a code smell by the way I implement by start() method in LwjglWindow, given the side effects possible.

So, my question is, given that this is how I am handling my LwjglExceptions, is it a code smell, and what alternatives would you suggest in that case? (Also, a general review of my conventions would be appreciated if you'd like, as obviously just getting back into Java after 2 years in the .NETsphere)

Exception handling in Java has certain best practices. But not all situations are covered equally.

In this case, what you have, is a combination of logging, handling, and shutdown handling.

In this case, your handling appears to be sane. The centralization of this logging/shutdown is convenient because it essentially extracts the same functionality from multiple places in to a single place.

There are two issues that I can see:

Logging in Java often is tied to the class that the log message is called from. For example, the Logger is created with a name of the class as a constructor/key.

This means that the logging of the exception may/will be logged with the name of the ErrorHandler class rather than the class that threw the exception.

Depending on your circumstances, this may be a problem.
Logic flow in situations like this are often complicated by the return types of the class. For example, your code may look like:

public int myMethod() {
    try {
        // .... do something ....
        return 1;
    } catch (LWJGLException ex) {
        exceptionHandler.onException(ex);
    }
}

but, that code will not compile because you need to return an int value after handling the exception.

You know that there will never be an exit from the onException, because it shuts down. The compiler does not know that, so the compiler makes the wrong assessment of the exit path of the method.

I recommend you change the signature of the onException class, to have a return type of RuntimeException. With that, you can have:

   } catch (LWJGLException ex) {
       throw exceptionHandler.onException(ex);
   }

That will keep the compiler happy too.


Read n lines, and count the lines that do not contain the string "CD" I would like to know how to improve the execution speed of the code below.

The context : n is an integer read from the stdin, and represents the number of cases. Each case is represented by a sequence of characters (can only be uppercase alphabet letters), that are read from stdin. For each case, if the sequence doesn't contain the sub-string "CD", we add 1  to the variable wins.

Finally, we print out wins : the number of strings not containing the string "CD".

n = int(raw_input()) #number of cases
wins = 0
for i in range(n):
    if "CD" not in raw_input(): #count number of strings that doesn't contain "CD"
        wins += 1
print wins

Note: Official Python performance tips suggest using map or list comprehension instead of for loop  to speed up, but I couldn't implement those without the for loop.

What other ways to speed the execution speed, knowing that every thing read from the stdin is necessary, and that the typing speed (I use python filename.py <input.txt method) isn't an issue (I'm looking for a way pertaining to the code structure, to speed the execution itself) ?

If you used better variable names, you wouldn't need your first comment. The second one, though, is pure noise as it just repeat the code without adding anything.

To save on memory (and thus a bit on speed as you won't need to allocate the array) you should use xrange rather than range.

You don't use the variable i so better call it _ as it is a convention in Python for unused variables.

Lastly, you can use the sum built-in and a generator expression to perform the looping in C rather than in Python:

cases_count = int(raw_input())
wins = sum('CD' not in raw_input() for _ in xrange(cases_count))
print wins


Speed up Django migration that adds two fields into one Currently trying to migrate two fields of information into one field. We have many, many objects to go through and it takes roughly 12 minutes to loop through all the objects. That would mean the website would be down for 12 minutes and we do not want that. Is there any way to speed up this migration? It works, it's just slow.

from __future__ import unicode_literals

from django.db import migrations, models

def set_people(apps, schema_editor):
    ParticipationCount = apps.get_model('partcount', 'ParticipationCount')
    for row in ParticipationCount.objects.all():
        row.people = row.male + row.female
        row.save()

class Migration(migrations.Migration):

    dependencies = [
        ('partcount', '0002_participationcount_people'),
    ]

    operations = [
        migrations.RunPython(set_people),
    ]

No is an acceptable answer in this case, I am just curious if there is a different way to do this, as loops can be slow. I am using Django 1.8 and python 2.

Unfortunately you're using django<2.2, since from 2.2 onwards you can use bulk_update , but you can still use something like

ParticipationCount.objects.all().update(
    people=F("male") + F("female")
)

Which will do it in one query to save time. This will fail for certain fields in Postgres however as it requires some specific type casting, but this can be done within the query


Reading text from the web and writing to a JSON file This code reads text from the web using a WebDriver element and writes data to a JSON file. I had to use if-else (to write only the last line to the JSON file) just because of the asynchronous behaviour of NodeJs, otherwise I was trying to add fs.appendFile(filepath,"}") in the end to close the JSON file.

I just want you to optimize this snippet as I am new to NodeJs.

saveListOfAllCitiesIntoJsonFile () {
    var filePath = './Data/AllCitiesAvailableOnWeb.json';
    var CITY_COUNT = pageElements["CityCount"];
    var AllCity = homePageElements['AllCity'];
    var CityList = homePageElements['CityList'];
    super.findElementByClassName(AllCity).click();
    super.findElementsByClassName(CityList).then(function (element_CityName) {
        fs.truncate(filePath, 0);
        fs.appendFile(filePath, "{" + "\n");
        var jsonData = null;
        var elementData = null;
        for (var i = 0; i < CITY_COUNT; i++) {
            elementData = element_CityName[i].getText();
            if (i === CITY_COUNT - 1) {
                elementData.then(function (text) {
                    jsonData = "\"" + text + "\"" + ": " + "\"/hotels-in-" + text;
                    fs.appendFile(filePath, jsonData + "\"" + "\n" + "}", function (err) {
                        if (err) {
                            console.log(err);
                        }
                    });
                });
            }
            else {
                elementData.then(function (text) {
                    jsonData = "\"" + text + "\"" + ": " + "\"/hotels-in-" + text;
                    fs.appendFile(filePath, jsonData + "\"," + "\n", function (err) {
                        if (err) {
                            console.log(err);
                        }
                    });
                });
            }
        }
    });
}

saveListOfAllCitiesIntoJsonFile () {
    var filePath = './Data/AllCitiesAvailableOnWeb.json';

Now if I was a consumer of this API, the function name is very vague in a sense that I don't know where that file is or what the file is for. What is this JSON file you speak of? Why is it saving cities?

If you start to reason "read the source code", then that's the wrong way to write code. Better if you either make the function more flexible by letting the caller provide a path or make the name more meaningful in what it does. That way, the caller knows where the file is going to or what that file is for.

var CITY_COUNT = pageElements["CityCount"];
var AllCity = homePageElements['AllCity'];
var CityList = homePageElements['CityList'];

I'm saving cities, but what are pageElements and homePageElements? Where do they come from? How are they related to the function that's saving to the JSON? It's best if you break apart functions into small units of functionality. Functions that do too much are hard to test or even eyeball for flaws.

Aside from the comment given, where it advises to NOT construct JSON this way, you are also hitting the filesystem for every chunk of syntax. That's inefficient. Constructing JSON using the stringify function will allow you to create a string once. You can then write that string to the filesystem once. No chunks. No multiple writes.

I suggest you break this operation into two: One function that reads the data into a data structure, and another function that accepts the data structure, serializes it and writes to the file.

function getCitiesFromPage(pageElements, homePageElements){
  // traverse and collect cities from pageElements and homePageElements
  return cities;
}

function writeCitiesToFile(data, path){
  // serialize data and write to path
}

function saveCitiesToFile(){
  let cities = getCitiesFromPage(pageElements, homePageElements);
  writeCitiesToFile(cities, './Data/AllCitiesAvailableOnWeb.json');
}