Finding a peak in an array This finds a peak element in an array. Please consider I want only one peak element. Also, my code should follow only C++11/14 and not C++03.

#include <iostream>

int peak(int * arr, size_t size)
{
    auto start = 0;
    auto end = size - 1;

    //empty array
    if(arr == nullptr || size <= 0)
    {
        return -1;
    }

    while(start <= end)
    {
      auto  mid = start + (end - start)/2;
        printf("arr[mid] %d \n",arr[mid]);
        if(start == end)
            return arr[start];

        if(arr[mid] >= arr [mid+1] && arr[mid] >= arr[mid-1])
        {
            return arr[mid];
        }
        else if(arr[mid] < arr[mid-1])
        {
            end = mid;
        }
        else
        {
            start = mid;
        }

    }

    return -1;
}

int main(int argc, const char * argv[]) {
    // insert code here...

    int arr[] = {1,1,8,9,22,2};

    auto elem = peak(arr, sizeof(arr)/sizeof(int)); //Do not understand how to use decaltype instead of int

    printf("%d",elem);

    return 0;
}

If you really intend that this be used only on arrays, you can simplify its use somewhat by turning the function into a function template that receives its argument by reference (and since we're making it a template anyway, we might as well make it accept a variety of types, not just int).

template <class T, size_t size>
T peak(const T (&arr)[size]) {
    // ...

This way you can just pass the array itself, without explicitly passing the size, so the call in main turns into something like this:

int arr[] = {1,1,8,9,22,2};

auto elem = peak(arr);

...and the compiler uses its existing knowledge of the number of elements in arr instead of our having to pass it explicitly (but note that this simply won't work at all for a collection of items allocated with new or malloc--if you try to pass a pointer, the code will be rejected).

The obvious alternative (and possibly a superior one) would be to pass iterators instead. This would let us make this algorithm look and feel a lot more like the others in the standard library. A sub-alternative (so to speak) would be to use a range library (e.g., Eric Neibler's) and pass a range instead of separate iterators.

In keeping with the idea of using a template, I'd also consider changing your conditions so they all use the same comparison operator--traditionally C++ has only required operator< to be defined, and figures everything out from it. In this case it may be more work than it's worth though. It makes no difference for int, but minimizes the interface required by the template, so it can be applied to types that define <, but not any other comparison operators. If you don't anticipate applying it to such types, it may be easier to leave it alone.

I'd also consider renaming some of the variables. I see little gain from using arr instead of array, for one obvious example.

Headers: right now you have #include <iostream>, but you use printf instead of an iostream. I don't see any reason to use printf in this case--I'd use std::cout << elem; instead. If, for whatever reason, you want to continue using printf, include the right header (<cstdio>) instead of <iostream> though.

The one question you've asked is eliminated by turning the code into a template. If, for whatever reason, you decide not to do that, you don't need to use decltype. Rather, to compute the number of elements in an array you typically want to use: (sizeof(arr)/sizeof(arr[0])) (but the template is clearly the better approach).

I'd also prefer to fail loudly, and use some special name for code that checks a pre-condition, such as ensure or requires, so if you did pass a pointer, you'd start the code with something like this:

int peak(int const *array, size_t size) {
    ensure(array != nullptr);
    ensure(size > 0);

...with ensure defined something like this:

#define ensure(cond) (bool(cond) || fputs(#cond, stderr))


Checking URL validity This is sort of a follow up to a previous question (Do I have any SQL leaks?).

I have changed some things around and am working on a solution where I use Active Directory to gather information about the user rather than use the user table from the application that is run on these App Servers (the thing I am trying to keep track of).

I think that this method might be better utilized if it were written differently:

public bool isValid(string url)
{
    HttpWebRequest urlReq;
    HttpWebResponse urlRes;
    try
    {
        urlReq = (HttpWebRequest)WebRequest.Create(url);
        urlReq.Method = "HEAD";
        urlReq.Timeout = 200000;
        urlRes = (HttpWebResponse)urlReq.GetResponse();
        urlRes.Close();
        return true;
    }
    catch (Exception ex)
    {
        //Url not valid
        strErrorMSG = "Exception From isValid Method. Exception to follow: " + ex.ToString();
        return false;
    }
}

And this is how I am currently using it:

if (isValid(strInput))
{
    try
    {
        XmlReader xmlReader = XmlReader.Create(strInput);
        using (xmlReader)
        {
            while (xmlReader.Read())
            {
                if (xmlReader.NodeType == XmlNodeType.Text)
                {
                    strServer = xmlReader.Value.ToString();
                    strServer = strServer.Replace("\r", "");
                    strServer = strServer.Replace("\n", "");
                    strServer = strServer.Replace(" ", "");
                }
            }
            xmlReader.Close();
        }
    }
    catch (Exception e)
    {
        strServer = "Error";
        strErrorMSG = "Error with the xmlReader Exception as follows: " + e.ToString();
    }
}
else
{
    strServer = "XML-Missing";
}

I take this information and then I store it in an SQL table where I pull the information from it to populate a data grid on a website so I can see how many users are on each app server (using the App Server indirectly by running the application).

The variables strServer and strErrorMSG (are horrible names I know, I may refactor that as well while I am working on the version) get inserted into an SQL table so that I can see that there is an error with the Windows Service and have an error message that I can grab.

One of the things on my to-do list is to add a function that will email me with errors should the SQL Server not be accessible from the client machine.

I only use 1 SQL table at the moment for all the information that is gathered with this Windows service, another reason why I am working on the e-mail functionality.

Note: This is code from a Windows service that runs on almost 1000 computers.

How does my boolean method look, and can I tweak this somehow to make it work with the XmlReader more efficiently (or even at all)?

I created this Windows service to track which AppServer a user's application is connected to via a load balancer.

I install an XML on each AppServer with their own unique "server" node(which is pretty much the only thing there), the windows service needs to query that XML and return the value in that node giving it the server that the application is connected to. After that the Windows service sends the information to a SQL table so that it can be viewed via a website.

The reason for the weird logging is because this service is running on somewhere around 1000 computers in the state, and I am already sending information to the SQL Table, where the information is being displayed on the website(what we monitor frequently) so this is the fastest way at the moment for seeing the error messages.  I am looking into a way where I can have the exceptions E-mailed to me instead of only sending to the Table so that I don't have to stare at the site all the time.

I agree that I don't like the isValid method and that it creates an extra request.  What I am looking for is a way to merge it with the XmlReader and some of you have given me really good ideas how to accomplish this and I appreciate it greatly.

It can be tough, but pretend you're not the developer who wrote this code and try to understand what it does by reading through it. At a quick glance some questions you might ask could be:

What does isValid actually check the validity of?
What is strInput? The line isValid(strInput) just tells me that someone has deemed some input string valid for doing something. Not very useful, right?
When there is an exception in the XMLReader portion, what does it mean to just set the exception text to a random set of instance variables and continue on like nothing happened?

Some quick fixes might be to just use better variable/method names such as: isResolvableUrl(url) and inputXmlUrl.

On to your actual code...it's a very bad practice to use try/catch blocks for flow control. Exceptions should be exceptional (see: When to throw an exception)! Right now, you're just brushing exceptions under the carpet, which makes the rest of your application unable to properly handle them.

Think about the actual flow of your application in an ideal case. First, you make a request to verify if the file is there, which has network cost. Then, you use the same url to create an XmlReader which has to read from there again! Since you're catching all exceptions, you're better off not doing a check to see if it'll fail and just letting it fail without doing the check in the first place.

If each of your methods had a single identifiable functionality, you'll find you don't need all the try/catch blocks:

public XmlReader GetXmlFromRemoteLocation(string xmlLocation)
{ }

public void ReplaceWhitespace(XmlReader xml)
{ }

public void WhateverYourMainMethodIs()
{
    // Let it throw Exceptions that can be caught!
}

// Alternatively (if you must):
public bool TryWhateverYourMainMethodIs()
{
    try { }
    catch ( ExpectedExceptionTypes )
    { }

    return success;
}


Synchronizing two SQL Server database tables with the most recent changes to both tables I'm very new to SQL, but I'm coding a C# program that interacts with a database. There's a master database on my computer, and there's a client database on my tablet.

When I re-connect my tablet to the network, I want it to merge the two databases, so that changes made on the tablet are pushed to the master, and changes made to the master are pushed to the client.

Here's my code to initialize the local database on the tablet:

CREATE DATABASE LocalClaytonDB
ON (Filename = 'C:\ProgramData\Clayton\Clayton.mdf')
   , (Filename = 'C:\ProgramData\Clayton\Clayton_log.ldf')
FOR ATTACH;
GO

EXEC sp_addlinkedserver @server='Server'

Here's my code to synchronize the two databases:

-- update the client from the master
MERGE [LocalClaytonDB].[dbo].[tableName] trgt
using [Server].[Clayton].[dbo].[tableName] src

ON trgt.id = src.id

WHEN matched AND trgt.lastmodified <= src.lastmodified THEN
  -- if the master has a row newer than the client
  -- update the client
  UPDATE SET trgt.[allColumns]      = src.[allColumns],
             trgt.[id]              = src.[id],
             trgt.[lastmodified]    = src.[lastmodified]

-- delete any rows added by a client
WHEN NOT matched BY source
THEN
  DELETE

-- insert any rows added by the master
WHEN NOT matched BY target
THEN
  INSERT ( [allColumns],
           [id],
           [lastmodified])
  VALUES (src. [allColumns],
          src.[id],
          src.[lastmodified]);

-- now we update the master from the client
-- Note:
-- because the serverDB is a linked server
-- we can't use another MERGE statement, otherwise
-- we get the error: "The target of a MERGE statement
-- cannot be a remote table, a remote view, or a view over remote tables."

UPDATE
    serverDB

SET
    [allColumns]        = [localDB].[allColumns],
    [id]                = [localDB].[id],
    [lastmodified]      = [localDB].[lastmodified]

FROM
     [Server].[Clayton].[dbo].[tableName] serverDB

INNER JOIN
     [LocalClaytonDB].[dbo].[tableName] localDB

-- update where the id is the same but the client is newer than the master

ON serverDB.id = localDB.id
       AND localDB.lastmodified >= serverDB.lastmodified

Also, I'm slightly worried about what would happen if this script failed partway through the process, and also just for anything I might be missing.

I'd love it if someone could look over this and point out any shortcomings. As I said, this is the first real SQL I've written and my program is going to have to rely on it.

How often would you be writing to the master? And what purpose is Master serving in your setup?

Merge or peer-to-peer replication is one solution to keep both databases in sync but keep in mind they can be time consuming to troubleshoot and adds another extra thing in the mix to worry about.

Typically this would be done by updating records based on ID and last_updated key on tables. Merge statements can be a cause for race conditions where multiple users try and run the merge at the same time. Doing these operations with individual statements may keep you more clarity on the different operations that are happening on the db and keep things simple.

Delete: I would do this first by marking the records as 'soft-delete' (can be bit value) on master. Then running a sql server job on a schedule to remove all the records from tables which are marked for deletion.

Insert: Pull new records down from master based on the primary key value being higher than what's in the client.

Update: Based on the last_update field. This will solve the issue of updating tables on the remote server as well.


Locking mechanism in C# I would like you to review the locking mechanism I implement in C# class. Will it be working correctly with many threads?
Please don't mind rest of the code because I just try to repair the class quickly to have a thread safe logging class for my project.
Any other suggestions are welcome.

public static class Debug
{

    static readonly object _consoleLocker = new object();
    static readonly object _diskDriveLocker = new object();

    public static void Log(Tryb tryb, int level, string caller, string message, string messageExt)
    {
        String strAppDir = Path.GetDirectoryName(Assembly.GetExecutingAssembly().GetName().CodeBase);
        strAppDir = strAppDir.Replace("file:\\", ""); // dla windowsa
        strAppDir = strAppDir.Replace("file:", "");  // dla linuksa
        string file = Path.Combine(strAppDir, "log.txt");
        ProcessLogMessage(tryb, "[log]", caller, message, messageExt, file);
    }

    public static void Log(Tryb tryb, int level, string caller, string message, string messageExt, string file)
    {
        ProcessLogMessage(tryb, "[log]", caller, message, messageExt, file);
    }

    public static void Error(Tryb tryb, string caller, string message, string messageExt)
    {
        String strAppDir = Path.GetDirectoryName(Assembly.GetExecutingAssembly().GetName().CodeBase);
        strAppDir = strAppDir.Replace("file:\\", ""); // dla windowsa
        strAppDir = strAppDir.Replace("file:", "");  // dla linuksa
        string file = Path.Combine(strAppDir, "error.txt");
        ProcessLogMessage(tryb, "[error]", caller, message, messageExt, file);
    }

    public static void Error(Tryb tryb, string caller, string message, string messageExt, string file)
    {
        ProcessLogMessage(tryb, "[error]", caller, message, messageExt, file);
    }

    static void ProcessLogMessage(Tryb tryb, string rodzaj, string caller, string message, string messageExt, string file)
    {
        string dane = String.Format("{0}, {1}, {2}, {3}, {4}", DateTime.Now, rodzaj, caller, message, messageExt);
        switch (tryb)
        {
            case Tryb.FILE:
                lock (_diskDriveLocker)
                {
                    WriteLogToFile(dane, file);
                }
                break;
            case Tryb.CONSOLE:
                lock (_consoleLocker)
                {
                    Console.WriteLine(dane);
                }
                break;
            case Tryb.FILEANDCONSOLE:
                lock (_consoleLocker)
                {
                    Console.WriteLine(dane);
                }
                lock (_diskDriveLocker)
                {
                    WriteLogToFile(dane, file);
                }
                break;
        }
    }

    static void WriteLogToFile(string strLogMessage, string strLogFile)
    {
        StreamWriter swLog = null;

        try
        {
            if (!File.Exists(strLogFile))
                swLog = new StreamWriter(strLogFile);
            else
                swLog = File.AppendText(strLogFile);

            swLog.WriteLine(strLogMessage);
        }
        finally
        {
            if (swLog != null)
            {
                swLog.Close();
                swLog.Dispose();
            }
        }
    }

}

public enum Tryb
{
    FILE = 0,
    CONSOLE = 1,
    FILEANDCONSOLE = 2
}

First of all, your locking seems ok, I don't expect any drawbacks by using multiple threads.

That being said, let us review this from bottom to top.

public enum Tryb
{
    FILE = 0,
    CONSOLE = 1,
    FILEANDCONSOLE = 2
}

The name of that enum is at the nicest say unclear. A name like LogTarget would be more clear and its meaning would be obvious to any reader/user of this code.
Because it only has 3 members you should consider to add the [Flags] atribute to that enum which would result in a much shorter code, because you could omit the switch statement in ProcessLogMessage method.

This would look like so

[Flags]
public enum LogTarget
{
    FILE = 1,
    CONSOLE = 2,
    FILEANDCONSOLE = FILE | CONSOLE
}

static void WriteLogToFile(string strLogMessage, string strLogFile)
{
    StreamWriter swLog = null;

    try
    {
        if (!File.Exists(strLogFile))
            swLog = new StreamWriter(strLogFile);
        else
            swLog = File.AppendText(strLogFile);

        swLog.WriteLine(strLogMessage);
    }
    finally
    {
        if (swLog != null)
        {
            swLog.Close();
            swLog.Dispose();
        }
    }
}

instead of having a Try..Finally and 2 different ways of creating the StreamWriter you should use the overloaded constructor StreamWriter(string, boolean) and enclose its usage in a using block which takes care of disposing and closing automatically.

Based on the valid comment of @Heinzi

  You don't need bool append = File.Exists(strLogFile);, just use new
  StreamWriter(strLogFile, true). According to the documentation, the
  Boolean parameter has no effect if the file does not exist

you can just use true for append so no need to check if the file exists.
although braces {} are optional for single line if..else statements you really should use them because they will make your code less error prone.

Applying this will lead to

static void WriteLogToFile(string strLogMessage, string strLogFile)
{
    using (StreamWriter swLog = new StreamWriter(strLogFile, true))
    {
        swLog.WriteLine(strLogMessage);
    }
}

static void ProcessLogMessage(Tryb tryb, string rodzaj, string caller, string message, string messageExt, string file)
{
    string dane = String.Format("{0}, {1}, {2}, {3}, {4}", DateTime.Now, rodzaj, caller, message, messageExt);
    switch (tryb)
    {
        case Tryb.FILE:
            lock (_diskDriveLocker)
            {
                WriteLogToFile(dane, file);
            }
            break;
        case Tryb.CONSOLE:
            lock (_consoleLocker)
            {
                Console.WriteLine(dane);
            }
            break;
        case Tryb.FILEANDCONSOLE:
            lock (_consoleLocker)
            {
                Console.WriteLine(dane);
            }
            lock (_diskDriveLocker)
            {
                WriteLogToFile(dane, file);
            }
            break;
    }
}

Now we can use the enum by calling the HasFlags() method which results in only 2 if statements.

static void ProcessLogMessage(LogTarget target, string rodzaj, string caller, string message, string messageExt, string file)
{
    string logMessage = String.Format("{0}, {1}, {2}, {3}, {4}", DateTime.Now, rodzaj, caller, message, messageExt);

    if (target.HasFlag(LogTarget.FILE))
    {
        lock (_diskDriveLocker)
        {
            WriteLogToFile(logMessage, file);
        }
    }

    if (target.HasFlag(LogTarget.CONSOLE))
    {
        lock (_consoleLocker)
        {
            Console.WriteLine(logMessage);
        }
    }
}

The composition of the error.txt and log.txt filenames should be done in separate methods and be assigned to class level variables so they could be reused.


isPrime Number Algorithm public class RunnableThreadPrimeNumber implements Runnable
{
    private long startNum;
    private long endNum;
    private Thread t;

RunnableThreadPrimeNumber(long start , long end )
{
        t = new Thread(this);
        this.startNum = start;
        this.endNum = end;

        t.start();
}

public void run()
{
    for ( int i = start ; i <= last ; i++)
    {
        if (isPrime(i) == true)
        {
            System.out.println(i + " is prime")
        }

    }

}

private boolean isPrime(long n)
{
    int counter = 0;

    for (int i = 2 ; i < n / 2 ; i++)
    {
        if ( n % i == 0 )
        {
            counter ++;
        }
    }

    if ( count > 0 )
    {
        //number is prime
        return true;
    }
    else
    {
        //number is not prime
        return false;
    }

}

public static void main(String[] args)
{
    RunnableThreadPrimeNumber(2000,4000);
    RunnableThreadPrimeNumber(4001,6000);
}

}

Questions:

Is there any way to improve this algorithm which checks if a given number is prime? The improvement can be anything from space to running time complexity.
For my function isPrime(long n), I am currently looping through every possible number less than half the input number and returning true or false only after the for loop has finished.

I am thinking of adding an addition if statement in the for loop which checks if the variable counter is greater than 0.  If it's greater than 0, immediately return true. The advantage to this approach is that for some numbers, I can immediately return true/false without needing to finish the for loop.  However, the cost comes at adding an additional if statement for every iteration in the for loop.

How should I go about deciding if the additional if statement is worth the cost?

Code review:

As a rule, it's almost never a good idea to create your own thread directly - better to  let an ExecutorService do the thread management for you.

if (isPrime(i) == true)

should be written

if (isPrime(i))

similarly

if ( count > 0 )
{
    //number is prime
    return true;
}
else
{
    //number is not prime
    return false;
}

should be

return count > 0 ;

You should also write some tests, since this condition looks backwards.

But for this specific case, kasperd has the right idea - you don't care how many different factors that the number has, just whether or not it is prime; early exit is the right answer here.

It would improve the code to separate the calculation from the reporting.

interface PrimeListener {
    void onPrime(long prime);
}

class TrivialPrimeListener implements PrimeListener {
    private final PrintStream out;

    TrivialPrimeListener (PrintStream out) {
        this.out = out;
    }

    TrivialPrimeListener () {
        this(System.out);
    }

    void onPrime(long prime) {
        // Even better would be to make this flexible about where it
        // sends t
        out.println(prime + " is prime");
    }
}

Then your loop would look like:

for ( long primeCandidate = start ; primeCandidate <= last ; primeCandidate++)
{
    if (isPrime(primeCandidate))
    {
        this.listener.onPrime(primeCandidate);
    }

}

And your initialization code would look something like

    this.startNum = start;
    this.endNum = end;
    this.listener = new TrivialPrimeListener();


Getting ready for Nullable Reference Types Looking forward for C# Nullable Reference Types in C# 8.0. I do not like to use the preview implementation in a production code though, so just went with these simple helper classes intending them to be upgraded to new syntax by search and replace when time comes.

Usage sample:

static void Main(string[] args)
{
    var johnk = new FullName("John", "F.", "Kennedy");
    var johnd = new FullName("John", null, "Do");
    var johnn = new FullName("John", null, null); // throws
}

Where business class is:

class FullName
{
    public FullName(Some<string> first, Option<string> middle, Some<string> last) =>
        (First, Middle, Last) = (first, middle, last);

    public string First { get; }
    public string Middle { get; }
    public string Last { get; }
}

Library code:

[DebuggerStepThrough]
struct Some<T> where T : class
{
    public static implicit operator Some<T>(T value) =>
        new Some<T>(value ?? throw new ArgumentNullException());

    public static implicit operator T(Some<T> some) =>
        some.Value ?? throw new ArgumentNullException();

    Some(T value) => Value = value;
    T Value { get; }
}

And

[DebuggerStepThrough]
struct Option<T> where T : class
{
    public static implicit operator Option<T>(T value) =>
        new Option<T>(value);

    public static implicit operator T(Option<T> some) =>
        some.Value;

    Option(T value) => Value = value;
    T Value { get; }
}

Nothing fancy, just does the job and it would be easy to upgrade syntax…

There are some disadvantanges about this solution. You not only loose the argument-name inside the exception but if you are using ReSharper then you won't be able to quick-fix this in future.

Thus I think it's not necessary to create such helpers because as soon as the new feature is there, ReSharper will probably have nice quick-fix for that so it'll turn all if(foo == null) throw new ArgumentException(..) into the new syntax. I doubt ReSharper will be able to quick-fix this.

Besides if you properly decorate your code with the [NotNull] and [CanBeNull] attributes, you are half-way safe. Since I started using them, It's been ages that I last saw a NullReferenceExcepiton or ArgumentNullExcepiton.

I also find ReSharper's naming convention more appealing. This means

Some becomes NotNull and
Option becomes CanBeNull


Find prime numbers from 1 to 100 Create a program to find all the prime numbers between 1 and 100.

  One way to do this is to write a function that will check if a number is prime (i.e., see if the number can be divided by a prime number smaller than itself) using a vector of primes in order (so that if the vector is called primes, primes[0]==2, primes[1]==3, primes[2]==5, etc.). Then write a loop that goes from 1 to 100, checks each number to see if it is a prime, and stores each prime found in a vector. Write another loop that lists the primes you found. You might check your result by comparing your vector of prime numbers with primes. Consider 2 the first prime.

Do you think my codes are short, simple, efficient, and easy to understand?

#include <iostream>
#include <string>
#include <vector>

int main()
{
    std::vector<int> primes = { 2 };
    for (int i = 2; i <= 100; ++i)
        for (int j = 2; j < i; ++j) {
            if (i % j == 0)
                break;
            if (j == i - 1)
                primes.push_back(i);
        }
    for (int x: primes)
        std::cout << x << ' ';
    return 0;
}

First suggestion: do a search before posting, there are many very similar questions on codereview.
What the problem itself suggests is actually better than what you have, because it suggests you write a function to check if a number is prime, while you have everything patched together.
So, the first step is, split the code and write a function that is something like bool is_prime(int input_number)
Then, as said in the comments, you can stop at the square root of the number you're checking.
You know that even numbers are not primes, so you can consider 2 the first prime, start the loop at 3 and increment by 2 instead of 1
Your code will print an extra blank space at the end
Just as an extra optimization, you can keep track of the primes that you've already found and use those to check, skipping what's in between, and continuing from there


Listing payments where all payments lines and transactions are closed The goal of this query is to get a list of payments where all the corresponding payment lines and transactions have the status CLOSED. These are one to many relationships. The database is Informix.

I wonder if there is a more efficient way to write this? In its current state it has a lot of repetition which, maybe because my Java background, seems quite ugly to me. Eventually this query will need to be generated through Hibernate, so I'm mostly concerned with its performance.

SELECT p.* FROM payment p
JOIN transaction t
ON t.payment_bk = p.bk
JOIN payment_line pl
on  p.bk = pl.payment_bk
WHERE p.valid AND p.valid_to is null
AND p.date>{ts '2015-05-21 00:00:00'}
AND p.date<{ts '2015-05-22 00:00:00'}
AND t.valid and t.valid_to is null
AND t.transaction_status = 'CLOSED'
AND NOT EXISTS (SELECT 1 FROM transaction t WHERE t.payment_bk = p.bk
AND t.transaction_status <> 'CLOSED'
AND t.valid and t.valid_to is null)
AND pl.valid and pl.valid_to is null
AND pl.payment_line_status = 'CLOSED'
AND NOT EXISTS (SELECT 1 FROM payment_line WHERE pl.payment_bk = p.bk
AND pl.payment_line_status <> 'CLOSED'
AND pl.valid and pl.valid_to is null)

valid is a boolean and the dates/timestamps will sometimes be just a day apart, sometimes a couple of months.

White space
Since you come from a Java background, you will understand the importance of using white space to make the code easier to read. Also it is best to stay consistent with capitalization of keywords. I would go for something like this:
SELECT p.*
FROM payment p
  JOIN transaction t
    ON t.payment_bk = p.bk
  JOIN payment_line pl
    ON  p.bk = pl.payment_bk
WHERE p.valid IS NULL
  AND p.valid_to IS NULL
  AND p.date>{ts '2015-05-21 00:00:00'}
  AND p.date<{ts '2015-05-22 00:00:00'}
  AND t.valid IS NULL
  AND t.valid_to IS NULL
  AND t.transaction_status = 'CLOSED'
  AND NOT EXISTS (
    SELECT 1
    FROM transaction t
    WHERE t.payment_bk = p.bk
      AND t.transaction_status <> 'CLOSED'
      AND t.valid and t.valid_to IS NULL
  )
  AND pl.valid IS NULL
  AND pl.valid_to IS NULL
  AND pl.payment_line_status = 'CLOSED'
  AND NOT EXISTS (
    SELECT 1
    FROM payment_line
    WHERE pl.payment_bk = p.bk
      AND pl.payment_line_status <> 'CLOSED'
      AND pl.valid IS NULL
      AND pl.valid_to IS NULL
  );

So, that already reads a lot better!

Table aliases are handy, but in your case I think your use of single-letter aliases makes the query harder to read. For instance, instead of payment p JOIN transaction t I would suggest something like payment AS pay INNER JOIN transaction AS tran. And payment_line pl could be payment_line AS line or something like that.

Normally I would recommend using Common Table Expressions instead of sub-queries, but since this will eventually be done by Hibernate it would not matter too much right now.

For clarity's sake, I would avoid to chain WHERE conditions like this:
WHERE p.valid AND p.valid_to IS NULL

I missed those at first, so that gives you an idea why.


Elegant function to "increase" a JavaScript string - for example, turning "aac" into "aad" I'm writing a function to increase a string so, for example, "aac" becomes "aad" and "aaz" becomes "aba". The result is horribly inelegant, I can't get it simple enough and I feel I'm missing something. How to improve that code?

function inc_str(str){
    var str = str.split("").map(function(a){ return a.charCodeAt(0); });
    var n = 1;
    while (n<str.length){
        str[str.length-n]++;
        if (str[str.length-n] > "z".charCodeAt(0))
            str[str.length-n] = "a".charCodeAt(0),
            ++n;
        else
            break;
    };
    return str.map(function(a){ return String.fromCharCode(a); }).join("");
};

Extra points for a brief functional style solution! (Even if it needs to implement other functional functions such as zip or whatever.)

A recursive approach is much more elegant in this case:

function inc_str(str) {
  var last = str[str.length - 1],
    head = str.substr(0, str.length - 1);
  if (last === "z") {
    return inc_str(head) + "a";
  } else {
    return head + String.fromCharCode(last.charCodeAt(0) + 1);
  }
}

Making it wrap around for inc_str('z') is left as an exercise for the reader (hint: last becomes undefined).


Module for providing users easy access to all social networking websites The following piece of code will act as a single access to all social networks. It aims to connect different social networks to get a broader picture of the social life of a user. I have just committed the code for Twitter access, but similar access will be provided to Facebook and Github.

The code is available here.

I'd appreciate any and all comments-- correctness, style, best practices, logging, error handling.

import tweepy
import TwitterConfig as config_twitter

class Twitter :

def __init__(self):
    self.CONSUMER_KEY = config_twitter.get_consumer_key()
    self.CONSUMER_SECRET = config_twitter.get_consumer_secret()
    self.oauth_token = None
    self.oauth_verifier = None
    self.api = None

def get_auth_url(self):
    self.auth = tweepy.OAuthHandler(self.CONSUMER_KEY, self.CONSUMER_SECRET)
    return self.auth.get_authorization_url()

def get_request_token(self):
    return (self.auth.request_token.key,self.auth.request_token.secret)

def set_token(self,token):
    self.oauth_token = token

def set_verifier(self,verifier):
    self.oauth_verifier = verifier

def set_request_token (self,ReqToken):
    self.request_token = ReqToken

def get_access_token (self):
    self.auth = tweepy.OAuthHandler(self.CONSUMER_KEY, self.CONSUMER_SECRET)
    token = self.request_token
    #session.delete('request_token')
    self.auth.set_request_token(token[0],token[1])
    self.auth.get_access_token(self.oauth_verifier)

def authorize (self):
    key = self.auth.access_token.key
    secret = self.auth.access_token.secret
    self.auth = tweepy.OAuthHandler(self.CONSUMER_KEY, self.CONSUMER_SECRET)
    self.auth.set_access_token(key, secret)
    self.api = tweepy.API(self.auth)

def update_status(self,status):
    if self.oauth_token == None or self.oauth_verifier == None or self.api == None :
        return "Verification Problem"
    self.api.update_status (status)
    return "Done"

def user_information (self):
    # returns information of the authenticate user
    if self.oauth_token == None or self.oauth_verifier == None or self.api == None :
        return "Verification Problem"
    return self.api.me()

def get_friends (self):
    if self.oauth_token == None or self.oauth_verifier == None or self.api == None :
        return ["Verification Problem"]
    return self.api.GetFriends(self.user_information().name)

def get_followers (self):
    if self.oauth_token == None or self.oauth_verifier == None or self.api == None :
        return ["Verification Problem"]
    return self.api.GetFollowers()

def get_followers_id (self):
    if self.oauth_token == None or self.oauth_verifier == None or self.api == None :
        return "Verification Problem"
    return self.api.followers_ids()

def get_friends_ids (self):
    # returns ids of the friends
    if self.oauth_token == None or self.oauth_verifier == None or self.api == None :
        return "Verification Problem"
    return self.api.friends_ids()

def get_rate_limit_status (self):
    #returns the rate limit status of the authenticated user
    return self.api.rate_limit_status()

def get_tweets(self):
    if self.oauth_token == None or self.oauth_verifier == None or self.api == None :
        return ["Verification Problem"]
    me = self.user_information()
    statuses = self.api.GetUseerTimeline(me.name)
    return statuses

def get_messages(self):
    if self.oauth_token == None or self.oauth_verifier == None or self.api == None :
        return ["Verification Problem"]
    return self.api.GetDirectMessages()

You are inconsistent in naming. Some of your methods are in camelCase and some are in under_score format. Pick one.

Where is the consistent API for the various social networks?? From your write up I expect to be able to write something like the following:

my_friends = []
for soc in my_social_networks:
    my_friends.append(soc.get_friends())
show(my_friends)

or:

my_messages = []
for soc in my_social_networks:
    my_messages.append(soc.get_current_messages())
show(my_messages)

Instead I see get_tweets or get_wall. That means I have to pay attention to whether I have a Twitter object or a Facebook object. So all of my code needs to have a long if/else for each new type of social network I want to support in my program.

Double underscores are not meant for code outside of Python. Use a single underscore.


Parsing molecular formula Is it possible to improve this code in terms of efficiency / line number ? (avoid nested code ...). It comes from a test I failed. I don't know how I could have done it better.

from collections import Counter

def parse_molecule(molecule):

    array = [[]]

    for token in map(str, molecule):
        if token.isalpha() and token.istitle():
            last = [token]
            upper = token
            array[-1].append(token)
        elif token.isalpha():
            last = upper + token
            array[-1] = [last]
        elif token.isdigit():
            array[-1].extend(last*(int(token)-1))
        elif token == '(' or token == '[':
            array.append([])
        elif token == ')' or token == ']':
            last = array.pop()
            array[-1].extend(last)

    return dict(Counter(array[-1]))

Examples of function usage are following:

>>> water = 'H2O'
>>> parse_molecule(water)
>>>{'H': 2, 'O': 1}

>>> magnesium_hydroxide = 'Mg(OH)2'
>>> parse_molecule(magnesium_hydroxide)
>>> {'Mg': 1, 'O': 2, 'H': 2}

>>> fremy_salt = 'K4[ON(SO3)2]2'
>>> parse_molecule(fremy_salt)
>>> {'K': 4, 'O': 14, 'N': 2, 'S': 4}

This is the statement:

  For a given chemical formula represented by a string, count the number
  of atoms of each element contained in the molecule and return a dict.

For example:

water = 'H2O'
parse_molecule(water)
return {'H': 2, 'O': 1}

magnesium_hydroxide = 'Mg(OH)2'

parse_molecule(magnesium_hydroxide)

return {'Mg': 1, 'O': 2, 'H': 2}

fremy_salt = 'K4[ON(SO3)2]2'

parse_molecule(fremy_salt)

return {'K': 4, 'O': 14, 'N': 2, 'S': 4}

As you can see, some formulas have brackets in them. The index outside the brackets tells you that you have to multiply count of each atom inside the bracket on this index. For example, in Fe(NO3)2 you have one iron atom, two nitrogen atoms and six oxygen atoms.

Note that brackets may be round, square or curly and can also be nested. Index after the braces is optional.

This code is confusing and inefficient:

for token in map(str, molecule):

It treats molecule as some kind of iterable object, and applies the str function to each element the iteration returns, an assigns each result successively to token.  In this case, molecule is a string, which is already a sequence of iterable characters, and a single character is itself a string, so this map function is taking individual character strings and calling str() on those character strings to return the same character strings.

The statement could be written much simpler:

for token in molecule:

These statements are testing for only 2 of the three grouping brackets:

    elif token == '(' or token == '[':
        ...
    elif token == ')' or token == ']':
        ...

The correct statements would read:

    elif token == '(' or token == '[' or token == '{':
        ...
    elif token == ')' or token == ']' or token == '}':
        ...

however, this is rather verbose.  You can use the in operator to check if a character appears anywhere in a string, so these test could be written more compactly as:

    elif token in '([{':
        ...
    elif token in ')]}':
        ...

isdigit() is not the best function to use to verify a string can be parsed as an integer.  It includes superscript and subscript digits, and other characters which will cause an exception when passed into int(...):

>>> "³₂".isdigit()
True

Instead, use isdecimal().

As mention by scnerd in their answer, it is hard to parse strings character by character.  You are detecting a lower case letter, adding it to the last token you found, and replacing the last token.  You would need to do the same thing when you find a digit, and only when the current series of digits end perform the replication of the last radical, but this results in numerous special cases, which again make the parsing hard.

It is better to break the molecule into low level tokens, not individual characters, and process complete tokens.  As mention by scnerd, the regular expression engine can help.  They diverted into attempt to match parenthesis in the regular expression parsing, which complicates things far too much.  Here is a simpler approach:

import re

def tokenize_molecule(molecule):
    return re.findall('[A-Z][a-z]?|\d+|.', molecule)

molecules = ['H2O', 'Mg(OH)2', 'K4[ON(SO3)2]2', 'C12H22O11']

for molecule in molecules:
    print(molecule)
    print("\t", tokenize_molecule(molecule))

which outputs:

H2O
     ['H', '2', 'O']
Mg(OH)2
     ['Mg', '(', 'O', 'H', ')', '2']
K4[ON(SO3)2]2
     ['K', '4', '[', 'O', 'N', '(', 'S', 'O', '3', ')', '2', ']', '2']
C12H22O11
     ['C', '12', 'H', '22', 'O', '11']

Here, the regex has 3 parts: [A-Z][a-z]? which matches an element, \d+, which matches one or more digits, and . which matches any character.  These are joined together with the | alternative operator; if the current character does not begin an element name, or a digit group, it is returned as a single character ... which is one of the bracket characters in a properly formed molecule.

Using this initial parsing will make the parsing the molecule much easier:

def parse_molecule(molecule):

    # ...

    for token in re.findall('[A-Z][a-z]?|\d+|.', molecule):

        if token.isalpha():
            # ...
        elif token.isdecimal():
            count = int(token)
            # ...
        elif token in '([{':
            # ...
        elif token in ')]}':
            # ...
        else:
            raise ValueError('Unrecognized character in molecule')

    # ...


Attempting to utilize OOP with a user-management class Originally, this isn't how I would have done this at all. I was told by someone I know who is a programmer that I needed to include several functions: one for removing and adding users, one for searching for users information etc.

I would have made it so you can pass a username into the constructor and then get all the user info and leave it to another UserManagement class to take care of things like adding and removing users, but I was told (very very forthrightly) that, that was NOT the way of going about doing what I'm doing.

Am I using OOP correctly? I have asked 2 people and their opinions are almost directly opposite to each others.  The code below works fine, performs well and does what I want it to just fine, but I'm worried that I'm not quite getting when and when not to use OOP.

class User{
    public $username;
    public $uid;
    public $date;
    public $location;

    public function __construct(){
    }

    public function getUser($user){

        $getUserInfo = new Database;
        $getuserInfo("SELECT * FROM users WHERE username = ?", array($user));
        $result = $getUserInfo->result;

        foreach ($result as $value) {
            $this->username = $value['username'];
            $this->uid = $value['uid'];
            $this->date = $value['date'];
            $this->location = $value['location'];
        }
    }

    public function getUserByUID($uid){
        $getUserInfo = $GLOBALS['databaseUserManagement']->prepare("SELECT * FROM users WHERE uid = ?");
        $getUserInfo->execute(array($uid));
        $result = $getUserInfo->fetchAll();

        foreach ($result as $value) {
            $this->username = $value['username'];
            $this->uid = $value['uid'];
            $this->date = $value['date'];
            $this->location = $value['location'];
        }
    }

    public function deleteUser($username){
        $deleteSpecifiedUser = $GLOBALS["databaseUserManagement"]->prepare("DELETE FROM users WHERE username = ?");
        $deleteSpecifiedUser->execute($user);
    }

    public function addUser($username, $password, $email, $ip){
        $cryptOptions = [
            'cost' => 11,
            'salt' => mcrypt_create_iv(22, MCRYPT_DEV_URANDOM),
        ];

        $passwordCrypted = password_hash($password, PASSWORD_BCRYPT, $cryptOptions);
        $email = $_POST['email'];
        $date = date("y-m-d h:m:s");

        $registerQuery = $GLOBALS["databaseUserManagement"]->prepare("INSERT INTO  `users` (`id` , `uid`, `username`, `password`, `date`,`ip`, `email`) VALUES (NULL,  ?, ?, ?,  ?,  ?, ?, ?)");
        $registerQuery->execute(array(uniqid(), $username, $passwordCrypted, $date, $ip, $email));
    }

}

No, this design is not object-oriented, nor would I recommend you follow your friend's advice. I also would not recommend your original design either. Neither one adheres very well to object-oriented design principles, namely the SOLID principles.

In your friend's rendition, he is proposing a structure that is very active record-like, where a domain object also knows how to persist and retrieve itself. While this pattern is used in many places (Ruby on Rails, CakePHP, etc), it can make testing very difficult, it can cause dependency problems, and it is not single responsibility.

It is difficult to test (especially your rendition) because the object is dependent on the database. Since you instantiate the database object in your method, there is no easy way to mock it or stub it out either.

Nor is there an easy way to substitute different implementations. Your class is tied to the concrete database implementation. It has a dependency that cannot be isolated.

The class has numerous responsibilities that are completely unrelated to representing a user. For example, in the getUser method, we are responsible for:

Getting a database connection object,
Building a query,
Running that query,
Fetching the results,
Translating the results into object properties.

This code has a lot of fingers in a lot of pies, and depends on the specific functionality of a lot of other parts.

Your proposal of simply having a data object, and a separate object to manage persistence is more on track. It falls down a bit in the details, though. To be truly object-oriented, you want to encapsulate your data. Having all public members does not encapsulate anything. The most basic way to make this more object-oriented is to have getters and setters (some may debate this, and there are other routes that can be taken, but this is a good start). This way, if the internal structure of your class changes, you can hide it by adjusting the implementations of the encapsulating methods while preserving the public API.

The pattern you are looking for is the Repository pattern. It allows you to isolate and encapsulate mapping between a persistence layer and domain objects, and allows the domain objects to focus on the single responsibility of representing the business concept they represent.

I would also recommend you learn about dependency injection to help you to understand why instantiating objects inline can cause problems, and what one potential solution is.


Refactoring while-do into tail recursion with F# I have a while-do loop nested inside a while-do loop in my program, due to it needing to iterate in two dimensions through an array of arrays, and originally my loop looked like this:

while ( tHeightIndex < tSearchHeight - 1 ) && tContinue do
    while ( tWidthIndex < tSearchWidth - 1 ) && tContinue do
        let tCurrentValue = tLargeArray.[tHeightIndex].[tWidthIndex]
        if tCurrentValue = tFirstSmallPixel then
            tMatch <- ArrayFunctions.SearchSubset tSmallArray tLargeArray ( tHeightIndex, tWidthIndex )
            if tMatch then tContinue <- false
        if tMatch = false && tContinue = true then
            tWidthIndex <- tWidthIndex + 1
    if tMatch = false && tContinue = true then
        tWidthIndex  <- 0                       // Reset to search next row
        tHeightIndex <- tHeightIndex + 1
tMatch, tWidthIndex, tHeightIndex

However, seeing as I'm doing this project entirely to learn better functional programming practices, I refactored that nested loop into two tail-recursive functions. This code passes my unit tests and appears to work correctly, so I would like advice about the style of the code and whether there's better ways I can make the code read clearly.

The relevant part to the original while-do loop is located after the declaration of firstSmallPixel.

module ImageSearch =
    open ImageFunctions

    let SearchBitmap (smallBitmap:Bitmap) (largeBitmap:Bitmap) =
        let smallArray = Transform2D <| LoadBitmapIntoArray smallBitmap
        let largeArray = Transform2D <| LoadBitmapIntoArray largeBitmap
        let searchWidth = largeBitmap.Width - smallBitmap.Width
        let searchHeight = largeBitmap.Height - smallBitmap.Height
        let firstSmallPixel = smallArray.[0].[0]

        let WidthLoop heightIndex =
            let rec WidthLoopRec heightIndex widthIndex =
                let ContinueLoop () = WidthLoopRec heightIndex (widthIndex + 1)
                let currentLargePixel = largeArray.[heightIndex].[widthIndex]
                match ( widthIndex < searchWidth , currentLargePixel = firstSmallPixel ) with
                | ( true , true  ) ->  let foundImage = ArrayFunctions.SearchSubset smallArray largeArray ( heightIndex, widthIndex )
                                       if foundImage then widthIndex , foundImage
                                       else ContinueLoop ()
                | ( true , false ) -> ContinueLoop ()
                | ( false, _     ) -> widthIndex , false
            WidthLoopRec heightIndex 0

        let HeightLoop () =
            let rec HeightLoopRec heightIndex =
                let widthIndex, foundImage = WidthLoop heightIndex
                match ( foundImage, heightIndex < searchHeight ) with
                | ( false , true  ) -> HeightLoopRec ( heightIndex + 1 )
                | ( _     , _     ) -> foundImage , widthIndex , heightIndex
            HeightLoopRec 0

        HeightLoop ()

I think that learning functional programming should be about making your code more readable (and also better in other aspects), neither of your samples seems very readable to me.

I also think that when working with collections in functional programming, the most important concept is not recursion, it's higher-order functions.

If you use those in the form of a query expression, your code could look like this:

query {
    for row in 0..searchHeight do
    for col in 0..searchWidth do
    let pixel = largeArray.[row].[col]
    where (pixel = firstSmallPixel)
    select (row, col, pixel)
    head
}

The nice thing about this code is not only that it's more readable and shorter, it's that it emphasizes what you want to do, not how to do it.


Binary Search For Data Types With Relation Operators I am trying to write a binary search algorithm to work with any data type that is defined to work with the relational operators and returns the position of the found element. I want to know if there is anything I need to be aware of when dealing with user defined types. As well as any flaws or improvements that I can make. Also one other question is how does my search compare to the built in binary_search function?

#include <iostream>
#include <iomanip>
#include <type_traits>

template<typename T, typename U>
U BinarySearch(T *arr, T toFind, U _size){ //make return type same as size for overflow issues
    static_assert(std::is_integral<U>::value, "Integral type required for size of array");

    U mid, low = 0, high = _size-1; //maintain same integral types
    T m, l = arr[low], h = arr[high]; //maintain same T types

    while(l<=toFind && h>=toFind){
        mid = (high + low)/2;
        m = arr[mid];
        if(m > toFind){
            h = arr[high = mid - 1];
        }
        else if(m < toFind){
            l = arr[low = mid + 1];
        }
        else
            return mid; //postion point + 1
    }

    //exits if not found
    return -1;
}

const auto SIZE = 10;

int main()
{
    double arr[SIZE] = {0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 1.1};
    auto pos = BinarySearch(arr, 1.1, SIZE);
    std::cout << pos << '\n';
    return 0;
}

Thanks for all your advice in advance.

Apart from the fact that you should prefer writing algorithms to work with iterators (to enable you to sort things that aren't arrays, such as std::lists), there are a few things here that are problematic:

Don't make assumptions about the signedness of your U type, which seems to represent the size type of the array. Oftentimes, U will be unsigned (e.g. because it is std::size_t), which means returning -1 on not finding a value might be unexpected behaviour for your users.
Check that _size is greater than zero. If it is not, your program invokes undefined behaviour.
Take toFind by const reference since you don't know which type it will be, whether it will be trivially copyable and so on and so forth. The same is true for m, l and h; you should either make them pointers to elements in the array or make them store the index to their respective elements instead.
Be careful with overflow. In particular, (high + low)/2; may overflow the passed size type and you may end up getting nowhere. Instead, divide them separately and add the results together.

These are the pitfalls and dangers I spotted. There is a lot more to say about your code, however, but I will leave this to somebody else.


C++ - algebraic expressions calculator by addition I'm ultra new to programming. I basically know only IFs, loops, variables. This is first time I used functions in my code and I'm not sure if I understand them correctly.

Basically it is a calculator that requests two natural positive numbers on input, and outputs it's addition, subtraction, multiplication, division and exponentiation. The only rule is: you can use only addition when calculating these expressions. I'm ultra new to programming. I basically know only IFs, loops, variables. This is first time I usentiation. It does not yet support: using '0' as input, using negative numbers as input, using floating point numbers as input, indivisible numbers.

My question is: can you please take a look at my code and just tell me if I made any mistakes that literally burn your eyes? What should I fix? What is especially bad?

#include <iostream>
using namespace std;

int addition(int a, int b)
{
    return a + b;
}

int subtraction(int a, int b)
{
    return a + (-b);
}

int multiplication(int a, int b)
{
    int result = a;
    for (int i = 1; i < b; i++)
        result += a;
    return result;
}

int division(int a, int b)
{
    int counter = 0;
    if (a % b == 0) {
        do {
            a = subtraction(a, b);
            counter++;
        } while (a > 0);
        return counter;
    }
    else
        return false;
}

int exponentiation(int a, int b)
{
    int result = 1;
    for (int i = 1; i <= b; i++)
        result = multiplication(result, a);
    return result;
}

int main()
{
    int numberA, numberB;
    cout << "Insert two positive natural numbers: ";
    cin >> numberA >> numberB;
    cout << "\nAddition: " << addition(numberA, numberB) << '\n';
    cout << "Subtraction: " << subtraction(numberA, numberB) << '\n';
    cout << "Multiplication: " << multiplication(numberA, numberB) << '\n';
    cout << "Division: " << division(numberA, numberB) << '\n';
    cout << "Exponentiation: " << exponentiation(numberA, numberB) << '\n';
    return 0;
}

Namespaces

You almost never want to write using namespace std; - this is a very common code complaint on this site with C++. The two main reasons are:

it's good to know when you're using stuff from the standard library explicitly,
standard library names can collide with your own since they've picked the best names!

auto

Consider also using return type deduction here as your functions are fairly trivial, and the following would work:

auto addition(int a, int b) { return a + b; }

The compiler has no trouble working out the return type is clearly an integer, and this will apply to other cases. Another use of auto is in your copy in the multiplication function, for example,

auto multiplication(int a, int b){
    auto result = a;
    // rest of code
}

You can apply this to a lot of other statements in your code. Depending on your style preference and the importance you place on uniformity, you can use the uniform initialisation syntax, auto result{a}.

Division

There's a problem in your division function. It returns counter which is an integer, and the return type is an integer - that's fine. But it can return false.

This compiles, and returning false will be like returning zero, but just do that explicitly, your return type isn't bool.

I would also move the declaration of the counter variable to inside the if statement, since if that statement fails, you don't need it.

So, if the integers don't exactly divide each other, you don't want to handle that case and you return zero. But you don't check this when outputting to the user. They'll get zero, so I'd opt for an explicit message, since that can have multiple interpretations.

In C++, return 0 is not required explicitly in main, so omit it. There's also an argument against it along the lines of, if you do include it, it's suggesting main can fail and that there are potentially other error codes that one must check for.


Spaces-to-tabs converter, updated version I recently posted Spaces To Tabs Converter but it had lots of bugs, so I rewrote the code. I wanted to ask, is it good code? I didn't find any bugs.

#include <stdio.h>
#define TABVALUE 4
#define ARRAYSIZE 3

void emptyArray(int *a);

int main(void) {
    int c, d, s;
    int savedChars[ARRAYSIZE];
    c = s = d = 0;
    while ((c = getchar()) != EOF) {
        if (c == ' ') {
            s = 0;
            s++;
            for (int j = 0; j < TABVALUE - 1; j++) {
                d = getchar();
                if (d != EOF) {
                    savedChars[j] = d;
                    savedChars[j + 1] = '\0';
                    if (d == ' ') {
                        s++;
                    } else {
                        break;
                    }
                }
            }
            if (s == TABVALUE) {
                emptyArray(savedChars);
                putchar('\t');
                s = 0;
            } else {
                putchar(c);
                for (int i = 0; i < 3; i++) {
                    if (savedChars[i] != '\0') {
                        putchar(savedChars[i]);
                    }
                }
            }
        } else {
            if (c != EOF) putchar(c);
        }
    }
    return 0;
}

void emptyArray(int *a) {
    for (int i = 0; i < ARRAYSIZE; i++) {
        a[i] = '\0';
    }
}

A chunk of memory that is used to temporarily queue up content before it is eventually flushed is typically called a buffer.  A typical variable name would be buffer or buf.

You defined constants TABVALUE and ARRAYSIZE.  It looks like ARRAYSIZE is dependent on TABVALUE, so you shouldn't hard-code both 4 and 3.  You also shouldn't hard-code 3 in the for loop.

The program outline is basically

while ((c = getchar()) != EOF) {
    if (c == ' ') {
        /* Huge chunk of buffer manipulation code here... */
    } else {
        if (c != EOF) putchar(c);
    }
}

You've already ascertained that c is not EOF in the while loop, so if (c != EOF) is pointless.  I would also recommend inverting the test so that there isn't a huge chunk of code before the else.  This outline is equivalent to your logic:

while ((c = getchar()) != EOF) {
    if (c != ' ') {
        putchar(c);
    } else {
        /* Huge chunk of buffer manipulation code here... */
    }
}

The first space encountered gets stored as c, but subsequent spaces get written to the savedChars buffer.  The code would be simpler if you just wrote the first space to the buffer too.

I'm not convinced that you need a savedChars buffer, though.  The buffer will only contain a string of spaces, possibly followed by a non-space character at the end.  You should be able to do the job using just the s count and the final character d.


Finding prime numbers in a range Finding the prime numbers between two given integers in the minimum number of comparisons:

#include <stdio.h>
#include <conio.h>
#include <math.h>

void main()
{
    clrscr();
    int i,n,count,n1,n2;
    printf("Enter n1 and n2:");
    scanf("%d",&n1);
    scanf("%d",&n2);
    n=n1;
    int c=0; //variable to store number of comparisions (number of times while() runs)
    for(n=n1;n<=n2;n1++,n=n1)
    {
        count=0; //number of factors found
        i=2;
        while((n>0)&&(i<=sqrt(n))) //condition for minimum comparisions
        {
            printf("\n\t%d",c++);//printing the comparision number
            if(n%i==0)
            {
                count++;
                n/=i;
            }
            else i++;
        }
        if(count==0)
            printf("\n%d",n1);
    }
    getch();
}

I have found some things that might help you improve your code:

Strive for portable code

Several of the features of this code are either platform-specific or compiler-specific or both.  Specifically, #include <conio.h>, clrscr(), and getch() are all non-standard.  Instead consider portable replacements.  For instance you could use this instead of clrscr():

putchar('\f');

Or possibly this:

for (int lines=40; lines; --lines)
    putchar('\n');

Alternatively, one could eliminate it entirely, which is what I've done in my modified version of your code.

Follow standard declaration of main

Your compiler may allow you to declare void main() but that construction is technically only valid for what are called "non-hosted" environments, e.g. one in which you are not running Windows.  For that reason, you should use the standard int main() instead.  Note that you don't need to explicitly provide a return 0; at the end of main -- it's created implicitly by the compiler.

Whitespace improves readability

Allofyourwordsarecrammedtogether.  Inserting some spaces in your lines will make them much easier to read.  For example, instead of this:

while((n>0)&&(i<=sqrt(n)))

try this:

while((n > 0) && (i <= sqrt(n)))

Avoid scanf if you can

There are so many well known problems with scanf that you're usually better off avoiding it.  The usual approach is to read in user input into a string and then parse from there, in this case using something like atoi().

Eliminate redundant code

In this case, the line n=n1; is redundant since the initializer of the for loop does that again just two lines later.

Consider reducing the scope of variables

The variable n is only used within the for loop, so you could declare it within the initializer:

for(int n=n1; n <= n2; n1++, n=n1)

Use a for loop instead of while

Because the i variable is used only within the while loop, consider again reducing scope and turning that into a for loop:

for (int i=2;(n>0) && (i <= sqrt(n)); )

Even better, in that loop, the only use of n is for testing primes, so within it you can instead use a variable local to that loop.  I've used t (for testing):

for (int i=2, t=n; (t>0) && (i <= sqrt(t)); )
{
    printf("\t%d\n",c++);//printing the comparision number
    if(t%i==0)
    {
        count++;
        t/=i;
    }
    else i++;
}

This change also allows you to remove n1 entirely and to simply use n everywhere that n1 had been used.

Improve your algorithm

Except for 2, all prime numbers are odd.  You can take advantage of that to reduce the number of comparisons:

for (int i=2, t=n; (t>0) && (i <= sqrt(t)); )
{
    printf("\t%d\n",c++);//printing the comparision number
    if(t%i==0)
    {
        count++;
        t/=i;     // better:  break;
    }
    else if (i==2)
    {
        ++i;
    } else
    {
        i += 2;
    }
}

However, once we discover that a number is not prime, why continue factoring it?  You can eliminate even more comparisons by replacing the line t/=i with break to break out of the enclosing for loop.

You can do a similar optimization on the outer loop, incrementing it by 2 each loop iteration once you assure that it is odd coming into the loop.

Print newlines after rather than before

The code currently has printf("\n%d", n1); but the effect is that the last line printed doesn't actually end with \n.  Instead, re-arrange the two printf statements to that they both end rather than begin with \n.

Results:

When I applied all of these to your code, I found that when I compute the primes from 500 to 1000, the original code made 6766 comparisons, while the optimized version made 1444.


Getting a JObject by a property value I am building a contact management system. Each contact can relate to the business in one of three ways: client, service provider, and third party.

I have a JSON file containing the relation types and their importance to the business (so I could priorities messages, for example):

{
    "Relations": [
        {
            "Name": "Client",
            "Importance": 1
        },
        {
            "Name": "Service Provider",
            "Importance": 2
        },
        {
            "Name": "ThirdParty",
            "Importance": 3
        }
    ]
}

I have a method that gets the name of the relation and retrieves a JObject containing the JSON representing this relation, using Json.NET:

private JObject GetRelationJSON(string relationName)
{
    string jString = File.ReadAllText("relations.json");
    JObject relationsJSON = JsonConvert.DeserializeObject<JObject>(jString);

    // I am specifically not sure about this line
    return (JObject)(relationsJSON["Relations"].Where(r => ((string)r["Name"]).Equals(relationName)).ToList()[0]);
}

Note that I am not allowed to deserialize the JSON into POCO classes.

How can I improve this code? Thanks!

Not being able to use POCO is tough enough so don't make your life even harder with the line you're asking about. It's much easier to query it with JsonPath like that:

return (JObject)relationsJSON.SelectToken($"$.Relations[?(@.Name=='{relationName}')]");

and since you are not using POCO then you don't have to deserialize it. It's ok when you just let json.net parse it with JToken and then do the JsonPath thing:

JToken.Parse(json).SelectToken(...);


Producing every string of a given length using some range of characters Ever since I was a kid I wanted to understand how brute force programs work. I have not been able to make sense of any of the examples throughout the internet, so it became a personal challenge to make my own. Now that I have a couple Java classes under my belt, I realized I could tackle this problem with a linked list. Any advice about best practices, better algorithms, or how to make the existing one faster are welcome.

The problem to solve is to produce every possible combination of characters for a given range of characters and a given string length. This is purely for the purpose of personal learning.

public class BruteForce {

    final int minASCII = 97;
    final int maxASCII = 122;
    final int stringLength = 4;

    CharNode firstNode = new CharNode(minASCII);

    public BruteForce() {

        CharNode tempNode = firstNode;
        //Create stringLength number of CharNodes as a doubly linked list
        //The last node is used as an end of string marker
        for(int i = 0; i < stringLength; i++) {
            CharNode nextNode = new CharNode(minASCII - 1);
            tempNode.setNextNode(nextNode);
            nextNode.setPreviousNode(tempNode);
            tempNode = nextNode;

            //Link the last node with the first node, and the first node with
            //the last node to make it a circular linked list
            if (i == stringLength - 1) {
                nextNode.setNextNode(firstNode);
                firstNode.setPreviousNode(nextNode);
            }
        }
        //keep printing and incrementing the nodes until the last
        //marker node is incremented
        while (firstNode.getPreviousNode().getData() == (minASCII - 1)) {
            printData();
            firstNode.incrementData();
        }
    }

    private void printData() {

        CharNode tempNode = firstNode;

        while (tempNode.getData() != (minASCII -1)){
            System.out.print((char)tempNode.getData());
            tempNode = tempNode.getNextNode();
        }
        System.out.println();
    }

    /*
     * One CharNode represents one character. Each CharNode links to
     * the next CharNode and to the previous CharNode. Based on the
     * circular doubly linked list data structure.
     */
    private class CharNode {

        private int data = 0;
        private CharNode nextNode = null;
        private CharNode previousNode = null;

        public CharNode(int data) {
            this.data = data;
        }

        public void setData(int data) {
            this.data = data;
        }

        public int getData() {
            return data;
        }

        public void setNextNode(CharNode nextNode) {
            this.nextNode = nextNode;
        }

        public CharNode getNextNode() {
            return nextNode;
        }

        //Automatically increment the next node and reset data to minASCII
        //once maxASCII is reached
        public void incrementData() {
            if (data >= maxASCII) {
                this.getNextNode().incrementData();
                data = minASCII;
            }
            else
                data++;
        }

        public CharNode getPreviousNode() {
            return previousNode;
        }

        public void setPreviousNode(CharNode previousNode) {
            this.previousNode = previousNode;
        }
    }

    public static void main(String[] args) {
        new BruteForce();
    }
}

There is not much to understand about "brute forcing" something. "Brute force" does not mean a particular algorithm.

"Brute force" simply means that instead of using a clever algorithm to simplify a problem (simplify here means reducing the running time) you just test every single possible input to see which one produces the right result.

For example in password cracking, "brute force" means calculating every single possible password and testing them one by one. A smart algorithm would try passwords that are based on names and such first, because we know those are used way too often.

So, first of all, you need to clarify the problem that you are trying to solve through brute force.


Reversing second half of linked list I recently had an interview where I was asked to reverse the second half of a linked list. The driver program was unknown to me, I just needed to write the method to do this. The method was supposed to behave as follows:

1 -> 2 -> 3 -> 4 becomes 1 -> 2 -> 4 -> 3
1 -> 2 -> 3 -> 4 -> 5 becomes 1 -> 2 -> 5 -> 4 -> 3

The function signature was as follows:

public static void reverseList(ListNode node)

I don't remember verbatim the code I submitted, but I wrote something up that follows very closely to what I submitted. The code follows:

public static void reverseList(ListNode node) {
    if(node == null) return;
    int size = size(node);
    if(size == 1) return;
    int i = 0;
    ListNode middleOfList = null;
    ListNode currentNode = node;
    //traverse the first half of the list
    while(i < size/2) {
        middleOfList = currentNode;
        currentNode = currentNode.getNext();
        i++;
    }
    ListNode previous = null;
    //reverse the second half of the list
    while(currentNode != null) {
        ListNode next = currentNode.getNext();
        currentNode.setNext(previous);
        previous = currentNode;
        currentNode = next;
    }
    //reattach first and second halves of the list
    middleOfList.setNext(previous);
}

public static int size(ListNode node) {
    ListNode current = node;
    int count = 0;
    while(current != null) {
        count++;
        current = current.getNext();
    }
    return count;
}

While I don't think this is a terrible approach for an internship interview, I'm nervous because the solution seems to be \$O(n^2)\$ since I first traverse the entire list to get the size and then traverse it again to do the actual reversal.

Is there a more efficient solution? Any general feedback on style is also appreciated.

Complexity analysis

  the solution seems to be O(n^2) since I first traverse the entire list to get the size and then traverse it again to do the actual reversal.

That's not \$\mathcal{O}(n^2)\$.  That's \$\mathcal{O}(n)\$, because \$n + n\$ is still \$\mathcal{O}(n)\$.  It would be quadratic time if you traversed the entire list \$n\$ times because \$n \cdot n\$ is \$\mathcal{O}(n^2)\$.  But you only do so twice.  For complexity analysis, twice is no different from once.  We only care about things that vary with the input size.

If you said this to them, this is a potential interview fail.

Using for loops for iteration

public static int size(ListNode node) {
    ListNode current = node;
    int count = 0;
    while(current != null) {
        count++;
        current = current.getNext();
    }
    return count;
}

It doesn't make any functional difference, but consider

public static int size(ListNode node) {
    int count = 0;
    for (ListNode current = node; current != null; current = current.getNext()) {
        count++;
    }

    return count;
}

Now we can easily see that for every node in the list, we want to increment count.  It even gets the scope right.  The count can be seen outside the loop, but current can't.  We don't need it after the loop, and it is gone.  We do need count, and it is still there to be returned.

This puts all the loop definition in one line.  The original spreads it throughout the method.

I also added a bit of extra whitespace for readability.

Know your exit conditions

    if(node == null) return;
    int size = size(node);
    if(size == 1) return;

You can write this more briefly as

int size = size(node);
if (size < 2 * 2) {
    return;
}

You don't need to check if node is null, as the size check will handle that for you.  You also don't to check lists of size 2 or 3.  Such lists won't be any different, as the second half of the list is only one element long.

I prefer to always use the block form of control structures rather than the statement form.  Some prefer the way that you did it.  This is something that could potentially fail an interview, so try to determine their attitude before picking a style.

I would put a blank line after this section to separate it from the rest of the method.

Don't duplicate work

    ListNode middleOfList = null;
    ListNode currentNode = node;
    //traverse the first half of the list
    while(i < size/2) {
        middleOfList = currentNode;
        currentNode = currentNode.getNext();
        i++;
    }

Consider

    ListNode middle = null;
    for (size /= 2; size > 0; size--) {
        middle = middle.getNext();
    }

    ListNode current = middle.getNext();

Now, instead of updating two variables each time, we update one.  Then we set the second once, after the loop is done.

We get rid of i altogether.  But at the cost of a misleading value for size.  Perhaps replace size with i or remaining or similar.

A reverse method

    middleOfList.setNext(previous);

Consider what you would have to do to replace that with

    middle.setNext(reverse(middle.getNext()));

Then you'd have another general purpose algorithm like size that could be reused elsewhere.

Evaluating

I would consider this a reasonable solution.  Perhaps you could make slight performance tweaks, but you generally shouldn't need to do so.  You have to traverse the list at least once, so it's going to be linear time.  Your version is still linear time.  Your size method is well contained and reusable.

You are not thread safe (consider what happens if another thread cuts the list by two thirds after you get the size but before you find the middle), but it's not evident that you need to be.

Other than that and the two potential failing issues, my only other comment is that I'd prefer more paragraphing in the code.  Separate the method into code blocks with blank lines between.

I personally would put a space between control structures like if and while and the following open parenthesis.  But in interviewing, try to determine their style and match it with your code.  If they do it the way that I do, then match them.  If they do it the way that you do, then just do it how you normally would.


Clone Vector in rust except at one index Let's consider you want to clone all items of a vector orig except the item of index i should be taken from alternative value alt.

Lets say the items are of the type

struct Bla{
  // Some fields
}

Using for loop

On simple solution would be:

fn clone(orig: &Vec<Bla>, i: usize, alt: Bla) -> Vec<Bla> {
  let cloned = Vec::new();
  for (j, item) in orig.iter().enumerate() {
    let new_item = if i != j {item.clone()} else {alt};
    cloned.push(new_item);
  }

  cloned
}

  Note: Does not compile, because the compiler does not know that i==j is only valid once.

Using functional style

The functional alternative would be the follow:

fn clone(orig: &Vec<Bla>, i: usize, alt: Bla) -> Vec<Bla> {
  orig.iter().enumerate()
    .map(|(j, item)| if i != j {item.clone()} else {alt})
    .collect::<Vec<Bla>>()
}

The last one looks nice, but does not compile :( because the rust compiler does not know that alt gets moved (or consumed) only once (i==j).

Using mem replace

In order to fix the version above one can use a default value for the index which should be replaced.

fn clone(orig: &Vec<Bla>, i: usize, alt: Bla) -> Vec<Bla> {
  let mut result = orig.iter().enumerate()
    .map(|(j, item)| if i != j {item.clone()} else {Bla::default()})
    .collect::<Vec<Bla>>();

  mem::replace(&mut result[i], alt);
  result
}

What is the most concise and fastest implementation to do so?

In my opinion none of your solution is idiomatic and all use an useless comparaison.

Using for loop:

Doesn't pre-allocate the vector
Use a comparaison that could be avoid

Using functional style:

Use a comparaison that could be avoid

Using mem replace:

Use a comparaison that could be avoid
Is unnecessary complex to just copy a vector

Also, you should avoid take a &Vec<T> as parameter, it's much better to take a &[T]. See: Why is it discouraged to accept a reference to a String (&String), Vec (&Vec), or Box (&Box) as a function argument?

I would use one of the following version:

#[derive(Clone)]
struct Pokemon {
}

fn a(pokemons: &[Pokemon], i: usize, pokemon: Pokemon) -> Vec<Pokemon> {
    let mut digimons = Vec::with_capacity(pokemons.len());

    digimons.extend_from_slice(&pokemons[0..i]);
    digimons.push(pokemon);
    digimons.extend_from_slice(&pokemons[i + 1..]);

    digimons
}

fn b(pokemons: &[Pokemon], i: usize, pokemon: Pokemon) -> Vec<Pokemon> {
    let mut digimons = pokemons.to_vec();
    digimons[i] = pokemon;

    digimons
}

fn c(pokemons: &[Pokemon], i: usize, pokemon: Pokemon) -> Vec<Pokemon> {
    pokemons[0..i]
        .iter()
        .cloned()
        .chain(std::iter::once(pokemon))
        .chain(pokemons[i + 1..].iter().cloned())
        .collect()
}

I don't really know witch version is better, a() and c() should be the same but sometime chain() is not optimized away. And b() is probably the most straightforward solution. I think I would choice a() in a production code.

However, looking at the assembly, we can see that a() and c() doesn't produce the same, and that b() is the simplest.


For each element in a list, finding the element in a second list, that minimizes a distance function Getting started:

For a given number x (say -5) I want to know which element of a list y (say [2, -6]), minimizes the value of distance function d (say the absolute value) which applied on x and all elements of y. My current solution to this problem is

x = -5
y = [2, -6]
def d(a1, a2):
    return abs(a1-a2) # more complicated in real application

min(y, key = lambda _: d(_, x)) # correctly returns -6

Next level: x is actually a list (say [1, 5, -10]), so I build a list comprehension on top my previous solution:

x = [1, 5, -10]
[min(y, key = lambda _: d(_, xindex)) for xindex in x] # correctly returns [2, 2, -6]

As this type of problem seems quite basic to me, I expect a simple solution for this problem (like argmin(list1=x, list2=y, function=d)). Here is a question where it is shown how a custom distance matrix can be build. But this alone (the step from the distance matrix to the argmins would also have to be done) seems too be more complicated than my code.

Overall my question is: is my code fine, or is there a nicer solution?

The approach is fine and this is exactly the purpose of the key attribute of min.

I’d just nitpick that the _ variable name is usually meant to indicate unused arguments and thus, using it inside the lambda goes against conventions.

Alternatively, you can make use of functools.partial which may yield better performances on larger lists:

[min(y, key=partial(d, a2=xindex)) for xindex in x]


Tonelli-Shanks algorithm implementation of prime modular square root I did an implementation of the Tonelli-Shanks algorithm as defined on Wikipedia. I put it here for review and sharing purpose.

Legendre Symbol implementation:

def legendre_symbol(a, p):
    """
    Legendre symbol
    Define if a is a quadratic residue modulo odd prime
    http://en.wikipedia.org/wiki/Legendre_symbol
    """
    ls = pow(a, (p - 1)/2, p)
    if ls == p - 1:
        return -1
    return ls

Prime modular square root (I just renamed the solution variable R to x and n to a):

def prime_mod_sqrt(a, p):
    """
    Square root modulo prime number
    Solve the equation
        x^2 = a mod p
    and return list of x solution
    http://en.wikipedia.org/wiki/Tonelli-Shanks_algorithm
    """
    a %= p

    # Simple case
    if a == 0:
        return [0]
    if p == 2:
        return [a]

    # Check solution existence on odd prime
    if legendre_symbol(a, p) != 1:
        return []

    # Simple case
    if p % 4 == 3:
        x = pow(a, (p + 1)/4, p)
        return [x, p-x]

    # Factor p-1 on the form q * 2^s (with Q odd)
    q, s = p - 1, 0
    while q % 2 == 0:
        s += 1
        q //= 2

    # Select a z which is a quadratic non resudue modulo p
    z = 1
    while legendre_symbol(z, p) != -1:
        z += 1
    c = pow(z, q, p)

    # Search for a solution
    x = pow(a, (q + 1)/2, p)
    t = pow(a, q, p)
    m = s
    while t != 1:
        # Find the lowest i such that t^(2^i) = 1
        i, e = 0, 2
        for i in xrange(1, m):
            if pow(t, e, p) == 1:
                break
            e *= 2

        # Update next value to iterate
        b = pow(c, 2**(m - i - 1), p)
        x = (x * b) % p
        t = (t * b * b) % p
        c = (b * b) % p
        m = i

    return [x, p-x]

If you have any optimization or have found any error, please report it.

Good job! I don't have a lot to comment on in this code. You have written straightforward clear code whose only complexity stems directly from the complexity of the operation it is performing. It would be good to include some of your external commentary (such as the renames of R and n) in the code itself to make it easier for someone to follow the documentation on wikipedia. You may want to include some of that documentation as well.

For reference, the rest of this review assumes that the code functions correctly; I don't have my math head on tonight.

There appears to be one case of redundant code, unless m can ever be 1, resulting in an empty range and thus no reassignment of i. Otherwise you can skip the assignment to i in the following:

i, e = 0, 2
for i in xrange(1, m):
    ...

There are a number of small strength-reduction optimizations you might consider, but in Python their impact is likely to be minimized - definitely profile before heading too deeply down the optimization path. For example in the following while loop:

# Factor p-1 on the form q * 2^s (with Q odd)
q, s = p - 1, 0
while q % 2 == 0:
    s += 1
    q //= 2

Both operations on q can be reduced. The modulus can be rewritten as a binary and q & 1, and the division as a binary shift q >>= 1. Alternately, you can use divmod to perform both operations at once.

Similarly, 2**(m - i - 1) is identical to 1 << (m - i - 1) for non-negative exponents.


VBA code to read Excel file and conditionally complete formulas based on the contents The code below does the following:

Look at rows starting at row 10 and continuing through the last row of a sheet, and based off certain criteria, loops through each row.
If the scenarios match, then the code will drop in 0s and 1s onto another sheet in a formulated range
In some scenarios, the code will fill down formulas.

The correct time to run is 3.5 seconds, I am wondering if there are any adjustments to speed this up.

Option Explicit

Sub CleanupCrew()
Dim ws As Worksheet
Dim ws1 As Worksheet
Dim ws2 As Worksheet
Dim ws3 As Worksheet
Dim icounter As Long
Dim lastrowB As Long
Dim lastrowd As Long
Dim a As Variant
Dim rw As Long

Set ws = Worksheets("Dashboard")
Set ws1 = Worksheets("Base")
Set ws2 = Worksheets("LOOKUP")
Set ws3 = Worksheets("Control")

Application.ScreenUpdating = False
Application.EnableEvents = False
Application.DisplayAlerts = False
Application.Calculation = xlCalculationManual

lastrowB = ws1.Cells(Rows.Count, 1).End(xlUp).Row
lastrowd = ws.Cells(Rows.Count, 1).End(xlUp).Row

Application.Calculate
For icounter = 10 To lastrowd
Dim varL As Integer
Dim varM As Variant

varL = ws.Cells(icounter, "AD")
varM = ws.Cells(icounter, "AI")
a = ws.Cells(icounter, "AD")

'If proposed Box vacancy greater than 12
If ws.Cells(icounter, "AN") > 12 And ws.Cells(icounter, "AX") = "True" Then

rw = ws.Cells(ws.Rows.Count, "A").End(xlUp).Row + 1
ws1.Cells(varM, a).Value = 1 - ws.Cells(icounter, "AE").Value
If a = 87 Then
ws1.Range(ws1.Cells(varM, a + 1), ws1.Cells(varM, 98)).Value = 0
ElseIf a = 98 Then
ws1.Range(ws1.Cells(varM, 87), ws1.Cells(varM, a - 1)).Value = 1
Else
ws1.Range(ws1.Cells(varM, 87), ws1.Cells(varM, a - 1)).Value = 1
ws1.Range(ws1.Cells(varM, a + 1), ws1.Cells(varM, 98)).Value = 0
End If

ws.Cells(rw, "A") = "Incumbent (Automated)"
ws.Cells(rw, "E").Value = ws.Cells(icounter, "N").Value
ws.Cells(rw, "B").Value = ws.Cells(icounter, "B").Value
ws.Cells(rw, "D").Value = Application.WorksheetFunction.VLookup(ws.Cells(rw, "E"), ws3.Range("C:I"), 7, 0)
ws.Cells(rw, "A").AddComment " " & ws.Cells(rw, "D") & ". "
ws.Range("Y" & lastrowd & ":AS" & rw).FillDown
End If

If ws.Cells(icounter, "AM") > 12 And ws.Cells(icounter, "AY") = "True" Then

rw = ws.Cells(ws.Rows.Count, "A").End(xlUp).Row + 1
ws1.Cells(varL, a).Value = 1 - ws.Cells(icounter, "Ae").Value
If a = 87 Then
ws1.Range(ws1.Cells(varL, a + 1), ws1.Cells(varL, 98)).Value = 0
ElseIf a = 98 Then
ws1.Range(ws1.Cells(varL, 87), ws1.Cells(varL, a - 1)).Value = 1
Else
ws1.Range(ws1.Cells(varL, 87), ws1.Cells(varL, a - 1)).Value = 1
ws1.Range(ws1.Cells(varL, a + 1), ws1.Cells(varL, 98)).Value = 0
End If

ws.Cells(rw, "A") = ")"
ws.Cells(rw, "E").Value = ws.Cells(icounter, "E").Value
ws.Cells(rw, "B").Value = ws.Cells(icounter, "B").Value
ws.Cells(rw, "D").Value = Application.WorksheetFunction.VLookup(ws.Cells(rw, "E"), ws3.Range("C:I"), 7, 0)
ws.Cells(rw, "A").AddComment "" & ws.Cells(rw, "D") & ". ."
ws.Range("Y" & lastrowd & ":AS" & rw).FillDown
End If

If ws.Cells(icounter, "AS") = True And ws.Cells(icounter, "A") <> "Termination" And ws.Cells(icounter, "Aw") = "True" Then

rw = ws.Cells(ws.Rows.Count, "A").End(xlUp).Row + 1

ws.Cells(rw, "A") = ""
ws.Cells(rw, "E").Value = ws.Cells(icounter, "E").Value
ws.Cells(rw, "B").Value = ws.Cells(icounter, "B").Value
'ws.Cells(rw, "D") = Application.WorksheetFunction.VLookup(ws.Cells(rw, "E"), ws3.Range("C:I"), 7, 0)
ws.Cells(rw, "A").AddComment " " & ws.Cells(rw, "E") & " "

ws.Range("Y" & lastrowd & ":AS" & rw).FillDown
End If

Next icounter
Application.ScreenUpdating = True
Application.EnableEvents = True
Application.DisplayAlerts = True

Application.Calculate

End Sub

Some tips I suggest trying...

VarM and a shouldn’t be variants. I would change those to integers.

Overly abundant on counting loops, in this scenario you should loop a range through a named range.

Also use the .offset(1,0) instead of  adding 1 for rw. Use .offset(0,1) instead of reading the cells(lastrow,”b”). Set application.calculation back to automatic at the end (I wouldn’t turn it off but depends on your data rows). Use with end with statements, it’ll help make the code more readable as well.

You have a lot of if else end if blocks. I would break that into a select case statement when a criteria is met for your formulas. I’m on my phone but I hope this makes enough sense for you to try. This is what the structure would look similar to:

with ws
for each cell in MyRNG

‘Select case for cell criteria
‘if statement here for formula criteria
‘End if
‘Next case statement here...

Next cell
End with


Retrying a web request in Python Is it fine to retry a web service call like this in Python? Would there be any scenarios the exception might not work the way we expect it? I expect it to continue only if it's fine, otherwise just retry, and if that doesn't work, just throw the error out.

def getblob(html):
    num_of_retries = 2
    time_interval = 2
    while num_of_retries:
        try:
            request_id = str(uuid.uuid4())
            params = [('requestId', request_id)]
            url = 'service.com?'
            url = url + urllib.urlencode(params)
            request = urllib2.Request(url, data=html)
            request.add_header('Accept', 'application/pdf')
            request.add_header('Content-Type', 'text/html')
            handle = urllib2.urlopen(request)
            pdf_blob = handle.read()
            break
        except Exception:
            typ, val, tb = sys.exc_info()
            logger.error(traceback.format_exception(typ, val, tb))
            time.sleep(time_interval)
            num_of_retries -= 1
            # If there aren't any retries - propogate
            if not num_of_retries:
                    raise
    return pdb_blob

I wnated to review this because recently I bumped into an issue where I hit an Internal Server Exception on the server side and the retry gave me an HTTP 200 response with incomplete file (it was a PDF without EOF Marker).

Would this snippet cause anything like this? It does work and I get PDF back as well. However from time to time I hit an HTTP 500 (some unknown server issue) and then HTTP 200 with incomplete blob. The incomplete blob is what makes me investigate if this loop can cause that problem. This HTTP 500 HTTP 200 (Incomplete Blob) is very much a pattern. Before I added this retry, I used to get HTTP 500 but never an incomplete blob.

Instead of using while to count a specific number of times and manually handle the number, you should use for _ in range(num_of_retries). It works the same except that you don't need to manually change num_of_retries. And this way num_of_retries wont be modified from the initial value, in case you need to access it again for any reason.

There is one small change to make this work. You need to use the for ... else syntax to raise if the loop is not broken. An else block after a for block will be executed if the for loop never reaches a break statement. This fits your case perfectly as break is called on success, but if there's never success then it will run through all the iterations and reach the else block where you can raise:

for _ in range(num_of_retries):
    try:
        request_id = str(uuid.uuid4())
        params = [('requestId', request_id)]
        url = 'service.com'
        url = url + urllib.urlencode(params)
        request = urllib2.Request(url, data=html)
        request.add_header('Accept', 'application/pdf')
        request.add_header('Content-Type', 'text/html')
        handle = urllib2.urlopen(request)
        pdf_blob = handle.read()
        break
    except Exception:
        typ, val, tb = sys.exc_info()
        logger.error(traceback.format_exception(typ, val, tb))
        time.sleep(time_interval)
else:
    raise

Without any explicit argument, raise will just use the last exception that has been raised and handled. See the SO answer here about it. Basically when you try except an exception, its details are being retained by Python and are passed to raise in the absence of any explicit exception passed to raise.


Password encrypt & check When a user registers (creates a new account), I want to encrypt password before storing in a database, and when a user logs on (with username & password) I want to check password. I did it in the following way:

function password_encrypt($pass) {
    $hash_format = "$2y$10$"; // Tells PHP to use Blowfish with a "cost" of a 10
    $salt_length = 22; // Blowfish salts should be 22-characters or more
    $salt = generate_salt($salt_length);
    $format_and_salt = $hash_format . $salt;
    $hash = crypt($pass, $format_and_salt);
    return $hash;
}

function generate_salt($salt_length) {
    // Not 100% unique, not 100% random, but good enough for a salt
    // MD5 returns 32 characters
    $unique_random_string = md5(uniqid(mt_rand(), true));

    // Valid characters for a salt are [a-zA-Z0-9./]
    $base64_string = base64_encode($unique_random_string);

    // But not '+' which is valid in base64 encoding
    $modified_base64_string = str_replace('+', '.', $base64_string);

    // Truncate string to the correct length
    $salt = substr($modified_base64_string, 0, $salt_length);

    return $salt;
}

function password_check($password, $existing_hash) {
    // existing hash contains format and salt at start
    $hash = crypt($password, $existing_hash);
    if ($hash === $existing_hash) {
        return true;
    } else {
        return false;
    }
}

Can you do a review of this code, give some suggestions?

Also, I have one more question: Is it a good idea to use these functions as private methods of

User Model Entity Class OR
User Repository Class

or simply to have a file called functions.php and to put them there and to use them in controller?

Why reinvent the wheel when you can just use password_hash? Your code could be replaced by this:

$hash = password_hash($password);

password_verify($password, $hash);

It uses bcrypt (which uses blowfish internally), and it even manages salts for you.

This should also answer your additional questions: You don't really need any of the functions.

Misc

Blowfish is an encryption algorithm, but when used with crypt, what you do is still hashing, not encryption. So your function should be called password_hash.
=== isn't timing safe. This is more of a theoretical problem, but it's still better to use a timing safe function here.
if (cond) {return true;} else {return false;} can be written as return cond;
Contrary to your comments, generated salts may contain =.
You should document that your salt generation function can only generate salts with a max length of 44, in case it's reused in a different context.
One-time variables can be nice to give something a name, thus resulting in easier to read code (eg in your first two functions). But sometimes they are not really needed. In your second function, I would simply write return crypt($password, $existing_hash) === $existing_hash, it's shorter and I don't think you loose any readability.