Changing object attributes by calling getters and setters using reflection We have a class with 10 attributes (longs, Strings) the following method should change any attribute to any value of an object specified by the client,

It mimics the SQL update statement Update Users Set username = 'newValue' where id = 5

the user will provide the 4 variables id = 5 to find the object and username = newValue to change its username.

The method :

 public static void usersUpdate( String attributeToBeChanged, String newAttributeValue, String whereKey ,String whereValue) throws Exception{

        Method getter=getDeclaredMethodIgnoreCase(User.class,"get"+whereKey);

        HashMap<String,User> refToUsersCache= usersCache.getUsersCache(); // The objects are stored in this map as <username,User Object>
        User requiredUser = null;

        for(String user: refToUsersCache.keySet()) { // loop through the hashmap and call its getter to check its "ID" or any attribute
            if (getter.invoke(refToUsersCache.get(user)).equals(whereValue))  // getID() == "5" (in our example)
                requiredUser = refToUsersCache.get(user); // this is the required object
        }

        Method setter=getDeclaredMethodIgnoreCase(User.class,"set"+attributeToBeChanged);
        setter.invoke(requiredUser,newAttributeValue); // change the username = "newValue"

);

attributeToBeChanged = username

newAttributeValue= newValue

whereKey= id

whereValue = 5

The given code is only a snippet, so it's not really possible to compile and test it, but here's my review for what you posted:

Never say that a method throws Exception

The caller will either have to declare his method with throws Exception, or - in the worst case - he will wrap the call into a try-catch block like this:

try { ... } catch (Exception e) { /* ignore */ }

The problem is that this catches all exceptions. Even a NullPointerException.

The exception should be more specific - without leaking implementation details to the call site. There are different possible solutions for this. In any case, you then have to decide what should happen in each case. You could

wrap the exceptions into specific ones. Maybe an existing one, like an IllegalArgumentException
"ignore" the error, and just print a warning to the console

In both cases, the exception message or log output should contain the error message like

"Could not find method with name " + methodName

or

"Could not invoke method with name" + methodName

Stop the loop when you can stop it

Imagine there are 10000000 users. And imagine that the requiredUser is the first one in this collection. Then you will invoke the line

if (getter.invoke(refToUsersCache.get(user)).equals(whereValue))

unnecessarily 9999999 times. Changing this will also allow you to make the scope of the requiredUser variable smaller. And maybe it also forces you to think about what should happen when the user is not found. In your current implementation, this will cause a NullPointerException...

Avoid repeated map lookups

When iterating over the elements of a map, you often see this pattern:

for (Key key  : map.keySet()) {
    Value value = map.get(key);
    ...
}

But in your case, you are not interested in the keys. You are just interested in the values, and thus, can iterate over them:

for (Value value : map.values()) {
    ...
}

Even if you are interested in the exact key-value pair, you can iterate over the entries

for (Entry<Key, Value> entry  : map.entrySet()) {
    Key key = entry.getKey();
    Value value = entry.getValue();
    ...
}

thus also avoiding the unnecessary lookups.

Program to the interface

Your code contained the line

HashMap<String, User> refToUsersCache = usersCache.getUsersCache();

Declaring the variable as a HashMap is not necessary. And in fact, it should not be possible. The getUsersCache() method should return a Map<String, User>, and you should thus only be able to declare your variable as a Map<String, User>.

Also see What does it mean to “program to an interface”? on stackoverflow.

Use // Inline comments judiciously

It's not clear whether you added these comments only for posting the code here. In any case, you should avoid appending such comments at the end of (already long) lines

Use sensible variable names

This is certainly controversial. But the line

HashMap<String, User> refToUsersCache = usersCache.getUsersCache();

makes me shudder. What type does the variable usersCache have? And whatever type it has, shouldn't getUsersCache() return an object that has a type that justifies the name usersCache? For me, this line looks as odd as a line like

Engine theEngine = engine.getCar();

Something just looks wrong there...

However, here is a sketch of how I made some of these changes locally. But note that this involves some guesses about the part of the code that you did not post, so take this with a grain of salt:

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;

public class GetterSetterReflection
{
    public static void usersUpdate(String attributeToBeChanged,
        String newAttributeValue, String whereKey, String whereValue)
    {
        Map<String, User> usersCacheMap = usersCache.getUsersCache();
        for (User user : usersCacheMap.values())
        {
            Object userWhereValue =
                invokeGetterUnchecked(User.class, user, whereKey);
            if (Objects.equals(userWhereValue, whereValue))
            {
                invokeSetterUnchecked(
                    User.class, user, attributeToBeChanged, newAttributeValue);
                return;
            }
        }
        // What should happen here?
        System.err.println("No user found where "+whereKey+" is "+whereValue);
    }

    private static Object invokeGetterUnchecked(
        Class<?> c, Object object, String propertyName)
    {
        try
        {
            String methodName = "get" + propertyName;
            Method getter = getDeclaredMethodIgnoreCase(c, methodName);
            return getter.invoke(object);
        }
        catch (NoSuchMethodException
            | IllegalArgumentException
            | InvocationTargetException
            | IllegalAccessException e)
        {
            throw new IllegalArgumentException(
                "Could not find or invoke getter for " + propertyName, e);
        }
    }

    private static void invokeSetterUnchecked(
        Class<?> c, Object object, String propertyName, Object value)
    {
        try
        {
            String methodName = "set" + propertyName;
            Method setter = getDeclaredMethodIgnoreCase(c, methodName);
            setter.invoke(object, value);
        }
        catch (NoSuchMethodException
            | IllegalArgumentException
            | InvocationTargetException
            | IllegalAccessException e)
        {
            throw new IllegalArgumentException(
                "Could not find or invoke setter for " + propertyName, e);
        }
    }

    //=========================================================================
    // Dummy code below this line

    private static Method getDeclaredMethodIgnoreCase(
        Class<?> c, String methodName) throws NoSuchMethodException
    {
        Method[] methods = c.getDeclaredMethods();
        for (Method method : methods)
        {
            String name = method.getName();
            if (name.equalsIgnoreCase(methodName))
            {
                return method;
            }
        }
        throw new NoSuchMethodException(
            "Method not found: " + methodName + " in " + c);
    }

    static class User
    {
    }
    static class UsersCacheHolder
    {
        HashMap<String,User> getUsersCache()
        {
            return null;
        }
    }
    private static UsersCacheHolder usersCache;
}


PHP e-mail sending class design I'm working on a class, what will be able to send e-mails for our customers.  I need to help with review of my classes design. I'm trying to do it to be easy extendable in future. I'm thinking about using dependency injection to inject InputReaderInterface. But is it worth doing it?

InputReaderInterface - has method: getContent()
WebReader - for reading source e-mail HTML (implements InputReaderInterface)
ParamSetter - for setting different params for users (implements InputReaderInterface)
$smtp - class for saving access attributes for SMTP
Mailer - uses PHPMailer class for sending e-mails

In the future, params will be loaded from the database.

<?php
$reader1 = new WebReader($mailUrl);
$message = $reader1->getContent();

$params = array(
  "test@test.com" => array(
    "customer" => "Name Surname"
  )
);

$reader2 = new ParamSetter($message, $params);

$mailer = new Mailer($smtp, $from, $alias);

foreach($rows as $row) {
  $reader2->setParamsFor($row["email"]);
  $message = $reader2->getContent();

  $mailer->sendMail($row["email"], $subject, $message);
}
?>

Naming
Your names are... I miss the word to describe your naming overall... Either way let's start.
Some of your names are good. $smtp, $from, $alias, $mailer and so on. Some of your names need improvement. $reader1 for example..
That reader would be better off as $mailReader or $messageReader or even $webReader.
Whenever you number your variables something is wrong..
Then there's your $reader2.. And this is what's breaking my understanding..
You called the Class it has ParamSetter, but you make it implement a InputReaderInterface, and call the variable $reader...
Make up your mind. Is that thing a setter, a reader or something entirely different?
It definitely looks like it's something different. ParameterSetter seems to reflect it's usage. Having it implement InputReaderInterface is definitely the wrong choice.
Concept
Actually your whole concept of OOP seems to be incorrect. I'd expect $message to be a Message and not a "String". That would allow you to restructure your code to something like this, which looks waay more OOP to me:
<?php
    $messageReader = new WebReader($mailUrl);
    $message = new Message(messageReader->getContent());

    $params /* how you get your params at all... */
    $paramReader = new ParamReader($params);
    $mailer = new Mailer($smtp, $from, $alias);

    foreach($rows as $row) {
       $message->applyParameters($paramReader->getParameters($row['email']));
       $mailer->sendMail($row['email'], $subject, $message->getContent());
    }
?>

Of course this means you'd have to change some underlying mechanics. Also I just dropped your InputReaderInterface. In that setup it's not really helpful, as you use your classes differently.
Your ParamSetter was a ParamReader and some Message-functionality. Keep concerns separate, the ParamSetter does not need to know how a message works, and what parameters it wants. He shouldn't even tamper with the message. That would be a violation of the "Principle of Least Knowledge", which is a very important principle for OOP designing.
Instead the only one altering the $message should be the $message itself. Thus your ParamSetter in fact just wants to be a Reader, who is used by the Message.


Tree list recursion from Stanford tutorial I have found this appearing-to-be famous problem in Stanford tutorial about trees and decided to solve it.

My solution appears to work great:

 #include <iostream>
    using namespace std;

    struct num {

        int n;
        num* left;
        num* right;
        num* next;
        num* pre;
    };

    num* cr (int data) {

        num* node = new num;
        node->n = data;
        node->left = 0;
        node->right = 0;
        return node;
    }

/* function for tree creation*/
    void treecre (num* & node) {

        node = cr (10);
        node->left = cr (8);
        node->left->left = cr (6);
        node->left->left->left = cr (4);
        node->right = cr (12);
        node->right->right = cr (14);
        node->right->right->right = cr (16);
        node->right->left = cr(11);
        node ->left->right = cr (9);
    }
   // function for transforming the tree into linked list with next and previous // links
// but without linking the first and last element yet
    num* btc (num* node, num*& list) {
        num* temp1 = 0;
        num* max = node;
        if (node == 0) return 0;
        else {
        btc (node->right, list);
        num* temp = new num;
        temp->n = node->n;
        temp ->next = list;
        if (list != 0) list->pre = temp;
        list = temp;
        btc (node->left, list);
        }
        return list;
    }

    void printtree (num* node) {
        if (node != 0) {
            printtree (node->left);
            cout << node->n;
            printtree (node->right);
        }
    }
    // printing the linked list backwards
    void printback (num*& list, int& k) {
        if (k < 15) {
            cout << list->n;
            list = list->pre;
            printback (list, ++k);}
        else {cout << list->n;}
    }
    // and in right direction
    void printlistto (num*& list, int& k) {
        if (k < 12) {
            cout << list->n;
            list = list->next;
            printlistto (list, ++k);
        }
        else {
            cout << list->n; }
    }

    int main () {
        num* node = 0;
        num* list = 0;
        int k = 0;
        treecre (node);
        printtree (node);
        cout << endl;
        list = btc (node, list);
// next lines link the first and last elemets
        num* temp1 = list;
        if (temp1->next!= 0) {
        while (temp1->next != 0) {
            temp1 = temp1->next;
        }}
        temp1->next = list;
        list->pre = temp1;

        printback (list, k);
        cout << endl;
        cin.get ();
        cin.ignore();
    }

