Threadsafe oneshot-event which fires on subscription if the event was fired in the past Assumptions

Basically, a Connection class has a "Disconnect" event. Subscribing to this event isn't thread-safe, because the disconnection may fire from another thread right before I subscribe. So checking before the subscription doesn't help.

Checking for the disconnect after subscription doesn't help either because the event may have fired in the meanwhile (2 threads might execute the same "observer" twice).

(My) Solution:

An event always fired once (and only once), even if the event itself already happened before Source is on GitHub as well.

Questions:

Are there other simpler solutions addressing this? (by simpler I mean from an outside or usage perspective)

Do you see any race conditions or things that could go wrong?

Maybe you have optimizations or simplifications to add?

Input is highly appreciated!

/// <summary>
/// Triggers if the event is invoked or was invoked before subscribing to it.
/// <para> Can be accessed safely by multiple threads.</para>
/// </summary>
public class AutoInvokeEvent<Sender, Args>
{
    public delegate void EventHandle(Sender sender, Args arguments);

    /// <summary>
    /// Handle will be invoked if the event was triggered in the past.
    /// <para>Unsubscribing happens automatically after the invocation and is redundant if done from the event handle.</para>
    /// </summary>
    public event EventHandle Event
    {
        add
        {
            if (!Subscribe(value))
                value(m_sender, m_eventArgs);
        }
        remove { InternalEvent -= value; }
    }

    private event EventHandle InternalEvent;

    // this is my personal lock implementation. in this case it is used like any other lock(object) so just ignore it
    private SafeExecutor m_lock = new SingleThreadExecutor();
    private volatile bool m_invoked = false;

    Sender m_sender;
    Args m_eventArgs;

    /// <summary>
    /// Invokes all subscribed handles with the given parameters.
    /// <para>All calls after the first are ignored.</para>
    /// </summary>
    public void Invoke(Sender sender, Args args)
    {
        GetEventHandle(sender, args)?.Invoke(m_sender, m_eventArgs);
    }

    private EventHandle GetEventHandle(Sender sender, Args args)
    {
        return m_lock.Execute(() =>
        {
            if (m_invoked)
                return null;

            m_sender = sender;
            m_eventArgs = args;
            m_invoked = true;
            EventHandle handle = InternalEvent;
            InternalEvent = null;
            return handle;
        });
    }

    /// <returns>Returns true if subscription was successful and false if handle needs to be invoked immediately.</returns>
    private bool Subscribe(EventHandle handle)
    {
        return m_lock.Execute(() =>
        {
            if (!m_invoked)
                InternalEvent += handle;

            return !m_invoked;
        });
    }
}

How the class could be used :

class Connection
{
    public AutoInvokeEvent<object, EndPoint> OnDisconnect = new AutoInvokeEvent<object, EndPoint>();

    public Disconnect()
    {
        OnDisconnect.Invoke(this, endpoint);
    }
}

void main()
{
    Connection connection = new Connection();
    connection.OnDisconnect.Event += DoStuffOnDisconnect;
}

void DoStuffOnDisconnect(object sender, EndPoint endpoint) { }

Thread safety

Both Subscribe and GetEventHandle use lock and and+remove on field event are thread-safe too. You can even remove the volatile from m_invoked because you only ever access it under the lock from those two methods.

API and naming

Event and Invoke are pretty obvious, especially with the description. I would only change the signature of Invoke to return bool (true = invoked, false = not invoked, already done before) to better reflect the fact, that it won't throw exception if you try to invoke it multiple times (could as well be named TryInvoke, maybe changing Invoke to throw exceptions if invoked multiple-times).

GetEventHandle does not sound as good name to me, because it does much more than just getting the handle, it stores sender+args, takes current handle and clears it. I would either use it (and Subscribe) directly where they are used (only one place for each), which also solves naming-problem, or make them local functions.

AutoInvokeEvent hmm, how about OneShotEvent? Although it automatically calls subscribed handlers even after the event was fired, this is more about how it does it under the hood, than what the API is about - it will execute the handler exactly once when it is fired or if it already was fired.

Optimization

I don't think any optimization is necessary. You would have to first specify (and or test/profile) how often is the event subscribed compared to invoking it, to even have some direction in which to optimise. I can see you have switched to using some read-write-lock in the implementation you currently have on GitHub, makes sense (concurrent subscriptions, if that is trully important to you). Could as well be implemented by double-checked-locking, thanks to the fact this is one-shot event, but I don't think it is worth it, too easy to make a mistake, very hard to make it right. (The idea is to have volatile object m_lock and make it null when you fire the event, instead of using that bool m_invoked.)


Generic object pool class I have decided to make small ObjectPool class for internal use. What do you think about it ?

template<class T>
class ObjectPool
{
public:
    inline T& getObject();
    inline void releaseObject(T& obj);
private:
    std::stack<std::reference_wrapper<T>> mFree;
    std::vector<T> mPool;
};

template<class T>
void ObjectPool<T>::releaseObject(T& obj)
{
    mFree.push(obj);
}

template<class T>
T& ObjectPool<T>::getObject()
{
    if (mFree.size()) {
        T& num = mFree.top();
        mFree.pop();
        return num;
    } else {
        mPool.emplace_back();
        return mPool.back();
    }
}

I also want to make all function and data static. Is that considered a bad practice?

First of all, free references will be invalidated each time mPool resizes. Which will lead to all sorts of undefined behavior.

Resizing pool is not a great idea in terms of performance, too. Better have constant size chunks, and add more chunks if needed.

Such a pool implies your user is no more able to get benefits of RAII, which can be a huge loss.

For your question about static methods. It is not a bad practice per se, but your user will no longer be allowed to use different pools for the same type. If you want to make a static method, you better have strong argument to do so.

Overall, consider to use containers+allocators instead of pool.


Optimization of Hibernate DAO in desktop application I'm working on medium sized desktop application (with around 100 tables in database). For persistence layer I decided to use Hibernate for the first time to avoid massive redundancy of code for persistence layer.

This is final implementation of DAO interfaces:

public class BaseDAOImpl<E extends Serializable> implements BaseDAO<E> {

    @Override
    public void insert(E t) {
        Session session = HibernateUtil.openSession();
        try {
            session.beginTransaction();
            session.save(t);
            session.getTransaction().commit();
        } catch (Exception e) {
            e.printStackTrace();

            session.getTransaction().rollback();
        } finally {

            session.close();
        }
    }

    @Override
    public void delete(Class<E> e, int id) {
        Session session = HibernateUtil.openSession();
        try {
            session.beginTransaction();
            Object object = get(id, e);
            if (object != null) {
                session.delete(object);
            }
            session.getTransaction().commit();
        } catch (Exception ex) {
            ex.printStackTrace();

            session.getTransaction().rollback();
        } finally {
            session.close();
        }

    }

    @Override
    @SuppressWarnings("unchecked")
    public List<E> getAll(Class<E> c) {
        Session session = HibernateUtil.openSession();
        List<E> t = null;
        try {
            session.beginTransaction();
            t = session.createCriteria(c).list();
            session.getTransaction().commit();
        } catch (Exception e) {
            e.printStackTrace();
            session.getTransaction().rollback();
        } finally {
            session.close();
        }
        return t;
    }

    @Override
    public void update(E t) {
        Session session = HibernateUtil.openSession();
        try {
            session.beginTransaction();
            session.merge(t);
            session.getTransaction().commit();
        } catch (Exception e) {
            e.printStackTrace();
            session.getTransaction().rollback();
        } finally {

            session.close();
        }

    }

    @SuppressWarnings("unchecked")
    @Override
    public E get(int id, Class<E> c) {

        Session session = HibernateUtil.openSession();
        E t = null;
        try {
            session.beginTransaction();
            t = (E) session.get(c, id);
            session.getTransaction().commit();
        } catch (Exception e) {
            e.printStackTrace();
            session.getTransaction().rollback();
        } finally {
            session.close();
        }
        return t;
    }

}

Now, I'm a bit suspicious about session management. Is this a proper way to manage them (open session, commit changes, close session) or should I use some other approach, and what are the other suggestions to improve my code?

Wrong Transaction Management

Transactions are not a Data Access Layer concern, they are Business Logic Layer concern.

Consider how you would delete an entity, (any operation updating the database is similar) :

public class WidgetManager {
    public void deleteWidget(int widgetId) throws BusinessException{
        Widget widget = dao.get(Widget.class, widgetId);

        if (widget == null) throw new BusinessException("WidgetNotFound");

        if (!widget.isDeletable()) throw new BusinessException("WidgetCannotBeDeleted");

        dao.delete(widget);
    }
}

this method should be run in a transaction.

I would suggest you use Spring for transaction management as @ohiocowboy. (But using Hibernate API or Hibernate through JPA API (for example EntityManager) is not relevant to your current question.)

Repetitive Code

The following snippet is repeated in each method:

    Session session = HibernateUtil.openSession();
    try {
        session.beginTransaction();

        // do something with `session`

        session.getTransaction().commit();
    } catch (Exception e) {
        e.printStackTrace();
        session.getTransaction().rollback();
    } finally {
        session.close();
    }

This situation arises quite frequently, mostly when you deal with resources such as Hibernate sessions, DB transactions, connections, resultsets, files, sockets, ...
You can use Execute Around Idiom to alleviate this problem.

Common code can be factored thus:

interface Function<T, R> {
    R apply(T t);
}

<R> R execute(final Function<Session, R> func) {
    R result = null;

    Session session = HibernateUtil.openSession();
    try {
        session.beginTransaction();

        result = func.apply(session);

        session.getTransaction().commit();
    } catch (Exception e) {
        e.printStackTrace();
        session.getTransaction().rollback();
    } finally {
        session.close();
    }

    return result;
}

Now get reads like this [Annotations omitted for clarity]:

public E get(final Class<E> c, final int id) {
    return execute(new Function<Session, E>() {
        public E apply(Session session) {
            return (E) session.get(c, id);
        }
    });
}

If you have Java 8, you already have Function interface. And above can be written even more concisely as follows:

public E get(final Class<E> c, final int id) {
    return execute(session -> (E) session.get(c, id));
}

For methods that return void, you can avoid unnecessary return null; statements with:

interface Consumer<T> {
    void accept(T t);
}

void execute(final Consumer<Session> cons) {
    execute(new Function<Session, Object>() {
        @Override
        public Object apply(Session t) {
            cons.accept(t);
            return null;
        }
    });
}

Now insert reads like this:

public void insert(final E t) {
    execute(new Consumer<Session>() {
        public void accept(Session session) {
            session.save(t);
        }
    });
}

If you have Java 8, you already have Consumer interface. And above can be written even more concisely as follows:

public void insert(final E t) {
    execute(session -> {session.save(t);});
}

Evil Singleton

HibernateUtil is either a singleton or, less likely and much worse, a real utility class that creates a SessionFactory that loads Hibernate mappings on each call to openSession. A utility class should not have any state, such as a SessionFactory.

EDIT: What to do about HibernateUtil singleton?

Let me tell you first how I would fix it: I would delete the static modifier from the openSession method and fix all the compile errors. But this style depends on how familiar you are with the compiler, your IDE and the code base you are currently working on, not to go out of hand.

However you do it in the end HibernateUtil and BaseDAOImpl should look similar to these:

public class HibernateUtil {
    private SessionFactory sessionFactory;

    public HibernateUtil (SessionFactory sessionFactory, ...) {
         this.sessionFactory = sessionFactory;
    }

    public openSession() { // <----- not static
        // The contents should not change much, if at all
    }
}

public class BaseDAOImpl .... {
    private HibernateUtil hibernateUtil;

    public BaseDAOImpl(HibernateUtil hibernateUtil) {
        this.hibernateUtil = hibernateUtil;
    }
// Later use `hibernateUtil` instance to call `openSession`
    Session session = hibernateUtil.openSession();

If you are using a DI framework, you make HibernateUtil another component and inject it to DAO instances.

If, instead, you are bootstrapping your application from main or some init method:

void initMyApp(.....) {

     // create SessionFactory once
     SessionFactory sessionFactory = .......

     // create HibernateUtil once
     HibernateUtil hibernateUtil = new HibernateUtil(sessionFactory) .......

     // Use the same `hibernateUtil` many times

     FooDAO fooDAO = new FooDAOImpl(hibernateUtil);
     BarDAO barDAO = new BarDAOImpl(hibernateUtil);
     BazDAO bazDAO = new BazDAOImpl(hibernateUtil);
}


Lightweight flexible observer framework using weak references I found a need for a simple, generic publish-subscribe framework for my Java application.  I looked over a few implementations on Code Review and I think mine avoids most of the pitfalls.  Single-class implementation using a Consumer to receive messages and an Object for indexing.

import java.util.Collections;
import java.util.Map;
import java.util.WeakHashMap;
import java.util.function.Consumer;

public class Broadcaster<M> {
    private final Map<Object,Consumer<M>> listeners = Collections.synchronizedMap(new WeakHashMap<>());

    public void post(M message){
        listeners.values().forEach(c->c.accept(message));
    }

    public void listen(Object listener, Consumer<M> consumer){
        listeners.put(listener, consumer);
    }

    public void ignore(Object listener){
        listeners.remove(listener);
    }
}

Here's a demo of (most of) the functionality.  Hopefully I wrote enough to give a general idea of how it works.

import java.util.function.Consumer;

public class NeedsToBroadcast{
    private Broadcaster<Thing> thingBroadcaster = new Broadcaster<>();

    public void listen(Object listener, Consumer<Thing> consumer){
        thingBroadcaster.listen(listener, consumer);
    }

    public void ignore(Object listener){
        thingBroadcaster.ignore(listener);
    }

    public void doStuff(int i){
        do{
            Thing thing = StuffProcessor.process(i);
            if(thing != null){
                // Multiple objects care about the Things we get
                i = thing.getNumber();
                thingBroadcaster.post(thing);
            }else{
                i = 0;
            }
        }while(i>1);
    }

    public static void main(String... args){
        NeedsToBroadcast ntb = new NeedsToBroadcast();
        StandingLog standingLog = new StandingLog(ntb);

        ntb.doStuff(20);

        System.out.println(standingLog.toString());
    }

    public static class Thing{
        private final int number;
        private final String text;

        private Thing(int number, String text){
            this.number = number;
            this.text = text;
        }

        public int getNumber(){
            return number;
        }

        public String getText(){
            return text;
        }
    }

    public static class StandingLog{
        private final StringBuffer buffer = new StringBuffer();

        public StandingLog(NeedsToBroadcast ntb){
            // As long as this object has a reference, it will receive updates
            ntb.listen(this, (thing)->{
                buffer.append(thing.getNumber()).append(":").append(thing.getText()).append('\n');
            });
        }