But then I take a look at this Stanford solution, which is way longer and uses other tactics.

Could you please tell me if I overlooked something? Does my version have some sufficient drawbacks or if it is fine?

Your solution may work great, but I see some room for improvement, especially in terms of readability and maintainability and using OO.

I quickly went over your recursive function btc, it seems to do the trick. But the code is a bit messy.

The stanford solution tries to embrace the typical list methods, like: appending elements, joining lists, etc. In fact that's the goal, right: porting a binary sorted tree to a list, so one should also provide a good list interface. They also do this in a good object oriented/modular and readable way.

A review on your coding style

To start: you miss some indentation (in your if/else clauses). This already can make it more readable!
Perhaps also document some steps in the recursion method, since recursion always results to confusion.

Methods

your method names are not that understandable, use camelCase and use appropriate, complete names, that indicate the purpose of the method.

for example:

treecre()

should be

treeCreate()

it would even be better to use a Tree class that has a create() method, which builds your test tree.

Classes

You also used a struct for your node, may I suggest to use classes, since they offer a wide set of feature that can be used for future purposes of your code: inheritance, polymorphism, etc. This is important since one might want to extend this implementation to handle nodes that contain ASCII characters instead of integers.  Or, refactor the class to use C++ templates to support generic types.

In addition I would use the standard list/binary tree names, like: root, node, .. which is easier for communication purposes.

I hope this is a bit helpful!


Decrease file size of encrypted file I am making a Cryptography application and I noticed that the encrypted file is approximately 2 times bigger because I am encoding the files in base64. Are there any file encodings that take less space? Here is the code that converts files into byte arrays:

Console.Write("Enter File Path: ");
docPath = Console.ReadLine();
extension = docPath.Substring(docPath.IndexOf(".")).Trim();
byte[] binarydata = File.ReadAllBytes(docPath);
text = System.Convert.ToBase64String(binarydata, 0, binarydata.Length);
var Encrypted = AESCryptography.Encrypt(text, m.ToString(), extension);
using (FileStream fs = File.Create(docPath.Substring(0,docPath.IndexOf(".")) + ".aent"))
{
    Byte[] info = new UTF8Encoding(true).GetBytes(Encrypted);
    // Add some information to the file.
    fs.Write(info, 0, info.Length);
}

and here is the Encryption class that does the actual encryption:

public static class AESCryptography
{
    private const int keysize = 256;
    public static string Encrypt(string plainText, string passPhrase, string extention)
    {
        byte[] plainTextBytes = Encoding.UTF8.GetBytes(plainText);
        using (PasswordDeriveBytes password = new PasswordDeriveBytes(passPhrase, null))
        {
            byte[] keyBytes = password.GetBytes(keysize / 8);
            using (RijndaelManaged symmetricKey = new RijndaelManaged())
            {
                symmetricKey.GenerateIV();
                symmetricKey.Mode = CipherMode.CBC;
                using (ICryptoTransform encryptor = symmetricKey.CreateEncryptor(keyBytes, symmetricKey.IV))
                {
                    using (MemoryStream memoryStream = new MemoryStream())
                    {
                        using (CryptoStream cryptoStream = new CryptoStream(memoryStream, encryptor, CryptoStreamMode.Write))
                        {
                            cryptoStream.Write(plainTextBytes, 0, plainTextBytes.Length);
                            cryptoStream.FlushFinalBlock();
                            byte[] cipherTextBytes = memoryStream.ToArray();
                            return Convert.ToBase64String(cipherTextBytes) + "\n" + Convert.ToBase64String(symmetricKey.IV) + "\n" + extention;
                        }
                    }
                }
            }
        }
    }

    public static string Decrypt(string cipherText, string passPhrase, string initVector)
    {
        byte[] initVectorBytes = Convert.FromBase64String(initVector);
        byte[] cipherTextBytes = Convert.FromBase64String(cipherText);
        using (PasswordDeriveBytes password = new PasswordDeriveBytes(passPhrase, null))
        {
            byte[] keyBytes = password.GetBytes(keysize / 8);
            using (RijndaelManaged symmetricKey = new RijndaelManaged())
            {
                symmetricKey.Mode = CipherMode.CBC;
                using (ICryptoTransform decryptor = symmetricKey.CreateDecryptor(keyBytes, initVectorBytes))
                {
                    using (MemoryStream memoryStream = new MemoryStream(cipherTextBytes))
                    {
                        using (CryptoStream cryptoStream = new CryptoStream(memoryStream, decryptor, CryptoStreamMode.Read))
                        {
                            byte[] plainTextBytes = new byte[cipherTextBytes.Length];
                            int decryptedByteCount = cryptoStream.Read(plainTextBytes, 0, plainTextBytes.Length);
                            return Encoding.UTF8.GetString(plainTextBytes, 0, decryptedByteCount);
                        }
                    }
                }
            }
        }
    }
}

Let us analyze what the code is doing:

reading a file by calling File.ReadAllBytes()
converting these bytes to a base64 String
passing this String to the Encrypt method
converting the passed String to byte[]
encrypting this byte[]
converting the encrypted byte[] to base64
converting the IV vector to base64
returning the addition of 6., 7. and the extension
converting the returned Stringto byte[] which is saved to a file with .aent extension

I can only see one reason why you want to do it in this way.

-> because you want to store the extension of the original file into the encrypted file.

Let us assume we don't want to store the extension of the original file inside the encrypted file, but we just append the .aent extension to the original filename.

This would have a lot of benefits. The code could be faster and the files could be smaller.

Why so ?

As CodesInChaos pointed out in the comments, Base64 increases the size of the data by 1.33 because for each 3 bytes you get 4 bytes back and the output is padded to always be a multiple of 4.
So starting with a file having a size of 1000 bytes after 2. you have 1336 bytes. After the encryption the conversion to base64 of the encrypted data, only assuming you still would have 1336 bytes, will result in 1784 bytes. Now adding the to base64 converted IV vector, which has a size of 16 for the default RijndaelManaged, will result in additional 24 bytes. The \n will result each in 2 bytes.

So all together: 1784 + 2 + 24 + 2 = 1812 almost a factor of 2.

So let us skip the base64 converting and the adding of the file extension.

Some remarks about the code

using of const for the keysize is good
using proper casing for methodnames and parameter is good except byte[] Encrypted
using the using keyword for using IDisposable objects is good

using a parametername plainText but also stating it could be a jpeg file is not so good
the way of getting the extension, instead of using built in methods is not so good

What you can do, but this is really a matter of taste, to reduce the indention of the code is stacking the using statements together like shown below. If we also extract the getting of the keybytes to a separate method, we loose one more indention and also the Encrypt() method gets shorter.

Refactoring

So let us start with the latter:

private static byte[] GetKeyBytes(string passPhrase)
{
    using (PasswordDeriveBytes password = new PasswordDeriveBytes(passPhrase, null))
    {
        byte[] keyBytes = password.GetBytes(keysize / 8);
        return keyBytes;
    }
}

Now we will change the String plainText input parameter to byte[] plainBytes and the returntype to byte[]

But wait, as we get two byte arrays, one of the encrypted data and one of the IV vector data, we need to combine these to one array. So let us introduce a method first

private static byte[] Combine(byte[] first, byte[] second)
{
    byte[] ret = new byte[first.Length + second.Length];
    Buffer.BlockCopy(first, 0, ret, 0, first.Length);
    Buffer.BlockCopy(second, 0, ret, first.Length, second.Length);
    return ret;
}

And now the Encrypt method

public static byte[] Encrypt(byte[] plainBytes, string passPhrase)
{
    byte[] keyBytes = GetKeyBytes(passPhrase);

    using (RijndaelManaged symmetricKey = new RijndaelManaged())
    {
        symmetricKey.GenerateIV();
        symmetricKey.Mode = CipherMode.CBC;

        using (ICryptoTransform encryptor = symmetricKey.CreateEncryptor(keyBytes, symmetricKey.IV))
        using (MemoryStream memoryStream = new MemoryStream())
        using (CryptoStream cryptoStream = new CryptoStream(memoryStream, encryptor, CryptoStreamMode.Write))
        {
            cryptoStream.Write(plainBytes, 0, plainBytes.Length);
            cryptoStream.FlushFinalBlock();
            return Combine(memoryStream.ToArray(), symmetricKey.IV);
        }
    }
}

If we want to decrypt this array, we need to get the last 16 bytes and use it as IV vector. The bytes till the last 16 bytes will then be the encrypted data itself.

Now, let us use the Encrypt() method

Console.Write("Enter File Path: ");
docPath = Console.ReadLine();
byte[] binarydata = File.ReadAllBytes(docPath);
byte[] Encrypted = Encrypt(binarydata, m.ToString());
File.WriteAllBytes(docPath + ".aent", Encrypted);

Additional

You really should check out the System.IO.Path class, as this class provides many helpful methods like GetExtension() for getting the fileextension, GetFileNameWithoutExtension() etc. .


Converting a DropDownList retrieval service to generics or delegate methods mappingService below is actually just Automapper. I have a wrapper around it so that I can inject it.

I want to refactor the following code to be cleaner:

 public class DropDownListService : IDropDownListService
{
    private readonly IMappingService _mappingService;
    private readonly ICompanyRepository _companyRepository;
    private readonly IProjectTypeRepository _projectTypeRepository;
    private readonly IStatusRepository _statusRepository;
    private readonly IProjectManagerRepository _projectManagerRepository;
    private readonly IProjectArchitectRepository _projectArchitectRepository;
    private readonly IFeeTypeRepository _feeTypeRepository;
    private readonly IStateRepository _stateRepository;
    private readonly IPersonTypeRepository _personTypeRepository;

    public DropDownListService(IMappingService mappingService, ICompanyRepository companyRepository, IProjectTypeRepository projectTypeRepository, IStatusRepository statusRepository, IProjectManagerRepository projectManagerRepository, IProjectArchitectRepository projectArchitectRepository, IFeeTypeRepository feeTypeRepository, IStateRepository stateRepository, IPersonTypeRepository personTypeRepository)
    {
        _mappingService = mappingService;
        _companyRepository = companyRepository;
        _projectTypeRepository = projectTypeRepository;
        _statusRepository = statusRepository;
        _projectManagerRepository = projectManagerRepository;
        _projectArchitectRepository = projectArchitectRepository;
        _feeTypeRepository = feeTypeRepository;
        _stateRepository = stateRepository;
        _personTypeRepository = personTypeRepository;
    }

    public IEnumerable<IDropDownList> RetrieveCompanies()
    {
        return _mappingService.Map(_companyRepository.GetAll(), new List<Company>());
    }

    public IEnumerable<IDropDownList> RetrieveJobTypes()
    {
        return _mappingService.Map(_projectTypeRepository.GetAll(), new List<ProjectType>());
    }

    public IEnumerable<IDropDownList> RetrieveStatuses()
    {
        return _mappingService.Map(_statusRepository.GetAll(), new List<Status>());
    }

    public IEnumerable<IDropDownList> RetrieveProjectManagers()
    {
        return _mappingService.Map(_projectManagerRepository.GetAll(), new List<ProjectManager>());
    }