        @Override
        public String toString(){
            return buffer.toString();
        }
    }
}

Update

I'm not trying to make something competitive with, say, 0MQ.  I'm trying to create a drop-in, single-class replacement for something like Java's Observable and Observer, with the added benefits of thread safety, cleanup for GC-elegible objects, and generic typing.

The original title of this post used "publish-subscribe".  I changed it to "observer" since, while I feel they are similar enough for this class, the distinction seems to have given people the wrong idea.

It seems the demo I posted doesn't do a fantastic job of demonstrating the functionality.  I'll see if I can rework something for clarity's sake.

I have some "readability-issues". It usually helps to name your methods with a verb and a noun, so, post would be postMessage, for instance.

listen and ignore are not named correctly, in my opinion. If I call listen(listener, consumer), I do not know what this method will do with the two parameters, it feels like, the method adds the listener to the consumer. I also have no idea, that the listener is used for unsubscribing later. If I call ignore(listener), it feels like I pause the listener, and can activate it later. addListener and removeListener would have been more clear.

Also, since we're talking about publishers and subscribers, I'd recommend to use the terminology of this pattern. Publisher, Subscriber (well, Consumer is given by the jdk), publish, subscribe and unsubscribe.

Well, it's a bit smart-arse-y, but, since I'm here,... By definition, or at least by the definition I know/remember, one thing the publish/subscribe pattern wants to achieve is, that the subscribers have the ability to subscribe to specific messages types, e.g. : You subscribe to a news feed, but only the sports part. So, I recommend to rework the terminology, because a user of your API can have different expectations of the API. (Yes, I'm one of them :P)

StuffProcessor is missing, btw, so I can't run the code.

I had a really hard time to understand your sample code. I tried to make an easier example of the usage:

    public class BroadcasterSampleUsage {
                public static void main(String[] args) {
                           Broadcaster<Message> broadcaster = new Broadcaster<>();
                           Object someIdentifier = new Object();

                           broadcaster.listen(someIdentifier, new MessageConsumer());

                           broadcaster.post(new Message("helloes"));
                }

                private static class MessageConsumer implements Consumer<Message> {
                           @Override
                           public void accept(Message t) {
                                       System.out.println("Received message: " + t.getMessage());
                           }
                }

                private static class Message {
                           private final String message;

                           public Message(String message) {
                                       this.message = message;
                           }

                           public String getMessage() {
                                       return message;
                           }
                }
    }

I'm not 100% certain if that's the intention of your API, after I've seen the usage code, but alas, if I understood your intention correctly, that would be easier to read.

The only Reason for the Object listener is to have it as key object, right? Now, why is that confusing for me: You call that thing listener. What is it listening for? And what's the difference between listening and consuming?

In your sample code, you actually do use this as key and use a lambda expression as parameter. So what if I would actually would overwrite the accept method in the StandingLog type, change the method signature in Broadcaster from

public void listen(Object listener, Consumer<M> consumer)

to

public void listen(Consumer<M> consumer)

, and instead of putting the listener and the consumer to the map, I use the consumer as key and value, wouldn't I achieve the same thing? If yes, I think you should be able to use this one: http://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#newSetFromMap(java.util.Map).

Also, check the JavaDoc about the synchronized* methods. Synchronization may not be done yet: It is imperative that the user manually synchronize on the returned map when iterating over any of its collection views. I do think, any means keys and values.

Hope this helps,

slowy


Evaluating an expression with integers, +, and *, as well as -, / - revision 3 The original question came from this web site.

Revision 1

Revision 2

And this is the third revision. I delete spaces and validate an equation before computing.

public class Main {

    private static final String EQUATION_REGEX = "^-?\\d+((\\/|\\*|\\+|-)\\d+)*$";

    // http://www.careercup.com/question?id=4911380140392448
    public static void main(String[] args) {
        String equationWithAllSigns = "-1 * 5 * 4/2 - 8*9 - 16+1";

        // Delete spaces
        equationWithAllSigns = equationWithAllSigns.replaceAll("\\s+", "");

        // Compute the equation if it's valid
        if (equationWithAllSigns.matches(EQUATION_REGEX)) {
            double allSignsResult = computeWithAllOperations(equationWithAllSigns);
            System.out.println(allSignsResult);
        } else {
            System.out.println("Invalid equation");
        }
    }

    private static double computeWithAllOperations(String input) {
        double result = 0.0;
        /*
         * 1. Replace - with +- except the minus which is the first symbol of
         * the input.
         * 2. Split the input string by +
         * 3. Calculate the products and quotients
         * 4. Sum up the results
         */

        // (\\d)- is used instead of - to support strings starting with -
        String parts[] = input.replaceAll("(\\d)-", "$1+-").split("\\+");
        for (String part : parts) {
            result += computeMultAndDivision(part);
        }
        return result;
    }

    private static double computeMultAndDivision(String part) {
        String[] parts = part.split("(?=[/*])|(?<=[/*])");
        double result = Double.parseDouble(parts[0]);
        for (int i = 1; i < parts.length; i += 2) {
            String op = parts[i];
            double val = Double.parseDouble(parts[i + 1]);
            switch (op) {
            case "*":
                result *= val;
                break;
            case "/":
                result /= val;
                break;
            }
        }
        return result;
    }
}

From your second revision:

  I don't consider 2 * -3 a valid equation. It should look like this:

    -2*3

What if you need to multiply two negative integers? :p Hold that thought for awhile...

Validation logic

Even if we suppose you have made a case for your existing validation logic, it should belong to computeWithAllOperations(), possibly throwing an IllegalArgumentException exception containing a message to let the caller know what went wrong. You may want to go one step further to give a brief documentation about this on the method's Javadoc.

Handling -

Your code transforms the - sign based on a preceding digit, which is a slightly odd twist in logic. Instead, I think you should be checking whether a - sign is preceded by any other mathematical operators ignoring whitespaces, which will yield the following change:

final String parts[] = input.replaceAll("\\s+", "")
                            .replaceAll("--", "+")
                            .replaceAll("((?<![/*+])-)", "+$1");

Just like your original code, we strip out whitespaces, then transform a 'minus negative' operation into an addition, and finally replace only the - signs that do not follow other mathematical operators: /, * and +. These are the only cases when we need to treat it as a subtraction rather than to signify a negative value. More can be added here to support % and/or ^.

However, this change does introduce a slight quirk, which changes expressions starting with a negative integer to "+-1*5...". To mitigate that, we can do a simple short-circuiting in computeMultAndDivision():

private static final Pattern REGEX = Pattern.compile("(?=[/*%^])|(?<=[/*%^])");

public static double computeMultAndDivision(final String part) {
    final String[] parts = REGEX.split(part);
    if (parts[0].isEmpty()) {
        return 0;
    }
    ...
}

Unit testing

Last but not least, I'll suggest doing proper unit testing for your utility method to assert its correctness. :)


A partially overlapping sequence of jQuery animations I know this is a mess!  How can I write this code better?

function anim() {
            $( "#p1-animation-1" ).fadeIn( 1200, function() {
                $( "#p1-animation-1" ).fadeOut( 1200 );
                $( "#p1-animation-2" ).fadeIn( 1200, function() {
                    $( "#p1-animation-2" ).fadeOut( 1200 );
                    $( "#p1-animation-3" ).fadeIn( 1200, function() {
                        $( "#p1-animation-3" ).fadeOut( 1200 );
                        $( "#p1-animation-4" ).fadeIn( 1200, function() {
                            $( "#p1-animation-4" ).fadeOut( 1200 );
                            $( "#p1-animation-5" ).fadeIn( 1200, function() {
                                $( "#p1-animation-5" ).fadeOut( 1200 );
                                $( "#p1-animation-6" ).fadeIn( 1200, function() {
                                    $( "#p1-animation-6" ).fadeOut( 1200 );
                                    anim();
                                });
                            });
                        });
                    });
                });
            });
        }
        anim();

@Paritosh has suggested some code to change what you have done, but doesn't explain it at all.

The suggestion is that you use a loop

(function loop() {
    $('.elements').each(function() {
        var $self = $(this);
        $self.parent().queue(function (n) {
        $self.fadeIn(2000).delay(200).fadeOut(2000, n);
        });
    }).parent().promise().done(loop);
}());

This is the code taken from Paritosh's JSFiddle

All that is being done here is that you are sending a node as a parameter to the function and then it is going through each child node of that node, fading it in and then fading it out.

This is much more efficient and can be used on any node, in other words you can reuse this code whereas the original code couldn't be reused unless you changed all the ID's.


Accessing and caching attributes I have the following manager class that handles access and caching attributes from classes. I am a bit worried about the caching mechanism.

Is there a better way of handling it?

public static class AttributeManager
    {
        private static Dictionary<Type, Attribute> _cache = new Dictionary<Type, Attribute>();

        public static AttributeType GetAttribute<AttributeType>(Type type)
            where AttributeType : Attribute
        {

            if (_cache.ContainsKey(type))
            {
                return (AttributeType)_cache[type];
            }

            var attr = Attribute.GetCustomAttribute(type, typeof(AttributeType));

            if (attr != null)
            {
                _cache.Add(type, attr);
                return (AttributeType)attr;
            }
            else
            {
                throw new ArgumentException($"The specified attribute: {nameof(AttributeType)} was not found.");
            }
        }
    }

Let’s define an extension method on Type:

public static class AttributeCache
{
    public static IEnumerable<T> Attributes<T>(this Type type)
        where T : Attribute => Cache
            .GetOrAdd(type, t => t.GetCustomAttributes(true).OfType<Attribute>().ToArray())
            .OfType<T>();

    static ConcurrentDictionary<Type, Attribute[]> Cache { get; } =
        new ConcurrentDictionary<Type, Attribute[]>();
}

Demo:

foreach (var a in typeof(MyCass).Attributes<TestAttribute>())
    Console.WriteLine(a.MyProp);


Unusual comparison of object and string I have a pretty unusual situation (please, don't ask why) where I need to compare objects which might be a string or double to string which might be a string or a double.ToString(). I've come up with something which I really don't like, so please feel free to criticize it and suggest a better solution.

public bool AreEqual(string stringValue, object objectValue)
{
    double numericValue;
    return (stringValue == (objectValue ?? string.Empty).ToString()) || // trying to compare values as strings
           (double.TryParse(stringValue, out numericValue) && (numericValue == objectValue as double?)); // otherwise - as double
}

To make it clear what I expect to get from that function:

[TestCase("a", "a", true)]
[TestCase("a", "b", false)]
[TestCase("10", 10, true)]
[TestCase("11", 10, false)]
[TestCase("11", "11.0", true)]
[TestCase("11", "11.5", false)]
public void ComparisionTest(string stringValue, object objectValue, bool areEqual)
{
     bool result = AreEqual(stringValue, objectValue);
     Assert.That(result == areEqual)
}

Your clarification made it a lot clearer what you want. I think you're trying too much to make it all fit on two lines and you can make it must more readable by splitting it out a little.

Following straightforward logic (really just translating your description) I came to this which works for all 4 tests (note that argument 10 is an integer, not a double).

public bool AreEqual(string stringValue, object objectValue)
{
  if (objectValue is string)
  {
      double parsedValue;
      if (double.TryParse(objectValue.ToString(), NumberStyles.Any, CultureInfo.InvariantCulture, out parsedValue))
      {
          return Math.Abs(double.Parse(stringValue, CultureInfo.InvariantCulture) - parsedValue) < 0.00000001;
      }

      return stringValue == objectValue as string;
  }

  if (objectValue is double)
  {
      return double.Parse(stringValue, CultureInfo.InvariantCulture) == objectValue as double?;
  }

  throw new ArgumentException("The object must either be a string or a double");
}

Note that you can also use a direct cast instead of as since it is guaranteed to be a possible cast, but I like to read as myself (the performance impact is minimal and not relevant in our scenario).

Note that I also use double.Parse instead of double.TryParse because from your description I can assume it is either a double or a string. If it isn't, I'm throwing an exception anyway at the end so I don't feel compelled to use TryParse.

I have made changes that take care of the added test cases which you specified later. Alongside that it also takes care of the culture-specific notation (dot vs comma) and floating point errors.

It's more lengthy than your solution but it is also more robust code and a lot easier to digest.

This takes care of the following testcases:

AreEqual("a", "a") // true
AreEqual("a", "b") // false
AreEqual("10", 10.0) // true
AreEqual("11", 10.0) // false
AreEqual("11", "11.0") // true
AreEqual("11", "11.5") // false
AreEqual("11.0", "11") // true


Copying text to the user's clipboard Here's my code.

javascript:x=document.createElement("script");x.src="https://cdn.jsdelivr.net/npm/clipboard@2/dist/clipboard.min.js"; document.getElementsByTagName("head")[0].appendChild(x);copy(1)

Is this the correct way? Are there any flaws in it?

Running:

copy(1)

doesn't work by itself so that's not a possibility, and

javascript:x=document.createElement("script");x.src="https://cdn.jsdelivr.net/npm/clipboard@2/dist/clipboard.min.js"; document.getElementsByTagName("head")[0].appendChild(x);copy(1)

appears to be the best so far.

Looks fine, except for polluting the namespace, you could consider using a self executing anonymous function.

javascript:(function(){x=document.createElement("script");x.src="https://cdn.jsdelivr.net/npm/clipboard@2/dist/clipboard.min.js"; document.getElementsByTagName("head")[0].appendChild(x);copy(1);})();


What is wrong with my knapsack alghoritm? My alghoritm works and produces good result, but it takes too much time(6 s for 300 input items that is too much for branch and bound alghoritm), so I have maken a mistake somewhere, but really can't find it :( It is supposed to be depth-first branch and bound. Any help would be appreciated!

public void knapsack2(Item[] items, int maxWeight)
{
    Item firstNode, secondNode, tempNode;

    int weight, bestValue = 0, bestWeight = 0;

    Stack stack = new Stack();

    firstNode = items[0];

    firstNode.setBound(bound(firstNode, maxWeight, items));

    stack.push(firstNode);

    Item bestNode = firstNode;

    while(!stack.isEmpty())
    {
        tempNode = (Item) stack.pop();
        weight = tempNode.getWeight();

        if( tempNode.getBound() > bestValue)
        {
            firstNode = new Item();
            firstNode.setLevel(tempNode.getLevel() + 1);
            firstNode.setValue(tempNode, items[firstNode.getLevel()].getValue());
            firstNode.setWeight(weight + items[firstNode.getLevel()].getWeight());
            firstNode.setBound(bound(firstNode, maxWeight, items));

            if(firstNode.getValue() > bestValue && firstNode.getWeight() <= maxWeight)
            {
                bestValue = firstNode.getValue();
                bestWeight = firstNode.getWeight();

                bestNode = firstNode;

            }

            if(firstNode.getBound() > bestValue)
            {
                stack.push(firstNode);
            }

            secondNode = new Item();
            secondNode.setLevel(tempNode.getLevel() + 1);
            secondNode.setValue(tempNode, 0);
            secondNode.setWeight(weight);
            secondNode.setBound(bound(secondNode, maxWeight, items));

            if(secondNode.getBound() > bestValue)
            {
                stack.push(secondNode);
            }
        }
    }

    System.out.println("Best value " + bestValue);
}

Bound function:

    public float bound(Item item, int maxWeight, Item[] items)
    {
    int j, k;
    int totalWeight;
    float result;

    if(item.getWeight() > maxWeight)
    {
        return 0;
    }
    else
    {
        result = item.getValue();
        j = item.getLevel() + 1;
        totalWeight = item.getWeight();

        while(j < items.length && (totalWeight + items[j].getWeight() <= maxWeight))
        {
            totalWeight += items[j].getWeight();
            result += items[j].getValue();
            j++;
        }
    }

    k = j;

    if(k < items.length)
    {
        result = result + (maxWeight - totalWeight) * (items[k].getValue() / (float) items[k].getWeight());
        return result;
    }

    return 0;
}

Branch&Bounding is not what you have implemented (at least I can't see the classic algorithm in there).

Branch And Bound is normally implemented as a recursive process. I don't see recursion here. Additionally, even if the recursion is unwound to your while loop, I can't see where the efficient Branching happens.... frankly, it's a bit confusing.

Additionally, as I read your code, I believe there is a bug in that your solution will always contain items[0] (the firstNode is always pushed to the stack, and the code completes when it is popped....).

The general solution for the knapsack algorithm is to try every combination of items and find ones that fit. This is a 'combination' problem. Using recursion to find combinations is quite easy, and it is well defined using the 'branch' part of Branch and Bound.

The Bound part of the problem is that you can eliminate whole branches based on the known state of the knapsack at that branch (i.e. adding anything more from that branch will cause the conditions to fail, so you can 'prune' the whole branch). The Bound part makes the Branch part more efficient.

I believe the Branch-side of your code is broken, so the Bounds-part comes later (when you have revised/reposted the branch side).

Working on the branch side of things, the easiest thing to do is to have recursion in a for-loop......

I recommend passing a 'Branch' Object back from the recursive method, and stipulating a target weight for the branch:

public Branch branch(Item[] items, int fromposition, int target) {
    if (fromposition >= items.length) {
        return null;
    }
    Branch best = null;
    for (int i = fromposition + 1; i < items.length; i++) {
        Branch current = new Branch(items[i]);
        // note that the recursion happens from fromposition+1
        current.addAll(branch(items, i, target - items[i].weight));
        if (current.weight <= target) {
            best = (best == null || current.weight > best.weight) ? current : best;
        }
    }
    return best;
}

With the above code we calculate the 'weight' of the Branch, and we compare that to the capacity remaining in the knapsack. If we compare every combination with our knapsack we would find the solution, but not in the most efficient way. This does the Branch but nothing about Bounds.

To compute the bounds there are a few solutions.... but, starting off with sorted items leads to the most efficient solutions.... so, sort your items in ascending order.

Then, your Bounds solution becomes the simple addition of a couple of constraints:

change:

for (int i = fromposition + 1; i < items.length; i++) {

to

for (int i = fromposition + 1; i < items.length && items[i].weight <= target; i++) {

Now we stop checking our branches the moment we find that the next option is too large, and we can skip processing anything after that item because our data is sorted, and we know that the rest of the stuff is even heavier.

Another bound we can add is to stop checking when we find a perfect solution:

        if (current.weight <= target) {
            best = (best == null || current.weight > best.weight) ? current : best;
        }

becomes:

        if (current.weight == target) {
            return current;
        } else if (current.weight < target) {
            best = (best == null || current.weight > best.weight) ? current : best;
        }

I have just typed this in, I have not verified any of the code, but, as you can see, the solution would be:

public void knapsack2(Item[] inputitems, int maxWeight) {
    // take a copy because we will sort the items...
    Item[] items = Arrays.copyOf(inputitems, inputitems.length);
    Arrays.sort(items, new Comparator<Item>() { ... sorts by weight...});
    Branch best = branchAndBound(items, 0, maxWeight);

}

public Branch branchAndBound(Item[] items, int fromposition, int target) {
    if (fromposition >= items.length) {
        return null;
    }
    Branch best = null;
    for (int i = fromposition + 1; i < items.length; i++) {
        Branch current = new Branch(items[i]);
        // note that the recursion happens from fromposition+1
        current.addAll(branch(items, i, target - items[i].weight));
        if (current.weight <= target) {
            best = (best == null || current.weight > best.weight) ? current : best;
        }
    }
    return best;
}

The Code for the Branch class will need to allow you to track the branch weight, and to add Items to the branch. Then, at the end, return the Items too.


Memory cache service I am repeating this code at lot of places:

public class ProductTypeService : IProductTypeService
{
    public async Task<IList<ProductType>> GetAllAsync(int sectionId = -1)
    {
        string key = "ProductTypeService_" + "GetAllAsync" + sectionId;
        var list = _memoryCacheService.Get<IList<ProductType>>(key);
        if (list == null)
        {
            list = await _repository.GetAllAsync(sectionId);
            _memoryCacheService.Save(key, list);
        }
        return list;
    }

public class VendorService : IVendorService
{
    public async Task<IList<Vendor>> GetAllAsync()
    {
        string key = "VendorService_" + "GetAllAsync";
        var list = _memoryCacheService.Get<IList<Vendor>>(key);
        if (list == null)
        {
            list = await _repository.GetAllAsync();
            _memoryCacheService.Save(key, list);
        }
        return list;
    }

The key is the combination of class + method + parameter. The only change is _repository.XX(XX, XX, ...........). How can I make it DRY?

First we don't review:

ProductType
Vendor
IProductTypeService (just a little)
IVendorService
IRepository (just a little)
what is _memorxCahceService? I will use ObjectCache.

What we need to discuss?

A generic service implementation which will contain the common methods and implementation.

BaseService

We will need a class which can be the bridge to the repository and the cache so define it aas BaseService. T means we will use it in different repository+cache combose int a generic way. No black magic here, move on.

First we need to extract the common things from the two service implementation in you question:

get a cache key
try load from cache the data

public abstract class BaseService<T>
{
    private ObjectCache ObjectCache { get; set; }
    private IRepository<T> Repository { get; set; }

    protected BaseService(IRepository<T> repository)
        : this(MemoryCache.Default, repository)
    {
    }

    protected BaseService(ObjectCache objectCache, IRepository<T> repository)
    {
        ObjectCache = objectCache;
        Repository = repository;
    }

    protected virtual string GetCacheKey(string suffix, int sectionId = -1)
    {
        return GetType().Name + "_" + suffix + sectionId;
    }

    protected virtual async Task<ICollection<T>> GetAllAsyncValueFactory()
    {
        return await Repository.GetAllAsync();
    }

    public virtual async Task<ICollection<T>> GetAllAsync(int sectionId = -1)
    {
        var key = GetCacheKey("GetAllAsync", sectionId);

        var list = ObjectCache.Get(key) as ICollection<T>;

        if (list == null)
        {
            return ObjectCache.AddOrGetExisting(key, await GetAllAsyncValueFactory(), new CacheItemPolicy
            {
                //TODO: define cache policy
            }) as ICollection<T>;
        }

        return list;
    }
    }

What do we here is that we defining in the constructor what we will need: a repository and an ObjectCache but we could you a default MemoryCache instance if we don't get anything other.

The main thing is i have extracted all the logic you are using commonly in your code but with a generic implementation (hint: don't use IList is a dirty interface). Every method is virtual so they can be easily override in derived classes you you can define different cache keys or you can tell how you would want to get the data from the current repository.

Interface implementation

Because the generic base class we only need to define the constructors in our interface implementation classes as you can se:

public class ProductTypeService : BaseService<ProductType>, IProductTypeService
{
    public ProductTypeService(ObjectCache objectCache, IRepository<ProductType> repository)
        : base(objectCache, repository)
    {
    }

    public ProductTypeService(IRepository<ProductType> repository)
        : base(repository)
    {
    }
}

The constructor implementation will be different based on your unknown needs.


Solving the Burst Balloon problem using Dynamic Programming Continuing where I left off previously to solve the problem described here, I've now solved the same using dynamic programming (following Tikhon Jelvis blog on DP).

To refresh, the challenge is to find a sequence in which to burst a row of balloons that will earn the maximum number of coins. Each time balloon \$i\$ is burst, we earn \$C_{i-1} \cdot C_i \cdot C_{i+1}\$ coins, then balloons \$i-1\$ and \$i+1\$ become adjacent to each other.

import qualified Data.Array as Array

burstDP :: [Int] -> Int
burstDP l = go 1 len
  where
    go left right | left <= right = maximum [ds Array.! (left, k-1)
                                            + ds Array.! (k+1, right)
                                            + b (left-1)*b k*b (right+1) | k <- [left..right]]
                  | otherwise    = 0
    len = length l
    ds = Array.listArray bounds
           [go m n | (m, n) <- Array.range bounds]
    bounds = ((0,0), (len+1, len+1))
    l' = Array.listArray (0, len-1) l
    b i = if i == 0 || i == len+1 then 1 else l' Array.! (i-1)

I'm looking for:

Correctness
Program structure
Idiomatic Haskell
Any other higher order functions that can be used
Other optimizations that can be done

Your use of Array for memoization can be extracted into array-memoize.

If one can stop instead of having negative balloons decrease score, go can be condensed into one case.

import Data.Function.ArrayMemoize (arrayMemoFix)
import Data.Array ((!), listArray)

burstDP :: [Int] -> Int
burstDP l = arrayMemoFix ((0,0), (len+1, len+1)) go (1, len) where
  go ds (left, right) = maximum $ 0 :
    [ds (left, k-1) + ds (k+1, right) + b (left-1)*b k*b (right+1) | k <- [left..right]]
  b = (!) $ listArray (0, len+1) (1 : l ++ [1])
  len = length l

If you don't care too much about performance, we can also memoize directly on the balloon list:

burstDP :: [Int] -> Int
burstDP = memoFix3 go 1 1 where go ds l r b = maximum
  [ ds left l x + ds right x r + l*x*r
  | (left, x:right) <- zip (inits b) (tails b)
  ]


How can I improve performance of my selecting code I have a page with a list of items. Items have several actions assigned to them. (see screenshot).

One may choose to directly click on an icon next to each row or check a checkbox on the left hand side and click action above the list.

The issue is that after clicking an item OR checking a checkbox of several items and then clicking an action there is a lag (a second or so). Imagine having 100 rows or more.

Script:

 $("a.action:not(.remove)").click(function (e) { // .remove => do not execute on download tasks page
            var selected = new Array(); // array to hold selections or clicks
            var obj = $(this), objData = obj.data();
            e.preventDefault();
            if (!obj.hasClass('disablelink')) {
                if (objData.ajaxMachineid) {
                    selected.push(objData.ajaxMachineid);
                } else {
                    $("input.checkbox:checkbox:checked:not(.checkall)").each(function () {
                        var checkbox = $(this), machineId = checkbox.val(), packageId = objData.ajaxPackageid.removeSpecialChars().toUpperCase(), operation = objData.ajaxType;
                        var $row = $("#" + machineId + packageId.removeSpecialChars().toUpperCase());
                        if ($row.length) {
                            $row.has("a[data-ajax-type=" + operation + "]:not(.hide)").length ? selected.push(machineId) : $(this).attr('checked', false);
                        }
                    });
                }
                if (selected.length > 0) {
                    // do something more here blah blah blah
                };
            }
            $("input.checkall").attr("checked", false);
        });

Sample HTML of one row:

<input type="checkbox" value="1960" class="checkbox">

GD009000246

PCGames
Up

10.48.1.236

   View
Install
   Remove

So pretty much everything will make a insignificant effect apart from the section where you are looping through each of the checkboxes. See my modifications and notes below.

$("a.action:not(.remove)").click(function (e) { // .remove => do not execute on download tasks page
     var selected = new Array(); // array to hold selections or clicks
     var obj = $(this),
         objData = obj.data(),

         // Moved these up here as they are not specific to the checkboxes
         packageId = objData.ajaxPackageid.removeSpecialChars().toUpperCase(),
         operation = objData.ajaxType;

     e.preventDefault();
     if (!obj.hasClass('disablelink')) {
         if (objData.ajaxMachineid) {
             selected.push(objData.ajaxMachineid);
         } else {

             // This seems a little convoluted to me, normally you would
             // use [type=checkbox]
             $("input[type=checkbox]:checked:not(.checkall)").each(function () {

                 var checkbox = $(this),
                     machineId = checkbox.val();

                 // No need to call .removeSpecialChars().toUpperCase() on packageId
                 var $row = $("#" + machineId + packageId);

                 if ($row.length) {

                     // Not 100% whether you need .length here
                     // It could also be beneficial to add a class to the row instead
                     // of calling this complex selector
                     $row.has("a[data-ajax-type=" + operation + "]:not(.hide)").length
                         ? selected.push(machineId)
                         : $(this).attr('checked', false);

                 }
             });
         }
         if (selected.length > 0) {
             // do something more here blah blah blah
         };
     }
     $("input.checkall").attr("checked", false);
 });

The obvious thing to do on top of my suggestions above is to cache the checkbox list, this means that every time it enters that section, the app will no longer need to go through every element in the DOM looking for all the checkboxes. Doing this will probably have the most meaningful impact on performance.

// in document ready
var checkboxes = $('input[type=checkbox]');

// When you need to find the checked check boxes
var checked = checkboxes.find(':checked:not(.checkall)');


A better implementation of the `execvp` call: the code implements `execvp` I'm trying to implement execvp () using execv (). I don't know if the code is the best implementation.
my code:
#include <string.h>
#include <unistd.h>
#include <stdlib.h>
#include <errno.h>
#include <stdio.h>

extern char **environ;

int my_execvp(char *file, char *argv[])
{
  char *p, *colon, *pathseq = getenv("PATH");
  size_t len;

  if(strchr (file, '/'))
    return execv(file, argv);

  if (pathseq == NULL) {
        len = confstr(_CS_PATH, (char *) NULL, 0);
        pathseq = (char *) alloca(1 + len);
        pathseq[0] = ':';
        (void) confstr (_CS_PATH, pathseq + 1, len);
  }
  len = strlen(file) + strlen(pathseq) + 2;

  for(p = pathseq; p && *p; p = colon)
  {
     char b[len];
     colon = strchr(p, ':');
     if(colon)
     {
        memcpy(b, p, (size_t) (colon - p));
        b[colon++-p] = 0;
     }
     else
        strcpy(b, p);
     strcat (b, "/");
     strcat (b, file);
     if(!access(b, F_OK)) {
            execv(b, argv);
            fprintf(stderr, "an error occurred in execv\n");
            abort();
     }
   }
   errno = ENOENT;
     return -1;
}

int main()
{
    /* "ls / -l" */
    char *arg[] = {"ls", "/", "-l", NULL};
    my_execvp (arg[0], arg);

    return 0;
}

And the question I have, is it a good implementation? How could it be improved or write a better implementation?

The implementation is non-conformant.

No handling of undefined PATH. Quoting man execp:

search path is the path specified in the environment by
``PATH'' variable.  If this variable is not specified, the default path
is set according to the _PATH_DEFPATH definition in <paths.h>

Once confstr returns 0, check errno, and act accordingly.

Calling abort on execv failure is plain wrong. At least return what execv returns.

Calling access is also incorrect. If the file exists, but doesn't have correct permissions, execvp shall continue searching.

As a side note, access is very rarely useful. Consider calling execvp blindly, and testing errno upon return.

Final errno = ENOENT; is also non-compliant. execvp sets ENOENT if the file does not exist in any path. If it was found at least once, and fails to execute, the errno shall be EACCES.

execvp is not supposed to fprintf anything.


Calculate change denominations 3 I am trying to refactor a large application to use more DDD principles as and when I get chance.  I am concentrating on the very simple areas first.  The app currently has a very simple Domain Service (one of many) as it stands (see option 1).  Please see the code below:

Option 1 - Live

public IEnumerable<System.Collections.Generic.KeyValuePair<decimal, int>> CalculateDenominations(decimal cost, ICurrency currency)
        {
            var target = cost;
            foreach (var denomination in currency.AvailableDenominations)
            {
                var numberRequired = target / denomination;
                if (numberRequired >= 1)
                {
                    int quantity = (int)Math.Floor(numberRequired);
                    yield return new KeyValuePair<decimal, int>(denomination, quantity);
                    target = target - (quantity * denomination);
                }
            }
        }

Option 2

public sealed class DenominationsRequired
    {
        private readonly decimal _cost;
        private readonly ICurrency _currency;

        public DenominationsRequired(decimal cost, ICurrency currency)
        {
            if (currency == null)
                throw new ArgumentNullException("Currency cannot be null", "ICurrency");
            if (cost < 0)
                throw new ArgumentException("Cost cannot be less than zero", "Cost");
            if (decimal.Round(cost, 2) != cost)
                throw new ArgumentException(string.Concat("Cost has too many decimal places.  It should only have: ", currency.DecimalPlaces), "Cost");
            _cost = cost;
            _currency = currency;
        }

        public decimal Cost
        {
            get { return _cost; }
        }

        public ICurrency Currency
        {
            get { return _currency; }
        }

        public IEnumerable<System.Collections.Generic.KeyValuePair<decimal, int>> CalculateDenominations()
        {
            var target = _cost;
            foreach (var denomination in _currency.AvailableDenominations)
            {
                var numberRequired = target / denomination;
                if (numberRequired >= 1)
                {
                    int quantity = (int)Math.Floor(numberRequired);
                    yield return new KeyValuePair<decimal, int>(denomination, quantity);
                    target = target - (quantity * denomination);
                }
            }
        }
    }

I am thinking about refactoring the Domain Service (option 1 - live) into a Value Object (option 2) so that I can add validation.  I would be grateful for critical comments about this approach.  Is a ValueObject (option 2) "better" than a domain service (option 1) for this?  I believe it is acceptable to put caclculations in Value Objects as long as they do not mutate state.

I am specifically interested to know:

Is a Value Object "better" than a Domain Service for this specific requirement?
Is it wise to have a property called: Currency.DecimalPlaces (indicates how many decimal places the currency allows e.g. 2 for pounds and 2 for dollars - some currencies allow 3 or 4 decimal places), which is used in the DenominationsRequired constructor for validation?
Should I keep option 1 or go with option 2? If option 1, then how do I approach validation?

1) is a Value Object "better" than a Domain Service?

This is certainly subjective, but i'll offer this pragmatic view point. Evaluate how often this code will be actively referenced and debugged by other developers. If this code is located in an easily reusable utility assembly, and it is a realistic expectation that it will be reused, then Value Object wins for me every time. It offers more self documentation than your initial attempt which is important because you have not implemented code comments at all.
At the end of the day you have to justify the time and effort required to get the code into production, code that is reused a lot, especially by other developers is worth the effort to get it right, but you have increased you code surface and will have to increase you unit test coverage to match.
Value objects like this i find are much easier to unit test, and like you have said with validation, the object now has additional options for expressing it's state without blocking execution with exceptions and we can more easily inherit this structure and when we need to override specific functionality, such as validation when we as developers do reuse your code.

  2) Is it wise to have a property called Currency.DecimalPlaces

Absolutely, especially with currency where rounding errors can have a real world monetary impact on the user, this seems like a useful thing to know, and it is natural to expect that Currency would already have this attribute.

  3) Should I go with option 2?

See my response to question 1  from a purist point of view your Value Object is a complete package, it has a dependency on Currency, but is easier to lock down with unit tests and to distribute in a utility assembly for reuse. It's up to you to evaluate the cost of developing this solution and refactoring existing code to use it.

Certainly follow the advice from @t3chb0t, you've gone to this much effort, implement your own contract definition for the response from CalculateDenominations, a key value pair response is too ambiguous for users know how to use the response, it's the missing bit of "self" documentation.

  How do i approach validation with option 1?

Throwing exceptions from the calculation routine is an acceptable method for validation, if this was a Mutable type (not readonly) then I would argue that it would be better than throwing exceptions from the constructor which can lead to other interesting and less than satisfactory implementation patterns.

  UPDATE: Moving validation to an explicit method called Validate() further helps to self document your class, and makes it easier to maintain long term. You still call validation from either your calculation method or the constructor. As a bonus when validation fails, the stack trace will now show DenominationsRequired.Validate() which further describes why the exception was raised for future debuggers.

For mutable types it's only at the point of execution that the validation matters, the code that interacts with the instances of this class should be free to instantiate the object and react to its state at a later point in the execution. It is hard for example to check the state of a Value object if we failed to create the object in the first place.

  Here your Value type is an Immutable Type, that is to say that it's value over time will not change, and cannot because you have marked the fields as readonly. This is a special case where it is more appropriate to validate in the constructor rather than in the calculation because there will not be any opportunity to change the value if it is not valid.

A quick note about your validation, you hardcoded 2 as the comparison, rather than Currency.DecimalPlaces, so it should be:

if (decimal.Round(cost, currency.DecimalPlaces) != cost)
        throw new ArgumentException(string.Concat("Cost has too many decimal places.  It should only have: ", currency.DecimalPlaces), "Cost");


Codility cyclic rotation solution in PHP Problem statement

A zero-indexed array A consisting of N integers is given. Rotation of the array means that each element is shifted right by one index, and the last element of the array is also moved to the first place.

For example, the rotation of array A = [3, 8, 9, 7, 6] is [6, 3, 8, 9, 7]. The goal is to rotate array A K times; that is, each element of A will be shifted to the right by K indexes.

My Solution

function solution($A, $K) {
   // when array is empty or has only one element
   if(count($A) == 0 || count($A) == 1){
        return $A;
    }
    //runs k times
    for($j=1; $j<=$K; $j++){
        $last_element = $A[count($A)-1];
        //runs for each element
        for($i=(count($A)-1); $i>0; $i--){
            $A[$i] = $A[$i-1];
        }
        $A[0] = $last_element;
    }
    return $A;
}

$A = [1, 2, 3, 4];
$K = 4;
$result = solution($A, $K);
print_r($result);

Output

Array
(
    [0] => 1
    [1] => 2
    [2] => 3
    [3] => 4
)

You could use array functions. They simplify and speed up the handling of arrays. See: http://php.net/manual/en/ref.array.php

So your code could become:

function rotateArray($inputArray,$rightShiftCount)
// shift all elements of the array to the right a number of times
{
  // extract the part of the array to move to the front
  $partToMove = array_splice($inputArray,-($rightShiftCount % count($inputArray)));
  // return extract part followed by what was left of the array
  return array_merge($partToMove,$inputArray);
}

This function does not check its arguments, if needed you can add that.

Notice that I have used sensible names for my variables instead of $A and $K. This is intentional.


A prime numbers enumeration optimized for speed I know there is this thing called "Eratosthenes" but that requires the allocation of a large array while I want to find (small) prime numbers fast, yet without needing too much memory. So I wrote PrimeTable.cs with this content:

using System;
using System.Collections.Generic;
using System.Linq;

namespace Primes
{
    public static class PrimeTable
    {
        private static readonly List<long> PrimeNumbers = new List<long>();

        public static long MaxValue { get; private set; } = 1;

        public static bool IsPrime(this long value)
        {
            if (value > MaxValue) { var longCount = Primes(true).TakeWhile(p => p <= value).LongCount(); }
            return PrimeNumbers.Contains(value);
        }

        public static long IndexOfPrime(this long value) => IsPrime(value) ? Primes().TakeWhile(p => p < value).LongCount() : -1;
        public static long NextPrime(this long value) => Primes().First(p => p > value);
        public static long PreviousPrime(this long value) => Primes().TakeWhile(p => p < value).LastOrDefault();

        public static IEnumerable<long> Primes(bool skipLast = false)
        {
            if (!skipLast) foreach (var l in PrimeNumbers) { yield return l; }
            while (MaxValue < long.MaxValue)
            {
                var max = (int)Math.Sqrt(++MaxValue);
                if (PrimeNumbers.Where(p => p <= max).All(p => MaxValue % p != 0))
                {
                    PrimeNumbers.Add(MaxValue);
                    yield return MaxValue;
                }
            }
        }
    }
}

The reason for this is because I want to stop looking after a certain value has been found. This is mere practice of my skills in enumerations and extension methods and I'm trying to be a bit creative.

So when I ask 11L.IsPrime() it will be true while 99L.IsPrime() will be false. But it won't calculate prime numbers over 11 until I ask if 99L is a prime. Then it won't go past 99. This keeps the number of calculations to a minimum.

The Primes() method is an enumerator that will basically continue calculating nearly forever and would thus take a while if I wasn't using deferred execution. But because of deferred execution, I can just stop enumerating at any moment and later continue the enumeration as it already knows the values it has had.

The IsPrime() is what I want to use in general, to check if a number is a prime or not. To do so, it needs to make sure it has calculated all primes up to the given number and if not, just calculate the remaining primes. It skips the primes it already knows but I have to find a better way to aggregate the enumeration as without the LongCount() in the end, it won't enumerate. It's deferred execution, after all. So, is there a better way to aggregate here?
I can't just use return Primes().Contains(value); as it would run almost forever when checking 99L.

The IndexOfPrime() will tell me the index of a prime number or -1 if it's not a prime.

The NextPrime() method is interesting, though. It will tell me the first prime number after a given value.
The PreviousPrime() method is trickier as I can't just ask for the last item less than value. It would enumerate nearly forever again.

The MaxValue field is just for debugging purposes so you can asl how far it has goine while enumerating...

The next challenge: can this be improved by using PLinq? If so, how?

There are many, many problems with this implementation, but they pretty much all come down to two major problems: first, you do a linear-time operation when a constant-time or log-time operation would suffice, and second, your code is chock-full of expressions that are useful for both their values and their side effects, which makes for code that is confusing.

return PrimeNumbers.Contains(value);

PrimeNumbers is a sorted list, but you check to see if a value is in it by starting from the beginning and searching every element in it. Do a binary search.

public static long IndexOfPrime(this long value) =>
  IsPrime(value) ? Primes().TakeWhile(p => p < value).LongCount() : -1;

This is bizarre. You use IsPrime for its side effect, and then do a linear search of the primes in a list to get their index. You have a list.  Just search the list for the index!

This was a good attempt but it has turned into an object lesson in what not to do. The fundamental strategy here is very sound and you should keep it, but the details around that strategy are confusing and inefficient. This is not a good use of LINQ.

What I would do here is refactor the program so that it does a smaller number of things and does them better. For example, suppose instead of this business of constantly enumerating Primes, you instead made two methods:

EnsureUpTo(n) -- makes sure that the list is filled in up to n.
NearestIndexOf(n) -- uses an efficient search to return the index of n, or, if n is not prime, the index of the nearest prime to n.
Prime(i) returns the ith prime.

From this simple interface you can answer all your questions:

You can determine if n is a prime by running EnsureUpTo(n) and then i = NearestIndex(n) and then m = Prime(i).  If n == m then n is prime, otherwise it is composite.
You can get the next or previous prime similarly; run i = NearestIndex(n) and then Prime(i-1) and Prime(i+1) are the next and previous.

Your routine for computing primes you don't already know also could use some work:

  var max = (int)Math.Sqrt(++MaxValue);

A number of problems here. Computing square roots is expensive; it is always better to do p * p <= m than p <= Sqrt(m).

The increment is also suspicious. Fully half the time you will be incrementing it to an even number! After you are at 3, increment it by 2.  Or, even better, notice that once you are above 5, you can pick any six numbers in order and at most two of them will be prime. That is, of 5, 6, 7, 8, 9 and 10 we know that 6, 8 and 10 are divisible by 2. We know that 6 and 9 are divisible by 3, so we only need to check 5 and 7. But that also goes for 11, 12, 13, 14, 15, 16:  12, 14, 15 and 16 cannot be prime, so we only have to check 11 and 13.  And then 17, 18, 19, 20, 21, 22 we only check 17 and 19. And so on.

So what you can do is increment MaxValue by 6 every time after you get to 5, and then check MaxValue and MaxValue + 2 for primality, and you do much less work.

if (PrimeNumbers.Where(p => p <= max).All(p => MaxValue % p != 0))

Again, this is really bad because LINQ does not know that the list is sorted.  You check the entire list, which is O(n) in the size of the list, for elements smaller than max, but you could be bailing out once you get to the first that is larger than max.  Where is not the right sequence operator here.  You want Until(p => p > max).


C++ shared pointer wrapper for lazy initialization I have written a very simple wrapper around std::shared_ptr that supports lazy initialization. Do you see any problems?
#include <functional>
#include <tuple>
#include <utility>

template <class T, typename ... Args>
class LazySharedPtr {
public:
    LazySharedPtr(Args ... args) :
        ptr(nullptr) {
        this->init = [args = std::make_tuple(std::forward<Args>(args) ...)]() mutable {
            return std::apply([](auto&& ... args) {
                return std::make_shared<T>(std::forward<Args>(args) ...);
                }, std::move(args));
        };
    }

    virtual ~LazySharedPtr() = default;

    bool IsInited() const noexcept {
        return ptr != nullptr;
    }

    void Init() {
        this->InitAndGet();
    }

    std::shared_ptr<T> Get() {
        return (ptr) ? ptr : InitAndGet();
    }

    const std::shared_ptr<T> Get() const {
        return (ptr) ? ptr : InitAndGet();
    }

    T* operator ->() {
        return this->Get().get();
    }

    const T* operator ->() const {
        return this->Get().get();
    }

    explicit operator bool() const noexcept {
        return this->IsInited();
    }

protected:
    std::function<std::shared_ptr<T>()> init;
    mutable std::shared_ptr<T> ptr;

    std::shared_ptr<T> InitAndGet() const {
        ptr = this->init();
        return ptr;
    }
};

Note: Visual Studio report warning for this->Get().get():

Warning   C26815  The pointer is dangling because it points at a
temporary instance which was destroyed.

However, I dont see why, because shared_ptr is owned by the class so there should alway be at least one instance "alive". Imho, This warning is not reported by compiler, only by Intellisense.

A really useful idea - well worth creating.
Missing #include <memory>.  With that fixed, I get almost clean compilation:
255367.cpp:9:5: warning: ‘LazySharedPtr<int, int>::init’ should be initialized in the member initialization list [-Weffc++]

I agree, we should use the initialisation list for init:
LazySharedPtr(Args ... args)
    : init{[args = std::make_tuple(std::forward<Args>(args) ...)]() mutable
           {
               return std::apply([](auto&& ... args) {
                                     return std::make_shared<T>(std::forward<Args>(args) ...);
                                 }, std::move(args));
           }},
      ptr{}
{}

I don't see why we have the inner lambda.  There's no good reason not to pass std::make_shared() directly to std::apply(), like this:
LazySharedPtr(Args ... args)
    : init{[args = std::make_tuple(std::forward<Args>(args) ...)]() mutable
           {
               return std::apply(std::make_shared<T, Args...>, std::move(args));
           }},
      ptr{}
{}

I think the class could be easier to use.  There's no need for the argument types to be part of the class itself, as they are erased by std::function.  So make the class template have only T as argument.  Otherwise we need separate code paths for our smart pointers if they are created differently.  Much better to separate the type that is essential to the LazySharedPtr from those which are accidental:
template <class T>
class LazySharedPtr {
public:
    template <typename ... Args>
    LazySharedPtr(Args ... args);
}

There's some functionality missing that I would expect from a drop-in replacement for std::shared_ptr:

operator*
get()
reset()

I'd also expect a LazySharedPtr to be assignable to std::shared_ptr (invoking the creator function in the process).  That could reduce the need for these functions (especially reset(), which might not make sense here).
On the other hand, I don't think that LazySharedPtr is intended as a base class for inheritance, so shouldn't need a virtual destructor.  And we don't need IsInited(), given we already have operator bool.
Do have a think about what it means to copy a lazy-pointer object.  As it stands, copying a materialized instance yields another shared pointer to the same T object, but copying an unmaterialized instance will result in different T objects in the original and the copy.  I wonder if this could make it difficult to use correctly; we might want to make this a move-only type, and require casting to std::shared_ptr in order to copy (thus materialising the object).
For efficiency, InitAndGet() shouldn't copy the shared pointer, but return a reference.  I'm not fond of the naming - C++ convention uses snake_case for function names.
There's a lot of unnecessary this-> cluttering the code.

Modified code
#include <cstddef>
#include <functional>
#include <memory>
#include <tuple>
#include <utility>

template <class T>
class LazySharedPtr {
public:
    template <typename... Args>
    LazySharedPtr(Args... args)
        : ptr{},
          init{[args = std::make_tuple(std::move<Args>(args)...)]() mutable
               { return std::apply(std::make_shared<T, Args...>, std::move(args)); }}
    {}

    // compiler-defaulted copy/move construct and assign, and destructor

    auto operator->() const
    { return object().get(); }

    auto operator*() const
    { return *object(); }

    auto operator[](std::ptrdiff_t idx)
    { return object()[idx]; }

    explicit operator bool() const noexcept
    { return ptr; }

    explicit operator std::shared_ptr<T>() const
    { return object(); }

private:
    mutable std::shared_ptr<T> ptr;
    std::function<std::shared_ptr<T>()> init;

    auto& object() const
    {
        if (!ptr) { ptr = init(); }
        return ptr;
    }
};

int main()
{
    LazySharedPtr<int> a{0};
    auto b = std::shared_ptr<int>{a};
    return *b;
}


Var scope on the same line Is there a better way I can code this? It's old code and I'm not sure makes sense today. JSLint gives me a lot of errors too. I don't have experience with JavaScript and I would like to optimize this snippet.

Any suggestion? Maybe I can achieve better code with JQuery?

<script type="text/javascript">
// <![CDATA[
    var jump_page = '{LA_JUMP_PAGE}:',
    var on_page = '{ON_PAGE}';
    var per_page = '{PER_PAGE}';
    var base_url = '{A_BASE_URL}';
    var style_cookie = 'phpBBstyle';
    var style_cookie_settings = '{A_COOKIE_SETTINGS}';
    var onload_functions = new Array();
    var onunload_functions = new Array();

    /**
    * Find a member
    */
    function find_username(url)
    {
        popup(url, 760, 570, '_usersearch');
        return false;
    }

    /**
    * New function for handling multiple calls to window.onload and window.unload by pentapenguin
    */
    window.onload = function()
    {
        for (var i = 0; i < onload_functions.length; i++)
        {
            eval(onload_functions[i]);
        }
    };

    window.onunload = function()
    {
        for (var i = 0; i < onunload_functions.length; i++)
        {
            eval(onunload_functions[i]);
        }
    };

// ]]>
</script>

This is how an handler is called for example:

<script type="text/javascript">
// <![CDATA[
    /**
    * Change language
    */
    function change_language(lang_iso)
    {
        document.forms['register'].change_lang.value = lang_iso;
        document.forms['register'].submit.click();
    }

    <!-- IF CAPTCHA_TEMPLATE and S_CONFIRM_REFRESH -->
    onload_functions.push('apply_onkeypress_event()');
    <!-- ENDIF -->

// ]]>
</script>

The modern way to attach onLoad and onUnload handlers is to use window.addEventListener('load', onloadFunction) and window.addEventListener('unload', onunloadFunction).  Unlike window.onload, you can attach multiple handlers, saving you the work of writing your own support for multiple handlers.

Unfortunately, Internet Explorer only added support for addEventListener() in IE 9.  Therefore, you'll probably need a shim.  That would be a common shim that supports a standard interface, rather than your own hack, so I would still consider it an improvement.


Catch the turtle - Python I made a game in python, where the objective is to catch a 'turtle'. The controls are the arrow keys.

#Catch the turtle
import turtle
import math
import random

score = 0
print ("\n" * 40)
print("Your score is:\n0")

#Title
t=turtle.Pen()
t.pencolor("Blue")
t.hideturtle()
t.penup()
t.setposition(-70,350)
t.write("Catch the turtle", font=("Verdana", 18))

#Tip
text=turtle.Pen()
t.pencolor("Red")
t.hideturtle()
t.penup()
t.setposition(-70, -350)
t.write("DON'T TOUCH THE EDGES!", font=("Verdana", 18))

#Set up screen
wn = turtle.Screen()
wn.bgcolor("lightblue")
wn.title("Catch the Turtle")

#Draw border
mypen = turtle.Turtle()
mypen.penup()
mypen.speed(10)
mypen.hideturtle()
mypen.setposition(-300,-300)
mypen.pendown()
mypen.pensize(3)
for side in range(4):
    mypen.color("yellow")
    mypen.forward(300)
    mypen.color("black")
    mypen.forward(300)
    mypen.left(90)
mypen.hideturtle()

#Create player turtle
player = turtle.Turtle()
player.color("blue")
player.shape("arrow")
player.penup()
player.speed(0)

#Create goal
goal = turtle.Turtle()
goal.color("red")
goal.shape("turtle")
goal.penup()
goal.speed(0)
goal.setposition(-100, -100)

#Set speed
speed = 1

#Define functions

def turnleft():
    player.left(30)

def turnright():
    player.right(30)

def increasespeed():
    global speed
    speed +=0.5

def decreasespeed():
    global speed
    speed -= 1

#Set keyboard binding
turtle.listen()
turtle.onkey(turnleft, "Left")
turtle.onkey(turnright, "Right")
turtle.onkey(increasespeed, "Up")
turtle.onkey(decreasespeed, "Down")

while True:
    player.forward(speed)

    #Boundary check
    if player.xcor() > 300 or player.xcor() < -300:
        print("GAME OVER")
        quit()

    if player.ycor() > 300 or player.ycor() < -300:
        print("Game OVER")
        quit()

    #Collision checking
    d= math.sqrt(math.pow(player.xcor()-goal.xcor(),2) + math.pow(player.ycor()-goal.ycor(),2))
    if d < 20 :
        goal.setposition(random.randint(-300,300), random.randint(-300, 300))
        score = score + 1
        print ("\n" * 40)
        print("Your score is")
        print (score)

Using Variables

score = 0
print ("\n" * 40)
print("Your score is:\n0")

You are setting the score to 0, then printing the value 0 as the score. What if you decided, that the player starts with 100 score points? You would have to change the value in two places. Since you already have the value in a variable, it is better to print the value of the variable. Like this: print('Your score is {0}'.format(score)) Note that I also removed the newline from the output, because Your score is 0 reads much more naturally than
Your score is:
0.

Using Functions

#Set up screen
wn = turtle.Screen()
wn.bgcolor("lightblue")
wn.title("Catch the Turtle")

This comment already reads like a function name, so pack the code inside of a function. This way you don't even need the comment, because the function name tells you all you have to know. It could look like this:

def setup_screen():
    wn = turtle.Screen()
    wn.bgcolor("lightblue")
    wn.title("Catch the Turtle")

Even better you can add parameters, to be able to easily change how the screen is setup without having to read and change the setup code:

def setup_screen(background_color, title):
    wn = turtle.Screen()
    wn.bgcolor(background_color)
    wn.title(title)

That way you achieve a good level of abstraction, where you don't have to read all of the details everytime you want to change something.

The same applies to many more blocks of code that begin with such a comment that sounds like "do this and that".

Logic and Bugs

#Title
t=turtle.Pen()
t.pencolor("Blue")
t.hideturtle()
t.penup()
t.setposition(-70,350)
t.write("Catch the turtle", font=("Verdana", 18))

#Tip
text=turtle.Pen()
t.pencolor("Red")
t.hideturtle()
t.penup()
t.setposition(-70, -350)
t.write("DON'T TOUCH THE EDGES!", font=("Verdana", 18))

In the second block, you are first initializing a variable text, but then you continue using the variable t that you used in the block before. This way you are changing values that you already set, and not changing others that you expect to be changed. I think all occurences of t in the second block should be text instead. However, if you were using functions as I explained above, this problem would not occur and you would be less likely to get this bug, which is probably caused by copy-pasting code and forgetting to change it.

#Boundary check
if player.xcor() > 300 or player.xcor() < -300:
    print("GAME OVER")
    quit()

if player.ycor() > 300 or player.ycor() < -300:
    print("Game OVER")
    quit()

This is redundant. You have the same code twice, with just different conditions. You can already see why this is bad: You typed the output strings differently without realising, so in some cases when the player is out of bounds the output will be GAME OVER, in some cases it will be Game OVER. This is a bug, and a very simple example of bugs being introduced or at least made much harder to find by redundant code.

To fix it you can either combine the logical expressions into one condition:

#Boundary check
if (player.xcor() > 300 or
    player.xcor() < -300 or
    player.ycor() > 300 or
    player.ycor() < -300):
    print("GAME OVER")
    quit()

Or, even better, define a function to check the bounds (this is way more readable and maintainable):

def is_player_in_bounds():
    return (player.xcor() < 300 and
            player.xcor() > -300 and
            player.ycor() < 300 and
            player.ycor() > -300)

Note that I inverted the logical expression, because I check for in bounds, not for out of bounds. Then you use it like this:

if not is_player_in_bounds():
    print("GAME OVER")
    quit()

#Collision checking
d= math.sqrt(math.pow(player.xcor()-goal.xcor(),2) + math.pow(player.ycor()-goal.ycor(),2))
if d < 20 :
    goal.setposition(random.randint(-300,300), random.randint(-300, 300))
    score = score + 1
    print ("\n" * 40)
    print("Your score is")
    print (score)

d is a very bad name for a variable. Since this looks like a calculation of Euler distance, I assume that it is supposed to mean distance, so better call your variable distance or at least dist.

You can write score += 1 here to make it shorter.

You might want to put the ouput code into a function (e. g. print_score()), to separate logic from I/O.

math.sqrt(math.pow(player.xcor()-goal.xcor(),2) + math.pow(player.ycor()-goal.ycor(),2)) is hard to read, because of the formatting, and because it is a long formula with variable names, namespaces and method/function calls. Better put it in a function:

def euler_distance(player, goal):
    distance_x = player.xcor() - goal.xcor()
    distance_y = player.ycor() - goal.ycor()
    return math.sqrt(distance_x ** 2 + distance_y ** 2)

Using a ** b for exponentiation is faster and looks cleaner than using math.pow(). Note that player and goal are local variables here, even if they have the same name as the global ones.

Use the function like this:

if euler_distance(player, goal) < 20:
    goal.setposition(random.randint(-300, 300), random.randint(-300, 300))
    score += 1
    print_score()

Even better if you would define the bounds somewhere as variables and access them that way.

Redundance

#Set speed
speed = 1

This comment tells you nothing about the code that the code does not already tell you. It just bloats the code and requires you to read more redundant information. When you read speed = 1, it is perfectly clear that the speed is being set. Comments should tell you why the code does something (if needed), while the code tells you what it does.

def turnleft():
    player.left(30)

def turnright():
    player.right(30)

These functions do not add any value. (Hint: Just after reviewing these functions I saw why they are defined, which is using them for event handling. In this case they do add value, but I will leave the info for understanding.) player.left(30) gives you more information than turnleft(). If you read turnleft() somewhere in your code, you do not know what is turned, you do not know how much it is turned. Whenever you want to write a function that only contains one or two lines, think about whether the one line or two lines would be more readable and give you more valuable information.

def increasespeed():
    global speed
    speed +=0.5

def decreasespeed():
    global speed
    speed -= 1

Here we have the same, but even worse: The functions change global state, which can make it much harder to find bugs. Try to avoid that whenever possible. Furthermore, speed += 0.5 and speed -= 1 already tell you that the speed is increased/decreased, and they tell you how much they are changed, which is valuable information. If your only reason for defining these functions is that you can easily change how much the speed is increased/decreased, define variables instead and do speed += SPEED_INCREASE. Writing it in uppercase later reminds you not to change it.

#Set keyboard binding
turtle.listen()
turtle.onkey(turnleft, "Left")
turtle.onkey(turnright, "Right")
turtle.onkey(increasespeed, "Up")
turtle.onkey(decreasespeed, "Down")

Since we said above that not defining these functions might be better, you can use lambdas as callbacks. Lambdas are anonymous functions, that means you do not have to define them with def, but you can just use them where you need them. In the cases where you just change a variable however, you will still need the function. This is what it could look like:

#Set keyboard binding
turtle.listen()
turtle.onkey(lambda: player.left(30), "Left")
turtle.onkey(lambda: player.right(30), "Right")
turtle.onkey(increasespeed, "Up")
turtle.onkey(decreasespeed, "Down")


User prompts for entering quiz questions I have two methods here  that does the same thing: get the user input and validate them.

If they enter a letter, the program will prompt them again to enter a number. |After that, if it's a number it will compare it to the bound given by the program or the user. It is working fine, but can anybody help me refactor this code? It would be really helpful if I could dissect the validations into smaller pieces of code or methods.

public void addToQuestionToQbank(){
        this.askForCategory();
        System.out.println("Add Questions[1]Multiple Choice[2] Identification: ");
        int choice;
        boolean notValid = true;
        do{
            while(!input.hasNextInt()){
                System.out.println("Numbers Only!");
                System.out.print("Try again: ");
                input.nextLine();
            }
                notValid = false;
                choice = input.nextInt();
        }while(notValid);

        while(choice > 2){
            System.out.println("Out of bounds");
            choice = input.nextInt();
            input.nextLine();
        }
    }

public Difficulty askForDifficulty(){
        System.out.println("Difficulty For This Question:\n1)Easy\n2)Medium\n3)Hard\nChoice: ");
        int choice = 0;

        boolean notValid = true;
        do{
            while(!input.hasNextInt()){
                System.out.println("Numbers Only!");
                System.out.print("Try again: ");
                input.nextLine();
            }
                notValid = false;
                choice = input.nextInt();
        }while(notValid);

        while(choice > diff.length){
            System.out.println("Out of bounds");
            choice = input.nextInt();
            input.nextLine();
        }
        System.out.println(diff[choice -1]);
       return diff[choice -1];
    }

There are 2 problems that must be addressed first. Where did diff.choice come from, what is that? Second. What is the this.askForCategory() call? It sounds like exactly what addToQuestionToQbank() is doing. addToQuestionToQbank() does not do what it says, so I wonder what the real adding method looks like ....

Anyway, I would expect the calling code to look something like this:

public void newQuestion () {
    Question myQuestion = new Question();

    myQuestion.Question = this.askForQuestion();
    myQuestion.Answer = this.askForAnswer ();

    // here is where we call your code, refactored.
    myQuestion.category = this.multiChoicePrompt("Add Questions[1]Multiple Choice[2] Identification: ", 2);
    myQuestion.Difficulty = this.multiChoicePrompt("Difficulty For This Question:\n1)Easy\n2)Medium\n3)Hard\nChoice: " ,3);

    this.addQuestionToQbank(myQuestion);
}

By passing the prompt as a parameter we can now refactor as you asked:

// this prompt designed specifically to pick from multiple choices
// note how the method name makes that comment irrelevant.
public int multiChoicePrompt(String askUser, int responseMaxValue) {
    System.out.println(askUser);

    int choice;
    boolean notValid = true;
    do{
        while(!input.hasNextInt()){
            System.out.println("Numbers Only!");
            System.out.print("Try again: ");
            input.nextLine();
        }
            notValid = false;
            choice = input.nextInt();
    }while(notValid);

    // assume numbered choices always start with "1"
    while(choice > responseMaxValue || choice < 1){
        System.out.println("Out of bounds");
        choice = input.nextInt();
        input.nextLine();
    }
    return choice;
}

The easiest way to "extract method" might be to physically move all that code to the new method and then recompile and let the compiler errors tell you what variables it doesn't "know"  - these are what you pass in as parameters.


Delete an item from a linked list I wrote this code to delete an item from a linked list. I checked it and it works, but I want to know if the logic could be written simpler or clearer.

typedef struct linked_list{
  int value;
  struct linked_list * next;
}linked;

int deleteSpecificItem(linked ** head, int val)
{
 if(!head || !(*head)) return -1;
 linked * tmp = *head;
 int ret;
 linked * listToDel;
 if(tmp->value == val)
 {
    *head = (*head)->next;
    ret = tmp->value;
    free(tmp);
    return ret;
 }
 while(tmp->next != NULL)
 {
    if(tmp->next->value == val)
    {
        listToDel = tmp->next;
        tmp->next = tmp->next->next;
        ret = listToDel->value;
        free(listToDel);
        return ret;
    }
    tmp = tmp->next;
 }
 return -1;
}

When checking if a pointer is NULL, I can use either !head or head != NULL. What is preferred in companies?

Apart from the less-than-ideal variable and type names already mentioned you are doubling up on code just to check head first and then separately doing the same again for every other node. The only difference is that you need to modify head if the element to be removed is the first one. This can be condensed into a single loop:

int deleteSpecificItem(linked** head, int val)
{
    if (!head || !(*head)) return -1;

    linked* tmp = *head;
    linked* prev = NULL;

    while (tmp->value != val && tmp->next != NULL)
    {
        prev = tmp;
        temp = temp->next;
    }

    if (tmp->value == val)
    {
        if (prev)
        {
            prev->next = tmp->next;
        }
        else
        {
            *head = tmp->next;
        }
        free(tmp);
        return val;
    }

    return -1;
}

The way this works is: First we try to find the node which should be removed and we also keep a pointer to the previous node. Once the loop finishes there are several cases to check:

If the current value is not equal to the one we are looking then it's not in the list and we can return
If the current value is equal to the one we are looking for then tmp points to the node which needs to be removed

If we have a valid prev then rewire by skipping tmp
If we don't have a valid prev then apparently head needs to be moved
Now the current node is unlinked and we can delete it
We can also return val instead of remembering the value of the current node because we know that they are equal.

Regarding !head vs head == NULL: I tend to prefer !head if it's for simple pointer checks and prev->next == NULL when the expression is more complex. However some companies enforce the style NULL == head because a typo like typing = instead of == will yield in a compile error this way (you can't assign something to a constant) instead of an accidental assignment. I never managed to consistently do it that way (old habits die hard) and to be honest I've never run into a bug due to this (compilers these days spit out warnings for these kind of constructs).


Fill a pandas dataframe (of predetermined size) with results from a similar dataframe (of size less than or equivalent to the original) I'd like some feedback/suggestions on how to improve the following. Specifically I want to know if what I'm doing is reliable and fast, or if there is a better way to accomplish this.
The problem:
I have some dataset containing counts of sales made at different times throughout the day, across different locations/shops. Let's say there are 4 different shops contained in this data (A, B, C, D), and there are 4 different time bins in the day [0,1,2,3]. I query this and return a query dataset, but the issue I have is that for this query there may be no transactions for a certain time bin. Or there may be no transactions even for a specific shop (maybe there was a rat infestation and it closed for the day).
Nevertheless, the end result must have the same number of rows (4 locations x 4 time bins), and simply contain zeros if there were no transactions there. In other words, I want records for all possible occurrences, even if they were not returned by the query itself.
Example:
import pandas as pd

# Specify the complete list of possible time bins
max_timebins = 3
bin_nums = list(range(max_timebins + 1))

# Specify the complete list of shops
shop_ids = ['A', 'B','C','D']

# Make a dataframe for all possible results without the counts
# This is just a dataframe with an index and no columns... this feels a little strange to me but it worked...
dat = {'shop':[], 'timebin':[]}
for shop in shop_ids:
    dat['shop']+=[shop]*len(bin_nums)
    dat['timebin'] += bin_nums
df_all = pd.DataFrame(dat)
df_all = df_all.set_index(list(dat.keys()))

# Example of a result of a query
dfq = pd.DataFrame(
    {
        'shop':['A', 'A', 'A', 'A',
                'B', 'B',
                'C', 'C', 'C',
                'D'],
        'time_bins':[0,1,2,3,
                     0, 3,
                     0,2,3,
                     2],
        'counts':[100,220, 300, 440,
                  500, 660,
                  120, 340, 90,
                  400]}).set_index(['shop', 'time_bins'])

result_df = pd.concat([df_all, dfq], axis=1).fillna(0).astype(int)

You can create the index directly:
pd.MultiIndex.from_product(
    (shop_ids, range(max_timebins + 1)), names=("shop", "timebin")
)

Here is how you can achieve the same result (including the full index even if there is no data) in a simpler way:
import pandas as pd

# Specify the complete list of possible time bins
max_timebins = 3

# Specify the complete list of shops
shop_ids = ["A", "B", "C", "D"]

target_index = pd.MultiIndex.from_product(
    (shop_ids, range(max_timebins + 1)), names=("shop", "timebin")
)

# Example of a result of a query
dfq = pd.DataFrame(
    {
        "shop": ["A", "A", "A", "A", "B", "B", "C", "C", "C", "D"],
        "time_bins": [0, 1, 2, 3, 0, 3, 0, 2, 3, 2],
        "counts": [100, 220, 300, 440, 500, 660, 120, 340, 90, 400],
    }
).set_index(["shop", "time_bins"])

result_df = dfq.reindex(target_index, fill_value=pd.NA)

print(result_df)

Output:
             counts
shop timebin
A    0          100
     1          220
     2          300
     3          440
B    0          500
     1         <NA>
     2         <NA>
     3          660
C    0          120
     1         <NA>
     2          340
     3           90
D    0         <NA>
     1         <NA>
     2          400
     3         <NA>

There might be a better solution, but that would require knowing a bit more context.