    public IEnumerable<IDropDownList> RetrieveProjectArchitects()
    {
        return _mappingService.Map(_projectArchitectRepository.GetAll(), new List<ProjectArchitect>());
    }

    public IEnumerable<IDropDownList> RetrieveFeeTypes()
    {
        return _mappingService.Map(_feeTypeRepository.GetAll(), new List<FeeType>());
    }

    public IEnumerable<IDropDownList> RetrievePhases()
    {
        return _mappingService.Map(_feeTypeRepository.GetAll(), new List<Phase>());
    }

    public IEnumerable<IDropDownList> RetrieveStates()
    {
        return _mappingService.Map(_stateRepository.GetAll(), new List<State>());
    }

    public IEnumerable<IDropDownList> RetrievePeopleTypes()
    {
        return _mappingService.Map(_personTypeRepository.GetAll(), new List<PersonType>());
    }

    public IEnumerable<IDropDownList> RetrieveMarkets()
    {
        return _mappingService.Map(_marketRepostiory.GetAll(), new List<Market>());
    }
}

As you can see I am injecting way too many things into this controller. I am not currently doing a spa application so I cannot do this separately on the presentation layer. What I would really like to do is use generics. The problem is, the DB value that is returned to me looks like this:

    public int StateID { get; set; }
    public string StateName { get; set; }

Each entity has the ID field and the name field only they are named for the table. I am unable to write one map (as far as I know how) with automapper to map them to the correct type.

How can I clean this class up?

The number of dependencies suggests this class is doing too much work and violates single responsibility principle. Instead of having one class factor all different enumerations, have multiple classes, e.g. one for project members, one for states, etc.


Naive prime finder, unexpected behavior in Python I have verified that both of my following functions successfully find prime numbers. I was pretty sure the function called is_prime_new would be faster than is_prime_old; however, after benchmarking (mainly consisting of generating huge lists of primes), I found this to be significantly false and I'm wondering why this may be.

I had two main reasons for thinking that is_prime_new would be a better algorithm than its counterpart:

is_prime_new does more checks for non-primailty early, thus eliminating numbers before iteration
It iterates in steps of 6, whereas its counterpart iterates in steps of 2, both to the same value sqrt(n)+1

Could anyone explain to me why the newer function is less optimized, and possibly debunk my above assumptions? Thank you.

def is_prime_new(n):
    if n < 2: return False
    if n == 2 or n == 3: return True
    if n%2 == 0 or n%3 == 0: return False
    sqr = math.sqrt(n)
    if sqr == int(sqr): return False

    i = 1
    while i*6 < sqr+1:
        if n % (i*6-1) == 0 or n % (i*6+1) == 0: return False
        i += 1

    return True

def is_prime_old(n):
    if n < 2: return False
    if n == 2: return True
    if n%2 == 0: return False

    for i in xrange(3, int(math.sqrt(n)+1), 2):
        if not n%i: return False

    return True

Duncan's explanation is incorrect. This isn't my best piece of writing - bear with me.

First lets deal with n < 2 and n == 2 or n == 3:

It is true that they let a lot more input go past them than they stop. But they don't account for the difference in time.

Even the old definition has n < 2 and n == 2.
The time spent on this part of the function is very insignificant compared to that spent in the rest of the function.

Next n%2 == 0 or n%3 == 0:

Again n%2 == 0 is in the old definition also.
n%3 == 0 is the first check in old's for loop
The time spent on this part of the function is also insignificant compared to that spent in the rest of the function.

I could've clubbed this with the previous section but didn't for the following reason:

These checks work on 4 out of every 6 input numbers. And 4 > 6-4. But is that the way to look at it? If say they'd worked on 3/6 [As in the case of old] are these checks useless? If say they'd worked on 2/6 should they be avoided? No.

By doing one(or two) comparison(s) on 6 inputs, they stop 2 from reaching the work horse of the code - the loop - that does most of the work. That's not to be avoided!

Also, by imposing additional constraints on the input that reaches the loop, the loop itself has been made faster! Knowing that no multiple of 3 enters the loop, you don't test divisibility of n by any other multiple of 3. Don't forget your main idea behind moving from old to new!

Faster loop + lesser numbers reaching loop should make new faster than old.

How much faster exactly?

I mentioned "lesser numbers reaching loop" being good only as a general guideline. It doesn't help much here because : In old 3,5,7,9... enter the loop, and in new 5,7,11,13... enter the loop. But in old 3,9... get stopped at the first check of the for loop. So only faster loop is important.
new's loop jumps in steps of 6 but old's jumps in steps of 2. So is new 3 times faster than old? No. At every step new does 3 comparisons - one for the while loop condition and two inside the loop. old does 2 - one at for loop condition and one inside the loop. So, new is (6/3) * (2/2) = 2 times faster than old.

So far I've only been explaining why new should be faster. Finally I'll address your question of why this isn't observed:

for loops are much faster than while loops. python doesn't know how i is going to change inside while. So the while loop's condition check is like any other comparison. On the other hand for loop's condition check is highly optimised. Henceforth lets assume that for loop's condition check doesn't count as a comparison.

Then old's time/new's time will be

6/3 * 2/2 = 2 if both use while
6/2 * 1/2 = 1.5 if both use for
6/2 * 2/2 = 3 if old used while and new used for
6/3 * 1/2 = 1 if old used for and new used while

That still means that old and new should be (at least approximately) equally fast. This is where I was stumped. I had to resort to experimenting to figure this out. The reasons for this not occurring, as it turns out, are

All the unnecessary arithmetic (*6) in the while loop
while i*6 < sqr+1: Here sqrt is a floating point number. As it turns out float comparison is costlier than int comparison. while i*6 < int(sqr+1) isn't good either. You should use sqrp1 = int(sqr+1); while i*6 < sqrp1

Note that the ratios provided are from rough calculations and far from being accurate - there are so many more things to consider. One major assumption is that time spent in the loops is much larger than time spent at base case checks - not true if all your input are multiples of 2.

To summarise, the reasons for new being slower, despite being expected to be faster, are:

Usage of while instead of for (this is not as important as the other two)
Unnecessary arithmetic inside while
Usage of float comparison instead of int comparison in while condition check.

The code given below illustrates some of the points made here.

import math, timeit

def is_prime_new_slowest(n):
    if n < 2: return False
    if n == 2 or n == 3: return True
    if n%2 == 0 or n%3 == 0: return False
    sqr = math.sqrt(n) #This is insignificant as it is outside the loop
    if sqr == int(sqr): return False
    sqrtp1 = sqr + 1 #Note that sqrtp1 is a float
    i = 1
    while i*6 < sqrtp1: # Comparison of floats adds some time
        if n % (i*6-1) == 0 or n % (i*6+1) == 0: return False #Unnecessary arithmetic
        i += 1
    return True

def is_prime_new_slow(n):
    if n < 2: return False
    if n == 2 or n == 3: return True
    if n%2 == 0 or n%3 == 0: return False
    sqrtp1 = int(math.sqrt(n) + 1)
    i = 6
    while i < sqrtp1:
        if n % (i-1) == 0 or n % (i+1) == 0: return False
        i += 6
    return True

def is_prime_new_fast(n):
    if n < 2: return False
    if n == 2 or n == 3: return True
    if n%2 == 0 or n%3 == 0: return False
    sqrtp1 = int(math.sqrt(n)+1)
    for i in xrange(6, sqrtp1, 6):
        if n % (i-1) == 0 or n % (i+1) == 0: return False
    return True

def is_prime_old_slow(n):
    if n < 2: return False
    if n == 2: return True
    if n%2 == 0: return False
    sqrtp1 = int(math.sqrt(n) + 1)
    i = 3
    while i < sqrtp1:
        if not n%i: return False
        i += 2
    return True

def is_prime_old_fast(n):
    if n < 2: return False
    if n == 2: return True
    if n%2 == 0: return False
    sqrtp1 = int(math.sqrt(n)+1)
    for i in xrange(3, sqrtp1, 2):
        if not n%i: return False
    return True

def sundaram3(max_n):
    numbers = range(3, max_n+1, 2)
    half = (max_n)//2
    initial = 4
    for step in xrange(3, max_n+1, 2):
        for i in xrange(initial, half, step):
            numbers[i-1] = 0
        initial += 2*(step+1)
        if initial > half:
            return [2] + filter(None, numbers)

i = sundaram3(10**6)[-1]

print "Input: largest prime less than 10^6, No. of runs: 25000"
print "new slowest:", timeit.timeit('is_prime_new_slowest(i)', setup="from __main__ import is_prime_new_slowest, i; import math", number=25000)
print "new slow:", timeit.timeit('is_prime_new_slow(i)', setup="from __main__ import is_prime_new_slow, i; import math", number=25000)
print "new fast:", timeit.timeit('is_prime_new_fast(i)', setup="from __main__ import is_prime_new_fast, i; import math", number=25000)
print "old slow:", timeit.timeit('is_prime_old_slow(i)', setup="from __main__ import is_prime_old_slow, i; import math", number=25000)
print "old fast:", timeit.timeit('is_prime_old_fast(i)', setup="from __main__ import is_prime_old_fast, i; import math", number=25000)

print "\nInput: range(1000000), No. of runs = 1"
print "new slowest:", timeit.timeit('for i in xrange(1000000): is_prime_new_slowest(i)', setup="from __main__ import is_prime_new_slowest; import math", number=1)
print "new slow:", timeit.timeit('for i in xrange(1000000): is_prime_new_slow(i)', setup="from __main__ import is_prime_new_slow; import math", number=1)
print "new fast:", timeit.timeit('for i in xrange(1000000): is_prime_new_fast(i)', setup="from __main__ import is_prime_new_fast; import math", number=1)
print "old slow:", timeit.timeit('for i in xrange(1000000): is_prime_old_slow(i)', setup="from __main__ import is_prime_old_slow; import math", number=1)
print "old fast:", timeit.timeit('for i in xrange(1000000): is_prime_old_fast(i)', setup="from __main__ import is_prime_old_fast; import math", number=1)

Output on my machine:

Input: largest prime less than 10^6, No. of runs = 25000
new slowest: 7.37190294266
new slow: 4.19636297226
new fast: 3.49502682686
old slow: 6.08556985855
old fast: 4.03657603264

Input: range(1000000), No. of runs = 1
new slowest: 21.5607540607
new slow: 13.8396618366
new fast: 12.3734707832
old slow: 20.0376861095
old fast: 14.1358599663

Note that for - while deterioration becomes less important as the number of comparisons inside the loop reduces [old - one comparison - time goes from 14 to 20, new - two comparisons - 12.3 to 13.83]. This is expected because 1 comparison becoming 2 has a greater effect on final time than 10 comparisons becoming 11. This is why while-for performance difference generally doesn't matter in real life.


Conversion functions between binary, hexadecimal and ASCII I've written a small suite to easily convert my data between binary, hexadecimal and ASCII:

#!/usr/bin/env python2
# -*- coding: utf-8 -*-
import sys

def to_list(chain, offset):
    return [chain[i:i+offset] for i in range(0, len(chain), offset)]

def bin2str(chain):
    return ''.join((chr(int(chain[i:i+8], 2)) for i in range(0, len(chain), 8)))

def bin2hex(chain):
    return ''.join((hex(int(chain[i:i+8], 2))[2:] for i in range(0, len(chain), 8)))

def str2bin(chain):
    return ''.join((bin(ord(c))[2:].zfill(8) for c in chain))

def str2hex(chain):
    return ''.join((hex(ord(c))[2:] for c in chain))

def hex2bin(chain):
    return ''.join((bin(int(chain[i:i+2], 16))[2:].zfill(8) for i in range(0, len(chain), 2)))

def hex2str(chain):
    return ''.join((chr(int(chain[i:i+2], 16)) for i in range(0, len(chain), 2)))

if __name__ == '__main__':
    word = sys.argv[1]
    word_bin = str2bin(word)
    word_hex = str2hex(word)

    # tests
    assert bin2str(word_bin) == word
    assert bin2hex(word_bin) == word_hex
    assert hex2str(word_hex) == word
    assert hex2bin(word_hex) == word_bin

    # output
    print "bin:", to_list(word_bin, 8)
    print "hex:", to_list(word_hex, 2)

Example:

$ ./utils.py test
bin: ['01110100', '01100101', '01110011', '01110100']
hex: ['74', '65', '73', '74']

How would you optimize those methods?

Also, I realize I'm actually only working with str representations of the different formats. What would you do to get real binary or hexadecimal results from those functions?

Step 1: minor cleanup

You've defined a to_list() function, but aren't really taking advantage of it.  Instead, you're still writing things like … for i in range(0, len(chain), 8)) in functions like bin2str.  You should change to_list() into a generator.  (Then, since it is no longer returning a list, you should also rename it.  You'll end up with the same thing as the chunks() function in this Stack Overflow answer.)

chain is an odd parameter name.  It strikes me as a botched English translation from a Romance language ("chaîne", "catena", etc.).  If we want to help programmers who aren't English speakers, I suggest replacing your …2… names with _to_.

Instead of slicing off the 0b and 0x prefixes generated by the bin() and hex() functions, and zero-padding using zfill(), use str.format() to take care of both problems.

# http://stackoverflow.com/a/312464/1157100
def _chunks(str, chunk_size):
    for i in xrange(0, len(str), chunk_size):
        yield str[i:i+chunk_size]

def hex_to_bin(hex):
    return ''.join('{:08b}'.format(int(x, 16)) for x in _chunks(hex, 2))

def str_to_bin(str):
    return ''.join('{:08b}'.format(ord(c)) for c in str)

def bin_to_hex(bin):
    return ''.join('{:02x}'.format(int(b, 2)) for b in _chunks(bin, 8))

def str_to_hex(str):
    return ''.join('{:02x}'.format(ord(c)) for c in str)

def bin_to_str(bin):
    return ''.join(chr(int(b, 2)) for b in _chunks(bin, 8))

def hex_to_str(hex):
    return ''.join(chr(int(x, 16)) for x in _chunks(hex, 2))

I've rearranged the order of appearance, grouping by the target type rather than the source type, as a segue to the next step…

Step 2: decluttering the interface

Looking at the code above, you'll notice a pattern: each function just mixes-and-matches code.  There is always a decoding portion (either … for something in _chunks(input, base) or … for c in str) and an encoding portion (either ''.join('{:format}'.format(something) …) or ''.join(chr(int(something, base)) …)).

A more disturbing observation is that the complexity is exponential.  If you wanted to add octal support, for example, you would need to add str_to_oct(), oct_to_str(), bin_to_oct(), oct_to_bin(), hex_to_oct(), and oct_to_hex() — doubling the number of functions.

A better design would be to convert everything to/from a common interface.  I've chosen an iterable of ASCII values as that hub.

def _chunks(str, chunk_size):
    for i in xrange(0, len(str), chunk_size):
        yield str[i:i+chunk_size]

def from_str(str):
    for c in str:
        yield ord(c)

def to_str(ascii):
    return ''.join(chr(a) for a in ascii)

def from_bin(bin):
    for chunk in _chunks(bin, 8):
        yield int(chunk, 2)

def to_bin(ascii):
    return ''.join('{:08b}'.format(a) for a in ascii)

def from_hex(hex):
    for chunk in _chunks(hex, 2):
        yield int(chunk, 16)

def to_hex(ascii):
    return ''.join('{:02x}'.format(a) for a in ascii)

Then, instead of writing str2bin(word), you would write to_bin(from_str(word)).

You happen to be at the break-even point: either way, there are six functions.  The architecture proposed in Step 2 simplifies your library (at a slight burden to the caller).  You would see a payoff, though, if you were to add octal support — you would be adding two functions instead of six.


String reversal function I have an assignment to create a function that reverses the letters of each word in a string individually. I feel like this should be a rather simple task. I did figure out a solution, although I feel like it uses too many variables as a sort of hack method. Any suggestions on how to make it more efficient/shorter?

#include<iostream>
#include<conio.h>
#include<string.h>

using namespace std;

void rev(char str[80])
{
char t;
int l = 0, n = strlen(str), k = 0;

for (int i = 0; i <= n; i++)
{
    if (str[i] == ' ' || str[i] == '\0')
    {
        for (int j = i - 1; j > (k + i - 1) / 2; l++, j--)
        {
            t = str[j];
            str[j] = str[l];
            str[l] = t;
        }
        k = i + 1;
        l = k;
    }
}

}

void main()
{
    char str[80];
    cout << "Please enter a string : ";
    cin.getline(str, 80);
    cout << "The original string is : " << str << "\n";
    rev(str);
    cout << "The new string is : " << str << "\n";
    system("pause");
}

main should be int main()
Why C-style strings? Use std::string
One function - one task. You are mixing 2 things into one function:

Reverse chars in a word
Find words in a sentence separated by spaces

I feel like your problem with too many variables will resolve when you split the tasks you are doing into separate functions.
Use better code formatting (auto-format).
Type mixing - strlen's return value is std::size_t. Do not assign to int. As a follow-up, use a loop index of type std::size_t to match.
This is my preference - not using using namespace std; and system("pause");. I think a lot of posts could be found explaining why.


Counting occurrences of "A…Z" substrings I'm trying to write code in the simplest and cleanest way possible. I've found a few ways to shorten and simplify my code through functions that I've never seen before or through using other methods. I'd like to expand my knowledge on writing code using various (but simple) methods, and also expand my function 'vocabulary'.

What I mainly want to know is if there are other ways of writing the for loop such that it is much easier to read, and is shorter and simpler. Just yesterday I found out that the enumerate function exists, which made my life so much easier! :)

On to the question-

Substring Counter:
(I thought of this function as a programming challenge for myself)

A function that takes in a string and returns the amount of times 'A' ends with 'Z'. MUST ONLY CONTAIN 1 LOOP. Not sure how to explain these, but here are examples:
substring_counter(string)
It is assumed that the input is a string and that all characters are alphabetical and capital. I know I can put checks for all these but I'm mainly focusing on the logical way the code is written.

substring_counter('AZAAZ')
> 4

'AZAAZ' has 4 instances where it starts with 'A' and ends with 'Z'.

'AZAAZ'

'AZAAZ'

'AZAAZ'

'AZAAZ'

substring_counter('AZZ')
> 2

'AZZ' has 2 instances where it starts with 'A' and ends with 'Z'.

'AZZ'
'AZZ'

1st method (using slicing):

def substring_counter(string):
    counter = 0
    for num, x in enumerate(string):
        if x == 'A':
            if  string[num:].find('Z') != -1:
                counter += string[num:].count('Z')
    return counter

2nd method (using counters):

def substring_counter(x):
    counter=0
    word = 0
    for i in x:
        if(i == 'A'):
           word += 1
        elif(i == 'Z'):
           counter += word
    return counter

Which method is more understandable, or simpler? Which one should I be using?

The second solution seems to be clearer and more efficient.
In both cases, a few details can be improved:

variable names
removing useless whitespaces
removing useless parenthesis
removing useless tests (no need to call find before calling count)
using the start parameter of count (suggestion of @SolomonUcko).

You'd get something like:

def substring_counter(string):
    counter = 0
    for num, c in enumerate(string):
        if c == 'A':
            counter += string.count('Z', num)
    return counter

def substring_counter2(string):
    counter = 0
    nb_a = 0
    for c in string:
        if c == 'A':
           nb_a += 1
        elif c == 'Z':
           counter += nb_a
    return counter


Generate a Bitcoin Address and the Wif from a phrase Is there a way to speed up my Python code?

import scrypt
import binascii
from passlib.utils.pbkdf2 import get_prf, pbkdf2
from coinkit import BitcoinKeypair

def getWallet(phrase, saltPhrase):
    s1 = scrypt.hash(password=phrase+'\x01', salt=saltPhrase+'\x01', N=1<<18, r=8, p=1, buflen=32)
    s2 = pbkdf2(phrase+'\x02', saltPhrase+'\x02', 1<<16, keylen=32, prf='hmac-sha256')

    key = ''.join(chr(ord(a) ^ ord(b)) for a,b in zip(s1,s2))
    key_hex = binascii.hexlify(key)
    newWallet = BitcoinKeypair(key_hex)
    return {"walletAddress":newWallet.address(), "walletWif":newWallet.wif_pk()}

myWallet = getWallet("ER8FT+HFjk0", "a@b.c")
print "Address: "+myWallet["walletAddress"]+"\nWif: "+myWallet["walletWif"]

To try it you need to install these libraries:

pip install scrypt passlib coinkit

This program generates a Bitcoin Address and the Wif from a phrase, but the problem is that it generates the Output in about 1 second, and that's too long. So i'm looking for a way to speed it up, and if there aren't any solutions, perhaps I could use another programming language?

Thank you.

If you're planning on running this repeatedly, you could try running this on PyPy, and see if the JIT starts to speed up some of the regions being run repeatedly.  That would be a low-effort way to try to get a performance improvement on long-running code.

Before doing any code changes, profile your current code.  That should tell you where the most gains are to be had.  Then you could move forward with optimizing with a lot more information.

And, it's possible that doing this in a language with a really good compiler for native machine code would get you faster results.  I would tend to think of OCaML in that context, but your milage may vary.


"Cryptographic" hash function I don't know much about the design of cryptographic hash functions but despite that I made an attempt for the sake of learning. This is what I got:

#include <stdio.h>

void hash(int* bits, int bitsLen, int* out, int outLen){
  const int tt[9] = {1,2,0,0,0,1,1,2,2};
  int st[128], i, k, j, y, a, b, o = 0;
  for (i=0; i<128; ++i)
    st[i] = i%3;
  for (k=0; k < bitsLen + outLen; ++k){
    for (j=0; j<32; ++j){
      if (k < bitsLen)
        st[0] = bits[k];
      for (i=j%2; i<128; i+=2){
        y = (i+1)%128;
        a = st[i];
        b = st[y];
        st[i] = tt[a*3+b];
        st[y] = tt[b*3+a];
      };
    };
    if (k >= bitsLen)
      out[o++] = (st[0] + st[1]*3 + st[2]*9 + st[3]*27) % 2;
  };
}

void printHex(int* bits, int bitsLen){
  int i;
  for (i=0; i<bitsLen/4; ++i)
    printf("%x", bits[i*4+0]*8+bits[i*4+1]*4+bits[i*4+2]*2+bits[i*4+3]);
}

void toBits(int n, int* bits, int bitsLen){
  int i;
  for (i=0; i<bitsLen; ++i)
    bits[bitsLen-i-1] = (n >> i) & 1;
}

int main(){
  const int bitsLen = 16;
  int bits[bitsLen];

  const int outLen = 256;
  int out[outLen];

  int i;
  for (i=0; i<256; ++i){
    toBits(i, bits, bitsLen);
    hash(bits, bitsLen, out, outLen);
    printHex(bits, bitsLen);
    printf(": ");
    printHex(out, outLen);
    printf("\n");
  };
}

This passes my (certainly lacking) criteria. I'd like to know what are the weaknesses of this function and how it could be exploited if actually used to replace, say, SHA3 in a cryptographic application that requires collision-resistance, non-reversibility and so on.

First you need to read Schneier's Memo to the Amateur Cipher Designer.  All amateur cryptography is faulty, not just yours.  I too designed a cryptographic hash, based round RC4, put it out on the web and Scott Fluhrer was kind enough to show me the obvious faults in it.  He is an expert, so he knew what to look for.  I was not and I did not know what I had to protect against.  Our conversation is probably still out there on the sci.crypt Usenet group from 2007.

To design a good hash function you need to know the types of attacks that are used against hashes.  For example, does your hash specifically guard against length-extension attacks?

How fast is your hash compared to other current cryptographic hashes such as SHA3?

For a first suggestion, learn how cryptographic hashes pad their input and why.  Either incorporate the same technique into your hash or else find a different way to serve the same purpose.  You are not the first person to write a hash.  Learn so you can stand on the shoulders of giants.


Take a result set and organize it into a dictionary I'm trying to take a result set and organize it into a dictionary then assign the dictionary values to variables that are publicly accessible from my controller. My issue is I have to check if there is a key, if there is a key, assign it to a variable manually, if there isn't set the variable to 0. Is there a better dynamic way of doing this? The code works but I suspect there is a better implementation.

public class OtherTimeTypes
     {
    private PersonnelManagementEntities db = new    PersonnelManagementEntities();
    private string _usrName;
    public double takenCompHours { get; private set; }
    public double takenBereavementHours { get; private set; }
    public double takenJuryDutyHours { get; private set; }
    //generate the different DayTypes in a dictionary for key/value processing. Key = variable name, Value = DayTypeID
    private Dictionary<string, int> dayType = new Dictionary<string, int>
    {
        { "takenCompHours", 7 },
        { "takenBereavementHours", 3 },
        { "takenJuryDutyHours", 4 }
    };

    public OtherTimeTypes(string userName)
    {
        //set the AD user name in constructor
        _usrName = userName;
        //Setup dictionary to store results
        Dictionary<string, double> hoursCount = new Dictionary<string, double>();
        //initalize base line hours
        double returnedHours = 0;
        // iterate through dictionary with linq queries
        foreach (KeyValuePair<string, int> pair in dayType)
        {
            var result = db.TimeRequests
                           .Include(t => t.User)
                           .Where(t => t.DayTypeID == pair.Value)
                           .Where(t => t.User.ADUserName == _usrName)
                           .Where(t => t.UserSubmitDate.Year == DateTime.Now.Year)
                           .ToList();

            //make the hours calculation and store it in dictionary.
            foreach (TimeRequest hours in result)
            {
                returnedHours = returnedHours + (((hours.eDateTime - hours.sDateTime).TotalDays + 1) * (int)hours.HoursRequested);
                //If the dictionary does not contain a K/V add it
                if (!hoursCount.ContainsKey(pair.Key))
                {
                    hoursCount.Add(pair.Key, returnedHours);
                }
                //If it does contain a value for the day type add it to the value compDay1 = 2 hrs, compDay2 = 4 hrs = compDayVariable=6
                else
                {
                    hoursCount[pair.Key] = hoursCount[pair.Key] + returnedHours;
                }
                returnedHours = 0;
            }
        }
//THIS IS THE PORTION OF CODE I DISLIKE
        //assign dictionary values to public variables for the TimeOffViewModel
        if (hoursCount.ContainsKey("takenCompHours"))
        { takenCompHours = hoursCount["takenCompHours"]; }
        else { takenCompHours = 0; }

        if (hoursCount.ContainsKey("takenBereavementHours"))
        { takenBereavementHours = hoursCount["takenBereavementHours"]; }
        else { takenCompHours = 0; }

        if (hoursCount.ContainsKey("takenJuryDutyHours"))
        { takenJuryDutyHours = hoursCount["takenJuryDutyHours"]; }
        else { takenJuryDutyHours = 0; }
    }
}

Naming

The C# standard for publicly-visible properties is to use PascalCase:

public double TakenCompHours { get; private set; }
public double TakenBereavementHours { get; private set; }
public double TakenJuryDutyHours { get; private set; }

Additionally, db is not a very descriptive name for your entities.  It seems something like personnel would better indicate what it is.

Similarly, dayType is a dictionary storing the types of time-off requests, and it is a field, so I would call it something like _timeRequestTypes.

Finally, the class name itself, OtherTimeTypes reads more like the type contains a list of possible time types that fall outside a standard set of types.

However, it's telling you the aggregate number of comp, bereavement, and jury duty hours requested for a particular user over the current year.  I'm pretty bad at naming myself, though, so perhaps someone else can come up with some good alternatives.

Design

The first thing I see is that the dayType dictionary only seems to exist as a way of identifying the int values the database uses.  This would actually be much simpler as an enum value:

public enum TimeRequestType
{
    BereavementHours = 3,
    JuryDutyHours = 4,
    CompHours = 7,
}

The next thing I see is that you have several discrete properties to store different types of hours, but you are using a dictionary to do the same thing.  You could just as easily add directly to the properties, or with some method extraction, you could perform a Sum in your LINQ.

Another thing I see is that you are hard-coding the year to be the current year.  I would instead pass that as a parameter.  Sure, the 99% case is that you are going to ask for the current year, but it is trivial to parameterize that, and you'll be glad you did if you ever have to get the values for not-this-year.

The last major thing I see is two-fold:

OtherTimeTypes both stores the data values returned for a user as well as performs the entity look-ups required to retrieve that data
All work is being done in a constructor

For this, I would first extract a data object class to store the values we are interested in:

public class UserTakenHours
{
    public UserTakenHours(double compHours, double bereavementHours, double juryDutyHours)
    {
        TakenCompHours = compHours;
        TakenBereavementHours = bereavementHours;
        TakenJuryDutyHours = juryDutyHours;
    }

    public double TakenCompHours { get; }
    public double TakenBereavementHours { get; }
    public double TakenJuryDutyHours { get; }
}

Then, I would move the logic for retrieving the data and aggregating it into a static method that returned the data object, along with some helper functions.

First, you can extract a method for calculating the hours in a particular TimeRequest:

private static double GetRequestedTime(TimeRequest request)
{
    return (((request.eDateTime - request.sDateTime).TotalDays + 1) * (int)request.HoursRequested);
}

With that, you can condense the foreach loop on your requests into a single-line LINQ statement:

result.Select(GetRequestedTime).Sum();

I would then extract the body of your loop on dayType into its own method, taking in arguments for the entities, username, year, and type:

private static double CalculateRequestedTime(PersonnelManagementEntities personnel, string user, int year, TimeRequestType type)
{
    var requests = personnel.TimeRequests
                            .Include(t => t.User)
                            .Where(t => t.DayTypeID == (int)type)
                            .Where(t => t.User.ADUserName == user)
                            .Where(t => t.UserSubmitDate.Year == year)
                            .ToList();

    return requests.Select(GetRequestedTime).Sum();
}

And finally, you can change the constructor into a static method that returns the data object:

public static UserTakenHours GetRequestedHours(string user, int year)
{
    var personnel = new PersonnelManagementEntities();
    var compHours = CalculateRequestedTime(personnel, user, year, TimeRequestType.CompHours);
    var bereavementHours = CalculateRequestedTime(personnel, user, year, TimeRequestType.BereavementHours);
    var juryDutyHours = CalculateRequestedTime(personnel, user, year, TimeRequestType.JuryDutyHours);

    return new UserTakenHours(compHours, bereavementHours, juryDutyHours);
}


Reactjs Tab Component I made a simple bare bones Tab component, i'm a beginner when it comes to ReactJS so any advice regarding the codes functionality is greatly appreciated.

Tab.js Component

import React from 'react';

class Tabs extends React.Component {

    constructor() {

        super();

        this.state = {
            activeIndex : 0
        }
    }

    handleOnClick(key, event) {

        event.preventDefault();

        this.setState({
            activeIndex : key
        });
    }

    renderNavItem(key) {

        let tab = this.props.children[key];

        return (

                { tab.props.title }

        );
    }

    render() {

        let index = 0;
        let active = this.state.activeIndex;

        let tabs = React.Children.map(this.props.children, function (child) {
            return React.cloneElement(child, {
                active : child.props.active === true ? true : (active == index++)
            });
        });

        return (

                    { Object.keys(this.props.children).map(this.renderNavItem.bind(this)) }

                    { tabs }

        )
    }
}

class Tab extends React.Component {

    render() {

        return (

                { this.props.children }

        )
    }
}

Tab.defaultProps = {
    active : false
};

export default {
  Tabs,
  Tab
};

Usage

import React from 'react';
import {Tabs, Tab} from './Tabs';

class App extends React.Component {

    render() {

        return (
            <Tabs className="tabs-wrapper">
                <Tab active="true" title="Tab One">Tab One content</Tab>
                <Tab title="Tab Two">

Tab Two Content

                </Tab>
            </Tabs>
        );
    }
}

React.render(
    <App/>,
    document.getElementById('react_example')
);

Example

http://jsbin.com/hajokofavu/edit?js,output

As there's not all that much code here to review, I've reviewed some style points:

You have some strange use of whitespace throughout your code:

handleOnClick(key, event) {

    event.preventDefault();

    this.setState({
        activeIndex : key
                // ^-- whitespace shouldn't be before a property colon
    });
}

and not necessarily that it's wrong, but there's a lot of empty whitespace lines that make your program look a lot beefier than it should.

child.props.active === true ? true : (active == index++)

You don't need to compare properties to booleans as simply specifying the variable without comparison performs a boolean comparison:

var thing = true;
console.log(thing === true ? 1 : 2); // identical
console.log(thing ? 1 : 2);          // identical

You've also got some inconsistency in your use of semicolons:

constructor() {

    super();

    this.state = {
        activeIndex : 0
    }
}


Google Crawler. Searches the 100 first google results for some term I wrote a simple Google Crawler in PHP, and my GoogleProxy interface does not seem to follow the Single Responsibility Principle.

Here's my Interface code:

<?php
/**
 * Interface that represents an online proxy service.
 * Every class that implement it must know how to return the apropriate ResponseInterface to an url based on what
 * the proxy service needs, and must know how to parse an url used within its pages.
 *
 * @package CViniciussDias\GoogleCrawler\Proxy
 * @author Vinicius Dias
 */
interface GoogleProxy
{
    /**
     * Gets the ResponseInterface for the informed URL based on all the information that
     * the proxy service needs
     *
     * @param string $url
     * @return ResponseInterface
     */
    public function getHttpResponse(string $url): ResponseInterface;

    /**
     * Parses an URL based on how they are encoded in the proxy service
     *
     * @param string $url
     * @return string
     */
    public function parseUrl(string $url): string;
}

And one of its implementations:

<?php
namespace CViniciusSDias\GoogleCrawler\Proxy;

use CViniciusSDias\GoogleCrawler\Exception\InvalidResultException;
use GuzzleHttp\Client;
use Psr\Http\Message\ResponseInterface;

/**
 * Class that represents the absense of a proxy service, making the direct request to the url
 * and returning its response
 *
 * @package CViniciussDias\GoogleCrawler\Proxy
 * @author Vinicius Dias
 */
class NoProxy implements GoogleProxy
{
    public function getHttpResponse(string $url): ResponseInterface
    {
        return (new Client())->request('GET', $url);
    }

    public function parseUrl(string $url): string
    {
        // Separates the url parts
        $link = parse_url($url);
        // Parses the parameters of the url query
        parse_str($link['query'], $link);

        $url = filter_var($link['q'], FILTER_VALIDATE_URL);
        // If this is not a valid URL, so the result is (probably) an image, news or video suggestion
        if (!$url) {
            throw new InvalidResultException();
        }

        return $url;
    }
}

Here's the full code if needed (I would enjoy some pull requests too):
https://github.com/CViniciusSDias/google-crawler

Should I split the two tasks it performs into 2 separate classes? Several other Proxies will be implemented in a near future. Wouldn't it make the number of classes increase more than necessary?

Some thoughts:

Usually when naming an interface, you want to give some hint as to what actions that interface describes.  Here I don't quite understand what implementing GoogleProxy should be able to do for the implementing class.  What do getting a response and parsing a URL have to do with a proxy? What do these methods have to do with Google specifically? Would this interface better be named as HttpClient or something like that which indicates that these are the methods that need to be implemented by a class to enable the request/response aspects of crawling?
You might consider adding @throws in your interface documentation for parseUrl().  Not that this really does anything, but perhaps at least it can drive code consistency in having implementing classes throw appropriate exceptions and might help when working in an IDE.
In your getHttpResponse() implementation, you are doing nothing to either handle underlying exceptions that can be thrown by the Guzzle Client or indicate that the function might throw (why no doc blocks in implementing class?).
You should namespace your interface file.
In your parseUrl(), I think it is problematic that you are parsing the query string into the same array that you put your parse_url() results into.  Why not just create a new array here to eliminate confusion over what your intent is and to eliminate any possibility of overwriting date in $link() (if you ever wanted to do anything else with that data.
Should you really throw an exception if the URL value in the query string is not valid? I would think that this could be an expected occurrence that you might need to handle as you have no control over this input. Does this really represent an unexpected state in your application as is often the intended use for throwing exceptions?
Should you add some validation for the parametric input to both of these functions (i.e. non-zero length string, properly formed URL). In your getHttpResponse() implementation you can get as far as attempting to make a (relatively expensive) call on a remote host even if you have an empty string or invalid URL passed to the method.  I generally think it good practice to enforce some basic level of validation on parameters passed to public methods, perhaps throwing InvalidArgumentException or similar before getting into method logic such that you are failing fast and failing loudly when bad input is passed.
From looking at your wider library on Github, it seems as if you are trying to work with dependency injection. If you want to be consistent in this approach, I would think your implementing classes would need to take on dependencies such as the Guzzle client in their constructors, as opposed to within the methods that are implementing the interface.
With the above thought in mind, I am actually not sure that what you have right now is even best modeled as an interface vs. class inheritance since all the proxy classes you currently have implemented don't really do much outside of implement these two methods.


Move files to specific directory, according to file extension I've written the following script

import shutil,os,glob

# setting up destination folders

PYDEST = "/home/ubuntu/python-scripts/python-test"

TEXTDEST = "/home/ubuntu/python-scripts/texte-test"

def move8files(pwd):
    PWD = pwd + "*"
    for files in glob.glob(PWD):
        if files.endswith(".txt"):
            print(files)
            shutil.move(files,TEXTDEST)
        elif files.endswith(".py"):
            shutil.move(files,PYDEST)

move8files("/home/ubuntu/python-scripts/")

Questions/Issues are the following:

How can I make it simpler, especially by using the os.listdir but with having the pathname (listdir does not give pathname)?
Would it be possible to include a part in my script, where instead of saying which folders are for python or text files, it will look syntaxically at specific folders containing words like python or texte in it?
What can I improve in my script, for better performance?

I have no idea, how to make it simpler. I could only offer an alternative:

#!/usr/bin/env python

import os
import shutil

fullpath = os.path.join
python_directory = "./py"
start_directory = "./mixed"
text_files = "./txt"

def main():
    for dirname, dirnames, filenames in os.walk(start_directory):
        for filename in filenames:
            source = fullpath(dirname, filename)
            if filename.endswith("py"):
               shutil.move(source, fullpath(python_directory, filename))
            elif filename.endswith("txt"):
                shutil.move(source, fullpath(text_files, filename))

if __name__ == "__main__":
    main()

I think os.walk()is what you are looking for.


Indeterminate progress bar with Raspberry Pi I am writing program for Raspberry Pi that will show indeterminate progress while user waiting. The program will start automatically and will be killed by another shell script.

Here is what I have at the moment. It works, but I feel that it is not perfect. I will be changing this code frequently after users will give their feedback.

#include <wiringPi.h>
#include <stdio.h>

#define PINS_NUMBER 8

void switchPin(int pin, int status, int timeout) {
  digitalWrite(pin, status);
  delay(timeout);
}

void switchLine(int pins[], int status) {
  int i;
  for (i = 0; i < PINS_NUMBER; i++) {
    switchPin(pins[i], status, 200);
  }
}

int main(void){
  wiringPiSetup();

  int leds[PINS_NUMBER] = {0,1,2,3,4,5,6,7};

  int i;
  for (i = 0; i < PINS_NUMBER; i++) {
    pinMode(leds[i], OUTPUT);
  }

  while(1) {
    switchLine(leds, LOW);
    switchLine(leds, HIGH);
  }
  return 0;
}

Naming

For first, your code looks good, it's not so hard to understand.

  It works, but I feel that it is not perfect.

There is no perfect code. A code is evolving, a code is changing. So, instead of chasing perfection, you should pursue such things like readability.

Your function names are a bit confusing. For example without knowing the body of the switchLine function it's hard to firgure out that it will change the state of several pins sequentially with some delay.

How about a main() function like the following?

int main() {
    struct PinLedMap connections = {{
        { 0, LED_0 }, { 1, LED_1 }, { 2, LED_2 }, { 3, LED_3 },
        { 4, LED_4 }, { 5, LED_5 }, { 6, LED_6 }, { 7, LED_7 }
    }};
    assignPinsToLeds(&connections);
    initialize();

    setDelayOfSequentialOperations(250);
    while(1) {
        turnLedsOnSequentially();
        turnLedsOffSequentially();
    }
    return 0;
}

I think that the code above is clear enough, and easy to understand.

Responsibility

I don't see the reason why do the switchPin function applies the delay? The switchPin function should switch the state of specified pin only. Applying delays between turning LEDs on or off is the responsibility of your switchLine function.

In my example the turnLedsOnSequentially and the turnLedsOffSequentially functions work as follows:

void setLedsSequentiallyTo(enum LedState state) {
    int index;
    for(index = 0; index < NUMBER_OF_LEDS; ++index) {
        setLedTo(index, state);
        delay(_delayInMilliseconds);
    }
}
void turnLedsOnSequentially() {
    setLedsSequentiallyTo(LED_ON);
}
void turnLedsOffSequentially() {
    setLedsSequentiallyTo(LED_OFF);
}

Complete code

I took my example into pieces, so it contains a header file (led_control.h) and two source files (led_control.c, main.c). I've presented the main.c before, so I won't present it again.

led_control.h

#ifndef LED_CONTROL_H
#define LED_CONTROL_H

enum LedNumber {
    LED_0, LED_1, LED_2, LED_3, LED_4, LED_5, LED_6, LED_7,
    NUMBER_OF_LEDS
};
struct PinLedPair {
    int pinNumber;
    enum LedNumber ledNumber;
};
struct PinLedMap {
    struct PinLedPair elements[NUMBER_OF_LEDS];
};

void assignPinToLed(struct PinLedPair* connection);
void assignPinsToLeds(struct PinLedMap* connections);
void initialize();
void setDelayOfSequentialOperations(int delayInMilliseconds);
void turnLedOn(enum LedNumber ledNumber);
void turnLedOff(enum LedNumber ledNumber);
void turnLedsOnSequentially();
void turnLedsOffSequentially();

#endif /* LED_CONTROL_H */

led_control.c

#include "led_control.h"
#include <wiringPi.h>
#include <assert.h>

int _leds[NUMBER_OF_LEDS] = { 0 };
int _delayInMilliseconds = 100;

enum LedState {
    LED_OFF = LOW, LED_ON = HIGH
};

void assignPinToLed(struct PinLedPair* connection) {
    assert(connection);
    _leds[connection->ledNumber] = connection->pinNumber;
    pinMode(connection->pinNumber, OUTPUT);
}
void assignPinsToLeds(struct PinLedMap* connections) {
    int index;
    assert(connections);
    for(index = 0; index < NUMBER_OF_LEDS; ++index)
        assignPinToLed(&connections->elements[index]);
}

void initialize() {
    wiringPiSetup();
}
void setDelayOfSequentialOperations(int delayInMilliseconds) {
    _delayInMilliseconds = delayInMilliseconds;
}

void _setLedTo(enum LedNumber ledNumber, enum LedState state) {
    digitalWrite(_leds[ledNumber], state);
}
void turnLedOn(enum LedNumber ledNumber) {
    _setLedTo(ledNumber, LED_ON);
}
void turnLedOff(enum LedNumber ledNumber) {
    _setLedTo(ledNumber, LED_OFF);
}

void _setLedsSequentiallyTo(enum LedState state) {
    int index;
    for(index = 0; index < NUMBER_OF_LEDS; ++index) {
        _setLedTo(index, state);
        delay(_delayInMilliseconds);
    }
}
void turnLedsOnSequentially() {
    _setLedsSequentiallyTo(LED_ON);
}
void turnLedsOffSequentially() {
    _setLedsSequentiallyTo(LED_OFF);
}


Prime Factorisation in Swift I made a playground to try find prime factors of any given number, and it works, and I'm happy with the first function, even if not correctly named - I don't know what to name it.

My main need for improvement is in the second half. I can't for the life of me think of a way of looping through a list until the functions output is constant. I thought of recursion, but I didn't understand it. This is what I came up with, and I'd like to see how I can improve it because it's sloppy and ugly.

import UIKit

func primeFact(tree: [Int]) -> Array<Int> {
    var newTree = tree
    for element in newTree{
        for divisor in 2..<element{
            if element%divisor == 0{
                newTree = newTree.filter { $0 != element}
                newTree+=[(element/divisor),divisor]
                break
            }
        }
    }
    return newTree
}

var initial = primeFact(tree: [992])
var temp = [Int]()
while true {
    if primeFact(tree: initial) == initial{
        break
    }
    temp = primeFact(tree: initial)
    initial = temp
}
print(primeFact(tree: initial))

To address your "main need" first: Your algorithm starts with a
single-element array, and then repeatedly calls primeFact() to
compute a new array, until the array is "constant". That can
be done more clearly as

var initial = [992]
var temp = [Int]()
repeat {
    temp = initial
    initial = primeFact(tree: temp)
} while initial != temp
print(initial)

However, your algorithms seems to be highly inefficient, for
the following reasons:

Since the trial division starts with the lowest possible divisors,
each found divisor is necessarily a prime number. But the
next call to primeFact() will again try to find divisors of that
number.
All calls to primeFact() will try all numbers starting from 2
as divisors for all elements in the "tree". For example, if the
current list is [2, 2, <someOddNumber>] then each call will
again try to divide <someOddNumber> by 2.
A lot of intermediate arrays are created.
Possible large arrays must be compared in order to determine if
the factorization is done.

Additional remarks:

Put spaces around operators, e.g. element % divisor for
better readability.
Use either [Int] or Array<Int> for array notation (I prefer
the first), but don't mix it.
Calling the parameter tree is confusing because you treat
it as an array, not as a tree.

A more efficient approach is to divide the given number by 2, 3, 4, ...
As soon as a factor is found, the number is divided by this factor.
Using the fact that composite number \$ n > 1 \$
must have a prime factor \$ p \$ for which  \$ p \le \sqrt n \$,
this leads to the following function:

func primeFactors(_ n: Int) -> [Int] {
    var n = n
    var factors: [Int] = []

    var divisor = 2
    while divisor * divisor <= n {
        while n % divisor == 0 {
            factors.append(divisor)
            n /= divisor
        }
        divisor += divisor == 2 ? 1 : 2
    }
    if n > 1 {
        factors.append(n)
    }

    return factors
}

As another small optization, only 2 and all odd numbers are used
as trial divisors.

Performance comparison:

For \$ N = 1000000000000 = 2^{12} \cdot 5^{12} \$:
Your tree factorization: 1ms. Direct factorization: 0.005 ms.
For \$ N = 1000000000001 = 73 \cdot 137 \cdot 99990001 \$:
Your tree factorization: 1,100ms. Direct factorization: 0.1 ms.

The tests were done on a 1.2 GHz Intel Core m5 MacBook, with the
code compiled in Release mode.


Loading data from a remote app I have a method which loads data from a remote app (send TCP request and parse response). Now, I have a simple class for sending a TCP request:

public class PremieraTcpClient
    {
        public PremieraTcpClient()
        {
            QueryItems = new NameValueCollection();
            int port;
            int.TryParse(ConfigurationManager.AppSettings["PremieraPort"], out port);
            Port = port;
            ServerIp = ConfigurationManager.AppSettings["PremieraServerIp"];
            ServiceId = ConfigurationManager.AppSettings["PremieraServiceId"];
        }
        public NameValueCollection QueryItems { get; set; }

        private int Port { get; set; }

        private string ServerIp { get; set; }

        private string ServiceId { get; set; }

        private string ReadyQuery { get; set; }

        public string SendQuery()
        {
            StringBuilder parameters = new StringBuilder();

            //...
            // build query for request
            //...
            ReadyQuery = parameters.ToString();
            return Connect();
        }

        private string Connect()
        {
            string responseData;

            try
            {
                TcpClient client = new TcpClient(ServerIp, Port);
                client.ReceiveBufferSize = Int32.MaxValue;

                Byte[] data = Encoding.GetEncoding(1251).GetBytes(ReadyQuery);

                NetworkStream stream = client.GetStream();

                // send data
                stream.Write(data, 0, data.Length);

                var sizeBuffer = new byte[10];

                stream.Read(sizeBuffer, 0, 10);
                var sizeMessage = int.Parse(Encoding.GetEncoding(1251).GetString(sizeBuffer, 0, 10));

                 data = new Byte[sizeMessage];

                var readSoFar = 0;
                //read data
                while (readSoFar < sizeMessage)
                {
                    readSoFar += stream.Read(data, readSoFar, data.Length - readSoFar);
                }

                responseData = Encoding.GetEncoding(1251).GetString(data, 0, data.Length);
                responseData = responseData.TrimStart('&');
                stream.Close();
                client.Close();
                return responseData;
            }
            catch (ArgumentNullException e)
            {
                //return responseData = string.Format("ArgumentNullException: {0}", e);
            }
            catch (SocketException e)
            {
                //return responseData = string.Format("SocketException: {0}", e);
            }
            return string.Empty;
        }
    }

This is method for load data:

 private static void GetUpdatesFromPremiera()
        {
            Debug.WriteLine(DateTime.Now + ":GetUpdatesFromPremiera");

            PremieraTcpClient client = new PremieraTcpClient();
            client.QueryItems.Add("QueryCode", QueryCode.GetUpdates.ToString());
            client.QueryItems.Add("ListType", "Movie;Hall;Session;Place;Delete");
            client.QueryItems.Add("Updates", _lastUpdateId);
            _lastUpdateId = String.Empty;
            var response = client.SendQuery();

            // here parse response
            //...
        }

This code works fine. But, now I have to load data from two remote app (tomorrow may be three).

The simple solution is to iterate through all remote apps:

private static void GetUpdatesFromPremiera()
{
   foreach(var remoteApp in listRemoteApp)
   {
        PremieraTcpClient client = new PremieraTcpClient();
        // here assigned different properties
        var response = client.SendQuery();
   }
}

Is there is a better way of doing it? Also, each time a connection is established, I think it impacts performance greatly.

I have some suggestions about PremieraTcpClient. The way it is written may lead to unreleased resources. If you have an error then you will remain with a stream and client  opened.
The correct way to do it is by using try...catch...finally or by using using.
Below you can find the code using try catch finally

    private string Connect()
    {
        string responseData = string.Empty;
        TcpClient client = null;
        NetworkStream stream = null;
        try
        {
            client = new TcpClient(ServerIp, Port);
            client.ReceiveBufferSize = Int32.MaxValue;

            Byte[] data = Encoding.GetEncoding(1251).GetBytes(ReadyQuery);

            stream = client.GetStream();

            // send data
            stream.Write(data, 0, data.Length);

            var sizeBuffer = new byte[10];

            stream.Read(sizeBuffer, 0, 10);
            var sizeMessage = int.Parse(Encoding.GetEncoding(1251).GetString(sizeBuffer, 0, 10));

            data = new Byte[sizeMessage];

            var readSoFar = 0;
            //read data
            while (readSoFar < sizeMessage)
            {
                readSoFar += stream.Read(data, readSoFar, data.Length - readSoFar);
            }

            responseData = Encoding.GetEncoding(1251).GetString(data, 0, data.Length);
            responseData = responseData.TrimStart('&');
            //stream.Close();
            //client.Close();
            //return responseData;
        }
        catch (ArgumentNullException e)
        {
            //return responseData = string.Format("ArgumentNullException: {0}", e);
        }
        catch (SocketException e)
        {
            //return responseData = string.Format("SocketException: {0}", e);
        }
        finally
        {
            if(stream!=null) stream.Close();
            if(client!=null) client.Close();
        }

        return responseData;
    }

And another small suggestion: I think is misleading to have a method named Connect that in fact does more than connect. It will be better to break the Connect method into smaller methods, each one with specific actions (even if they are private).


Search for a symmetrical inner portion of a list What the code does: Takes a Python list of integers as input and searches for a 'symmetrical' inner-portion of the list. Example:

symmetrical_sum([10,8,7,5,9,8,15]) == ([8, 7, 5, 9, 8], 37)

Note: Symmetry occurs if the value of the ith element from the start of the list is equal to the value of the ith element from the end of the list.

def symmetrical_sum(a):
'''Takes a Python list of integers as input and searches for a 'symmetrical' inner-portion of the list

Example: symmetrical_sum([10,8,7,5,9,8,15]) == ([8, 7, 5, 9, 8], 37)

Symmetry occurs if the value of the ith element at the start of the list is equal to the value of the
ith element at the end of the list'''

#extract duplicate value
dupe = [x for n, x in enumerate(a) if x in a[:n]]

#if no duplicate values found, do the following:
if dupe == []:
    middle = float(len(a))/2
    if middle % 2 != 0:
        sym = a[int(middle - .5):int(middle + .5)]
        ans = a[int(middle - .5)]
        tuple1 = (sym,ans)
    elif middle % 2 == 0:
        sym = a[int(middle - 1):int(middle + 1)]
        ans = sum(a[int(middle - 1):int(middle + 1)])//2
        tuple1 = (sym,ans)
    return tuple1
else:
    d_to_i = int("".join(map(str, dupe))) #convert duplicate value to integer
    p1 = a.index(d_to_i) #get index of first duplicate
    p2 = a.index(d_to_i, p1+1) #get index of second duplicate
    sym = a[p1:p2+1] #[symmetrical-portion]
    ans = sum(sym) #sum-of-symmetrical-portion
    tuple2 = (sym, ans)

return tuple2

Looking at my code I realize that it could probably be more efficient than this. It works, but I'm sure it could be better. I just don't possess the skills to 'compress it'. I've submitted my code already so this is just for my own personal development.

The code in the question is not working as intended.

  Symmetry occurs if the value of the ith element at the start of the list is equal to the value of the
  ith element at the end of the list

Meaning for any list l the symmetry occurs when l[i] == l[~i].

>>> symmetrical_sum([1, 2, 1, 3, 4])
([1, 2, 1], 4)
>>> symmetrical_sum([0, 1, 2, 1, 3, 4])
([1, 2, 1], 4)

This says that the midpoint is the same distance from the end of the list as the starting value. And it says 1 and -3 are the same distance.
No where does it say the input will only have one duplicate.

>>> symmetrical_sum([1, 2, 3, 4, 2, 1])
ValueError: 21 is not in list
>>> symmetrical_sum([1, 2, 3, 4, 1, 2])
ValueError: 12 is not in list
>>> symmetrical_sum([1, 2, 3, 1, 4, 2])
ValueError: 12 is not in list

The output returns different amount of values when the size of the input are different multiples of two.

>>> symmetrical_sum([1, 2])
([1], 1)
>>> symmetrical_sum([1, 2, 3, 4])
([2, 3], 2)
>>> symmetrical_sum([1, 2, 3, 4, 5, 6])
([3], 3)
>>> symmetrical_sum([1, 2, 3, 4, 5, 6, 7, 8])
([4, 5], 4)
>>> symmetrical_sum([1, 2, 3, 4, 5, 6, 7, 8, 9, 10])
([5], 5)


Function that checks for truth value I was writing a function that takes an argument and returns a truth value.

My initial idea was just return !!value, but this will fail for '0', {} or []. I know that '0' should be true but in my application we are considering it false.

So, my function should return false for:

Falsey values: false, 0, undefined, null, ''
Blank values: {}, []
Special requirement: '0'

So considering it, I updated code to following to handle

var isTrue = function(value) {
  switch (typeof(value)) {
    case "string":
      return !(value === '0' || value === 'false');
    case "number":
    case "boolean":
      return !!value;
    case "object":
      return Object.keys(value).length > 0
  }
}

console.log(false,isTrue(false))
console.log(true,isTrue(true))
console.log(0,isTrue(0))
console.log(1,isTrue(1))
console.log('0',isTrue('0'))
console.log('1',isTrue('1'))
console.log('test',isTrue('test'))
console.log('Object',isTrue({}))
console.log('Array',isTrue([]))

Now this works fine. It does not include handling for functions but that is out of scope of current question. My question is, is there a better way to handle the same?

I thought it might be faster to:

first merely evaluate the value "standard" truth with !!value
then add conditions for your special requirements (including 'false', not cited in your question but present in your code)

This way, it also reduces the code, like this:

var isTrue = function(value) {
  return (!!value && value !== '0' && value !== 'false'
    && !!(typeof value !== 'object' || Object.keys(value).length));
}

console.log(false,isTrue(false));
console.log("'false'",isTrue('false'));
console.log(true,isTrue(true));
console.log(0,isTrue(0));
console.log(1,isTrue(1));
console.log("'0'",isTrue('0'));
console.log("'1'",isTrue('1'));
console.log("'test'",isTrue('test'));
console.log('{}',isTrue({}));
console.log('[]',isTrue([]));
console.log('Object',isTrue({a: 1}));
console.log('Array',isTrue([1]));

Now we can observe that it works more than 2 times faster.
Here an execution of the same cases set as above, repeated 100 times:

var isTrue = function(value) {
  switch (typeof(value)) {
    case "string":
      return !(value === '0' || value === 'false');
    case "number":
    case "boolean":
      return !!value;
    case "object":
      return Object.keys(value).length > 0
  }
}

var isTrue_2 = function(value) {
  return (!!value && value !== '0' && value !== 'false'
    && !!(typeof value !== 'object' || Object.keys(value).length));
}

var values = [false, 'false', true, 0, 1, '0', '1', 'test', {}, [], {a: 1}, [1]],
    times = 100;

console.time('isTrue');
for (var i = 0; i < times; i++) {
  for (var value of values) {
    isTrue(value);
  }
}
console.timeEnd('isTrue');

console.time('isTrue_2');
for (var i = 0; i < times; i++) {
  for (var value of values) {
    isTrue_2(value);
  }
}
console.timeEnd('isTrue_2');

Last point of interest: if you can have some statistics about how much the special requirements appear, you may change the order of the conditions to put the most frequent cases first, so yet improving performance.


Circular Array - OOP I have a solution for a circular array. How can I improve this?

package oopdesign.CircularArrayNes;

import java.util.Iterator;
import java.util.Spliterator;
import java.util.function.Consumer;

public class ArrayIterator<T> implements Iterable<T> {

    @Override
    public Iterator<T> iterator() {
        return null;
    }

    @Override
    public void forEach(Consumer<? super T> action) {

    }

    @Override
    public Spliterator<T> spliterator() {
        return null;
    }
}

package oopdesign.CircularArrayNes;

public class CircularArray<T> {

    protected T[] array;
    protected int limit;
    protected int pointer;
    protected int index;

    protected CircularArray(){
        limit = 5;
        T[] array = (T[])new Object[limit];
        pointer = 0;
        index = 0;
    }

    public void add(T data){
        index = getCurrentIndex() + 1;
        array[index] = data;
        pointer++;
    }

    public int getCurrentIndex()
    {
        return (pointer) % limit;
    }

    public void delete(T data){
        // 4 5 9 11 1
        // delete 9
        int deleteIndex = findIndex(data);
        if (deleteIndex == -1) return;
        for(int i=deleteIndex;i< array.length-1;i++) {
            array[deleteIndex] = array[deleteIndex+1];
        }
        pointer-- ;
    }

    private int findIndex(T data) {
        for(int i=0;i< array.length;i++) {
          if (array[i] == data){
              return i;
          }
        }
        return -1;
    }

}

The Iterator is a second stage provision for API users. As such I will not comment on it.
The package name should not contain capitals. It is only a convention, but for java it is maintained relatively strict in the community, due to the hundreds of libraries.
Using T[] or rather Object[] requires some disregard to typing. I solved that by passing the class of T: Integer.class or such. Alternatively one could use ArrayList<T> instead of T[]. Then there is full generic typing.
protected is a dubious choice for such a container class, but maybe a next task would be to add functionality in the form of a child class.
Now to the critics:

limit should rather be a parameter in the constructor, made two constructors, one with a default limit. As limit is redundant, equal to array.length you need not have it as field.
You can use final for unchanging fields.
index and pointer are no-names, often unsuited for fields.
No display of really using circularity, start index and end index running around.
Delete in the middle is atypical for a circular array, should not go from 0 to length, and should actually delete 9 in 3 9 5.
Here I represented circularity by a size to distinghuish empty from full, and a start index consumeIndex and an end index produceIndex. Modulo array.length is needed.
findIndex used == but the Object wrappers are hideous: Integer.valueOf(3) == Integer.valueOf(3) (internal cache upto 128) but Integer.valueOf(300) == Integer.valueOf(300). So use equals.

So (not guaranteeing correctness):
import java.lang.reflect.Array;

public class CircularArray<T> {

    public static final int DEFAULT_LIMIT = 5;
    private final Class<T> elementType;
    private final T[] array;
    private int consumeIndex = 0;
    private int produceIndex = 0;
    private int size = 0;

    public CircularArray(Class<T> elementType) {
        this(elementType, DEFAULT_LIMIT);
    }

    public CircularArray(Class<T> elementType, int limit) {
        this.elementType = elementType;
        array = (T[]) Array.newInstance(elementType, limit);
    }

    public void add(T data) {
        if (size == array.length) {
            throw new IllegalStateException("CircularArray is full");
        }
        array[produceIndex++] = data;
        if (produceIndex >= array.length) {
            produceIndex = 0;
        }
        size++;
    }

    public void delete(T data) {
        // 4 5 9 11 1
        // delete 9
        int deleteIndex = findIndex(data);
        if (deleteIndex == -1) {
            return;
        }
        for (int index = deleteIndex; (index + 1) % array.length != produceIndex; ++index) {
            array[index] = array[(index + 1) % array.length];
        }
        produceIndex = (produceIndex - 1 + array.length) % array.length;
        --size;
    }

    private int findIndex(T data) {
        for (int i = 0; i < size; i++) {
            int index = (consumeIndex + i) % size;
            if (array[index].equals(data)) {
                return index;
            }
        }
        return -1;
    }

}

Some test code (as requested by comment)
CircularArray<Integer> ca = new CircularArray<>(Integer.class);
ca.add(1);
ca.add(2);
ca.add(3);
ca.add(4);
ca.add(5);
try {
    ca.add(6);
    throw new IndexOutOfBoundsException("Overflow expected");
} catch (IllegalStateException e) {
    System.out.println("Expected overflow");
}
int i = ca.delete(3);
if (i != 2) {
    throw new IllegalStateException();
}
ca.delete(1);
ca.add(7);
ca.add(8)
int i = ca.findIndex(8);
if (i != 0) {
    throw new IllegalStateException();
}

This is not real testing, more demo, and unit tests are a different thing, with many scenarios. Also note that the code can be optimized, for instance for deleting the first element:
    public void delete(T data) {
        // 4 5 9 11 1
        // delete 9
        int deleteIndex = findIndex(data);
        if (deleteIndex == -1) {
            return;
        }
        // Optimisation:
        if (deleteIndex == consumeIndex) {
            consumeIndex = (consumeIndex + 1) % array.length;
            return;
        }


Remove nodejs/browser specific code in library I'm the maintainer of the following library on github: https://github.com/edi9999/docxtemplater/blob/master/coffee/docxgen.coffee

I want to maintain a library that works on node and in the browser, however I don't want to have to update two separate repositories.

The ugliest code looks like this:

root= global ? window
env= if global? then 'node' else 'browser'

if env=='node'
    global.http= require('http')
    global.https= require('https')
    global.fs= require('fs')
    global.vm = require('vm')
    global.DOMParser = require('xmldom').DOMParser
    global.XMLSerializer= require('xmldom').XMLSerializer
    global.PNG= require('../vendor/pngjs/png-node')
    global.url= require('url')

    ["grid.js","version.js","detector.js","formatinf.js","errorlevel.js","bitmat.js","datablock.js","bmparser.js","datamask.js","rsdecoder.js","gf256poly.js","gf256.js","decoder.js","qrcode.js","findpat.js","alignpat.js","databr.js"].forEach (file) ->
        vm.runInThisContext(global.fs.readFileSync(__dirname + '/../vendor/jsqrcode/' + file), file)
    ['jszip.js'].forEach (file) ->
        vm.runInThisContext(global.fs.readFileSync(__dirname + '/../vendor/jszip2.0/dist/' + file), file)

output: (options={}) ->
        if !options.download? then options.download=true
        if !options.name? then options.name="output.docx"
        if !options.type? then options.type="base64"
        result= @zip.generate({type:options.type})
        if options.download
            if env=='node'
                fs.writeFile process.cwd()+'/'+options.name, result, 'base64', (err) ->
                    if err then throw err
            else
                #Be aware that data-uri doesn't work for too big files: More Info http://stackoverflow.com/questions/17082286/getting-max-data-uri-size-in-javascript
                document.location.href= "data:application/vnd.openxmlformats-officedocument.wordprocessingml.document;base64,#{result}"
        result

As you can see, they is a lot of specific node/browser js code, and I would like to remove that from the main code, however, I can't find an easy way to do this.

Any idea ?

Yeah..

Are you against maintaining 2 repositories or maintaining 2 main files? Honestly I would have 2 main files with a third file ( or more ) containing routines that are host agnostic.

docxgen_node.coffee , docxgen_www.coffee and docxgen_common.coffee

It sounds uncool, but I think in the end, this should be easiest on the eyes.

Other, than that, I would take the opportunity to use that massive ( ugly ) string array to document why you need each library and maybe which versions are usable for your repository:

var requiredLibraries = [
  "grid.js",     //Use xyz version, uses abc features
  "version.js",  //...
  "detector.js", //..
  ...
]

requiredLibraries.forEach (file) ->
    vm.runInThisContext(global.fs.readFileSync(__dirname + '/../vendor/jsqrcode/' + file), file)


Drawing a roaming figure in a field, with less flickering I'm a student to programming. I currently just working a thing where the player can walk around a field of hashtags. so far you can only walk horizontally but before i go any further, i realized that the program flickers a lot? Is there any cure for this? I'm using python 3 on windows.

import os
from msvcrt import getch

def clear():
    os.system( 'cls' )

map1=("#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#")
def draw_setting(x):
    edit_map=list(map1)
    del edit_map[x]
    edit_map.insert(x,"o") #The player is represented by o
    print (*edit_map, sep='')

x=0
draw_setting(x)
while True:
    key = ord(getch()) #Fetching the key pressed
    if key==100: #If d is pressed
        if x<30:
            x=x+1
            load_map=1
    elif key==97: #If a is pressed
        if x>0:
            x=x-1
            load_map=1
    elif key=="stop":
        break
    if load_map==1: #Only render if necessary
        clear()
        draw_setting(x)
        load_map=0

You may want to use the curses library.

def clear():
    os.system('cls')

You complain about "flicker", which is related to perceptible "slowness". Almost certainly your environment supports xterm/vt100 ANSI escape sequences. You forked off a command, cls, that printed <ESC>[H to put cursor at top of screen, and <ESC>[2J to clear screen. You could more quickly print() such escape sequences.

map1=("#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#","#")

Please don't do that. Express it as map1 = '#' * 31

del edit_map[x]
edit_map.insert(x, "o")

Delete and insert both have linear cost, which is small for a 31-char string, but still, avoid turning O(1) ops into O(n) ops. Instead, just assign: edit_map[x] = 'o'.

You can reposition the cursor with a simple CR rather than ANSI sequences: print(edit_map + '\r'). That alone could make the flicker negligible.